Skip to content
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

Merged
merged 7 commits into from
Jun 28, 2016
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
27 changes: 26 additions & 1 deletion extensions/amp-form/0.1/amp-form.css
Original file line number Diff line number Diff line change
Expand Up @@ -16,5 +16,30 @@

form.amp-form-submit-success [submit-success],
form.amp-form-submit-error [submit-error] {
display: block;
display: block;
}


.-amp-validation-bubble {
transform: translate(-50%, -100%);
background-color: white;
box-shadow: 0 5px 15px 0 rgba(0, 0, 0, .5);
max-width: 200px;
position: absolute;
display: none;
box-sizing: border-box;
padding: 10px;
border-radius: 5px;
}

.-amp-validation-bubble:after {
content: ' ';
position: absolute;
bottom: -8px;
left: 30px;
width: 0;
height: 0;
border-left: 8px solid transparent;
border-right: 8px solid transparent;
border-top: 8px solid #fff;
}
93 changes: 89 additions & 4 deletions extensions/amp-form/0.1/amp-form.js
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -37,6 +39,9 @@ const FormState_ = {
SUBMIT_SUCCESS: 'submit-success',
};

/** @type {?./validation-bubble.ValidationBubble|undefined} */
let validationBubble;
Copy link
Member

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done


export class AmpForm {

/**
Expand All @@ -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_);

Expand Down Expand Up @@ -94,12 +102,16 @@ 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({mutate: reportValidity}, {form: this.form_});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll need to use the same {measure: undefined, mutate: reportValidity} hidden class. 😞

I'll file an issue to add state to #measure and #mutate.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do. This is for type matching of the Task def, right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

return;
}

Expand Down Expand Up @@ -146,7 +158,7 @@ export class AmpForm {
}

/**
* @param {!Object} data
* @param {!Object=} data
* @private
*/
renderTemplate_(data = {}) {
Expand Down Expand Up @@ -176,9 +188,75 @@ export class AmpForm {
}


function reportValidity(state) {
reportFormValidity(state.form);
}

/**
* Reports validity for the first invalid input - if any.
* @param {!HTMLFormElement} form
* @private
*/
function reportFormValidity(form) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Naming here is odd, since we're actually reporting invalidity.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The native API is called reportValidity.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack.

const inputs = toArray(form.querySelectorAll('input,select,textarea'));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to move this to dom.js and implement via :scope when available.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a need to toArray?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need. I had it earlier because I was doing forEach but no longer.

Copy link
Contributor Author

@mkhatib mkhatib Jun 24, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dvoytenko for moving this to dom.js When :scope is not available do we just do querySelectorAll or did you want us to navigate the DOM and check child by child (and its children and so on)?

I'll do this in a separate PR as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's always element.querySelectorAll('.class') or such. The trick is that adding :scope to it is a lot faster. The problem: :scope is not available everywhere so we can't do it universally. An inconvenience for a generic solution is that you have to have :scope for each sub-selector. I.e. input, select becomes :scope input, :scope select.

Alternatively, you can use element.getElementsByTagName() API or such: obviously given three selectors this might not be better.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. Writing up a separate PR for the dom.js changes. Thanks for the explanation

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sent out a PR #3778

Copy link
Contributor Author

@mkhatib mkhatib Jun 27, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed in #3778 this isn't necessary. Dropped the toArray party.

Copy link
Member

Choose a reason for hiding this comment

The 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
* @private
*/
function onInvalidInputKeyUp_(event) {
validationBubble.hide();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will schedule two VSync tasks, when only one is ever necessary. Either it's valid, and we can hide it, or it's invalid, and we need to update and center it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

if (!event.target.checkValidity()) {
validationBubble.show(event.target, event.target.validationMessage);
}
}


/**
* Hides validation bubble and removes listeners on the invalid input.
* @param {!Event} event
* @private
*/
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
* @private
*/
function reportInputValidity(input) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto naming.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
* @private
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there's a need to mark un-exported functions as private.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that makes sense, @erwinmombay @cramforce shoild we not use @private at all when defining file-level methods/vars?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

On Fri, Jun 24, 2016 at 9:44 AM, Justin Ridgewell notifications@github.com
wrote:

In extensions/amp-form/0.1/amp-form.js
#3734 (comment):

/**

  • Installs submission handler on all forms in the document.
  • @param {!Window} win

I don't think there's a need to mark un-exported functions as private.


You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
https://github.com/ampproject/amphtml/pull/3734/files/957c812439bb3654460089220b746a2cff48ca96#r68425990,
or mute the thread
https://github.com/notifications/unsubscribe/AAFeT7uaY0EY2nyCT6InRZN1wVEQrVAoks5qPAl1gaJpZM4I8WLq
.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

*/
function installSubmissionHandlers(win) {
onDocumentReady(win.document, () => {
Expand All @@ -189,10 +267,17 @@ function installSubmissionHandlers(win) {
}


/**
* @param {!Window} win
* @private
*/
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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I like having a global validationBubble. Why not just make it an instance variable on AmpForm? What if there are multiple forms on the page?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 {};
});
Expand Down
106 changes: 106 additions & 0 deletions extensions/amp-form/0.1/validation-bubble.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,106 @@
/**
* 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({
mutate: hideBubble,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto {measure, mutate} hidden class.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will it ever calculate with display: none?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

targetElement is not display:none that is the bubbleElement.

}


/**
* 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',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is display: initial supported by all platforms we need? It might be better to just set it to ''

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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`,
});
}