-
Notifications
You must be signed in to change notification settings - Fork 3.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Polyfill reportValidation calls and bubble UI for webkit. #3734
Changes from 4 commits
26ba3f7
957c812
910fbee
07ed9b0
c04f97b
29bfdc7
ca352f6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,6 +26,8 @@ import {templatesFor} from '../../../src/template'; | |
import {removeElement, childElementByAttr} from '../../../src/dom'; | ||
import {installStyles} from '../../../src/styles'; | ||
import {CSS} from '../../../build/amp-form-0.1.css'; | ||
import {ValidationBubble} from './validation-bubble'; | ||
import {vsyncFor} from '../../../src/vsync'; | ||
|
||
/** @type {string} */ | ||
const TAG = 'amp-form'; | ||
|
@@ -37,6 +39,9 @@ const FormState_ = { | |
SUBMIT_SUCCESS: 'submit-success', | ||
}; | ||
|
||
/** @type {?./validation-bubble.ValidationBubble|undefined} */ | ||
let validationBubble; | ||
|
||
export class AmpForm { | ||
|
||
/** | ||
|
@@ -50,6 +55,9 @@ export class AmpForm { | |
/** @const @private {!Element} */ | ||
this.form_ = element; | ||
|
||
/** @const @private {!../../../src/service/vsync-impl.Vsync} */ | ||
this.vsync_ = vsyncFor(this.win_); | ||
|
||
/** @const @private {!Templates} */ | ||
this.templates_ = templatesFor(this.win_); | ||
|
||
|
@@ -94,12 +102,19 @@ export class AmpForm { | |
* @private | ||
*/ | ||
handleSubmit_(e) { | ||
if (e.defaultPrevented) { | ||
if (this.state_ == FormState_.SUBMITTING) { | ||
e.preventDefault(); | ||
return; | ||
} | ||
|
||
if (this.state_ == FormState_.SUBMITTING) { | ||
const shouldValidate = !this.form_.hasAttribute('novalidate'); | ||
if (shouldValidate && | ||
this.form_.checkValidity && !this.form_.checkValidity()) { | ||
e.preventDefault(); | ||
this.vsync_.run({ | ||
measure: undefined, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add a TODO #3776, please. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
mutate: reportValidity, | ||
}, {form: this.form_}); | ||
return; | ||
} | ||
|
||
|
@@ -146,7 +161,7 @@ export class AmpForm { | |
} | ||
|
||
/** | ||
* @param {!Object} data | ||
* @param {!Object=} data | ||
* @private | ||
*/ | ||
renderTemplate_(data = {}) { | ||
|
@@ -176,6 +191,73 @@ export class AmpForm { | |
} | ||
|
||
|
||
/** | ||
* Reports validity of the form passed through state object. | ||
* @param {!Object} state | ||
*/ | ||
function reportValidity(state) { | ||
reportFormValidity(state.form); | ||
} | ||
|
||
|
||
/** | ||
* Reports validity for the first invalid input - if any. | ||
* @param {!HTMLFormElement} form | ||
*/ | ||
function reportFormValidity(form) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Naming here is odd, since we're actually reporting invalidity. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The native API is called reportValidity. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ack. |
||
const inputs = toArray(form.querySelectorAll('input,select,textarea')); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need to move this to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a need to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need. I had it earlier because I was doing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dvoytenko for moving this to I'll do this in a separate PR as well. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's always Alternatively, you can use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good. Writing up a separate PR for the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sent out a PR #3778 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As discussed in #3778 this isn't necessary. Dropped the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. very anecdotal but :scope has always been faster for me because querySelectorAll is not element rooted in the way it works. (it does not work the way you think it does) see http://ejohn.org/blog/thoughts-on-queryselectorall/ for explanation. in saying that, not too sure if its worth the trouble. (unless its some hot code) |
||
for (let i = 0; i < inputs.length; i++) { | ||
if (!inputs[i].checkValidity()) { | ||
reportInputValidity(inputs[i]); | ||
break; | ||
} | ||
} | ||
} | ||
|
||
|
||
/** | ||
* Revalidates the currently focused input after a change. | ||
* @param {!KeyboardEvent} event | ||
*/ | ||
function onInvalidInputKeyUp_(event) { | ||
if (event.target.checkValidity()) { | ||
validationBubble.hide(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One last: We may end up double calling There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah we'd also need to store the targetElement and visibility state of the bubble in that state to make sure we're calling show for the same element as well. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Filed a bug to track this will do in another PR #3782 |
||
} else { | ||
validationBubble.show(event.target, event.target.validationMessage); | ||
} | ||
} | ||
|
||
|
||
/** | ||
* Hides validation bubble and removes listeners on the invalid input. | ||
* @param {!Event} event | ||
*/ | ||
function onInvalidInputBlur_(event) { | ||
validationBubble.hide(); | ||
event.target.removeEventListener('blur', onInvalidInputBlur_); | ||
event.target.removeEventListener('keyup', onInvalidInputKeyUp_); | ||
} | ||
|
||
|
||
/** | ||
* Focuses and reports the invalid message of the input in a message bubble. | ||
* @param {!HTMLInputElement} input | ||
*/ | ||
function reportInputValidity(input) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ditto naming. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ack. |
||
input./*OK*/focus(); | ||
|
||
// Remove any previous listeners on the same input. This avoids adding many | ||
// listeners on the same element when the user submit pressing Enter or any | ||
// other method to submit the form without the element losing focus. | ||
input.removeEventListener('blur', onInvalidInputBlur_); | ||
input.removeEventListener('keyup', onInvalidInputKeyUp_); | ||
|
||
input.addEventListener('keyup', onInvalidInputKeyUp_); | ||
input.addEventListener('blur', onInvalidInputBlur_); | ||
validationBubble.show(input, input.validationMessage); | ||
} | ||
|
||
|
||
/** | ||
* Installs submission handler on all forms in the document. | ||
* @param {!Window} win | ||
|
@@ -189,10 +271,16 @@ function installSubmissionHandlers(win) { | |
} | ||
|
||
|
||
/** | ||
* @param {!Window} win | ||
*/ | ||
function installAmpForm(win) { | ||
return getService(win, 'amp-form', () => { | ||
if (isExperimentOn(win, TAG)) { | ||
installStyles(win.document, CSS, () => installSubmissionHandlers(win)); | ||
installStyles(win.document, CSS, () => { | ||
validationBubble = new ValidationBubble(window); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure I like having a global There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We really only need one validation bubble. Even with multiple forms, we only need one because we're always showing the bubble on one field at a time. I don't really see reasons to have multiple instances here. |
||
installSubmissionHandlers(win); | ||
}); | ||
} | ||
return {}; | ||
}); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,107 @@ | ||
/** | ||
* Copyright 2016 The AMP HTML Authors. All Rights Reserved. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS-IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
|
||
import {vsyncFor} from '../../../src/vsync'; | ||
import {viewportFor} from '../../../src/viewport'; | ||
import {setStyles} from '../../../src/style'; | ||
|
||
export class ValidationBubble { | ||
|
||
/** | ||
* Creates a bubble component to display messages in. | ||
* @param {!Window} win | ||
*/ | ||
constructor(win) { | ||
|
||
/** @private @const {!Viewport} */ | ||
this.viewport_ = viewportFor(win); | ||
|
||
/** @private @const {!../../../src/service/vsync-impl.Vsync} */ | ||
this.vsync_ = vsyncFor(win); | ||
|
||
/** @private @const {!HTMLDivElement} */ | ||
this.bubbleElement_ = win.document.createElement('div'); | ||
this.bubbleElement_.classList.add('-amp-validation-bubble'); | ||
win.document.body.appendChild(this.bubbleElement_); | ||
} | ||
|
||
/** | ||
* Hides the bubble off screen. | ||
*/ | ||
hide() { | ||
this.vsync_.run({ | ||
measure: undefined, | ||
mutate: hideBubble, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ditto There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
}, { | ||
bubbleElement: this.bubbleElement_, | ||
}); | ||
} | ||
|
||
/** | ||
* Shows the bubble targeted to an element with the passed message. | ||
* @param {!HTMLElement} targetElement | ||
* @param {string} message | ||
*/ | ||
show(targetElement, message) { | ||
const state = { | ||
message, | ||
targetElement, | ||
bubbleElement: this.bubbleElement_, | ||
viewport: this.viewport_, | ||
}; | ||
this.vsync_.run({ | ||
measure: measureTargetElement, | ||
mutate: showBubbleElement, | ||
}, state); | ||
} | ||
} | ||
|
||
|
||
/** | ||
* Hides the bubble element passed through state object. | ||
* @param {!Object} state | ||
* @private | ||
*/ | ||
function hideBubble(state) { | ||
setStyles(state.bubbleElement, { | ||
display: 'none', | ||
}); | ||
} | ||
|
||
|
||
/** | ||
* Measures the layout for the target element passed through state object. | ||
* @param {!Object} state | ||
* @private | ||
*/ | ||
function measureTargetElement(state) { | ||
state.targetRect = state.viewport.getLayoutRect(state.targetElement); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will it ever calculate with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
} | ||
|
||
|
||
/** | ||
* Updates text content, positions and displays the bubble. | ||
* @param {!Object} state | ||
* @private | ||
*/ | ||
function showBubbleElement(state) { | ||
state.bubbleElement.textContent = state.message; | ||
setStyles(state.bubbleElement, { | ||
display: 'initial', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
top: `${state.targetRect.top - 10}px`, | ||
left: `${state.targetRect.left + state.targetRect.width / 2}px`, | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add undefined to allowed type if we dont init with null
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done