Allow basic usage of form and polyfill validate check globally #3373
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,68 @@ | ||
| <!doctype html> | ||
| <html ⚡> | ||
| <head> | ||
| <meta charset="utf-8"> | ||
| <title>Forms Examples in AMP</title> | ||
| <link rel="canonical" href="amps.html" > | ||
| <meta name="viewport" content="width=device-width,minimum-scale=1,initial-scale=1"> | ||
| <style amp-boilerplate>body{-webkit-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-moz-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-ms-animation:-amp-start 8s steps(1,end) 0s 1 normal both;animation:-amp-start 8s steps(1,end) 0s 1 normal both}@-webkit-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-moz-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-ms-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-o-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}</style><noscript><style amp-boilerplate>body{-webkit-animation:none;-moz-animation:none;-ms-animation:none;animation:none}</style></noscript> | ||
| <script async src="https://cdn.ampproject.org/v0.js"></script> | ||
| <style amp-custom> | ||
| form.amp-form-invalid input:invalid { | ||
| background-color: #dc4e41; | ||
| } | ||
|
|
||
| form .invalid-message, | ||
| /* was invalid but now valid */ | ||
| form.amp-form-invalid:valid .invalid-message, | ||
| form .now-valid-message { | ||
| display: none; | ||
| } | ||
|
|
||
| form.amp-form-invalid .invalid-message { | ||
| display: block; | ||
| color: #dc4e41; | ||
| } | ||
|
|
||
| /* was invalid but now valid */ | ||
| form.amp-form-invalid:valid .now-valid-message { | ||
| display: block; | ||
| color: #4CAF50; | ||
| } | ||
| </style> | ||
| </head> | ||
| <body> | ||
| <h2>Subscribe to our weekly Newsletter</h2> | ||
|
|
||
| <!-- | ||
| AMP would add a class to reflect the validity state of the form after a | ||
| user tries to submit (.amp-form-valid, .amp-form-invalid) unlike | ||
| (:valid, :invalid) pseudo classes which the browser applies them directly | ||
| with no user interaction. | ||
|
|
||
| This way publishers can control when their success and error messages | ||
| appear - specially on browsers that doesn't have validation messages | ||
| reporting built yet. | ||
| --> | ||
| <form method="post" action="http://localhost:8000/form/echo-html/post" target="_blank"> | ||
| <fieldset> | ||
| <label> | ||
| <span>Your name</span> | ||
| <input type="text" name="name" required> | ||
| </label> | ||
| <label> | ||
| <span>Your email</span> | ||
| <input type="email" name="email" required> | ||
| </label> | ||
| <input type="submit" value="Subscribe"> | ||
| </fieldset> | ||
|
|
||
| <div class="invalid-message"> | ||
| There are some input that needs fixing. Please fix them. | ||
| </div> | ||
| <div class="now-valid-message"> | ||
| Thanks for fixing these! Now you can submit! | ||
| </div> | ||
| </form> | ||
| </body> | ||
| </html> | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,111 @@ | ||
| /** | ||
|
There was a problem hiding this comment. I think we should move this to There was a problem hiding this comment. See my comment with respect to whether it should be a service. Adding custom rules to dep-check-config is easy :) There was a problem hiding this comment. Not entirely sure what should be added to There was a problem hiding this comment. The goal is to avoid any circumstances where this handler gets installed twice. But please respond to @cramforce 's note on whether or not this is a service. Generally, though, I believe we've been using services for this kind of features. There was a problem hiding this comment. Ack. Do I need to add anything to |
||
| * Copyright 2015 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 {getService} from './service'; | ||
|
|
||
| /** | ||
| * @param {!Window} window | ||
| */ | ||
| export function installGlobalSubmitListener(window) { | ||
| submitHandlerFor(window); | ||
| } | ||
|
|
||
| /** | ||
| * @param {!Window} window | ||
| */ | ||
| export function uninstallGlobalSubmitListener(window) { | ||
|
There was a problem hiding this comment. We don't seem to use this? Could we simplify this file and get rid of the service? There was a problem hiding this comment. Do we just store a local variable in this file and memoize it? How do we handle multiple window situations? Would something like the following be ok? /**
* @type {?SubmitHandler} Store the global handler for form submits.
*/
let submitHandler_;
/**
* @param {!Window} window
*/
export function installGlobalSubmitListener(window) {
submitHandlerFor(window);
}
/**
* @param {!Window} window
*/
function submitHandlerFor(window) {
return submitHandler_ || (submitHandler_ = new SubmitHandler(window));
}There was a problem hiding this comment. @cramforce let me know if the above is good or if we handle this in a different way? There was a problem hiding this comment. Done. Let me know if this looks good. As per chat, we only need to install this once on the passed window. |
||
| submitHandlerFor(window).cleanup(); | ||
| } | ||
|
|
||
| /** | ||
| * @param {!Window} window | ||
| */ | ||
| function submitHandlerFor(window) { | ||
|
There was a problem hiding this comment. This has been dropped. |
||
| return getService(window, 'submithandler', () => { | ||
| return new SubmitHandler(window); | ||
| }); | ||
| } | ||
|
|
||
| /** | ||
| * Intercept any submit on the current document and polyfill validation | ||
| * check for the requested form. | ||
| */ | ||
| export class SubmitHandler { | ||
| /** | ||
| * @param {!Window} window | ||
| */ | ||
| constructor(window) { | ||
| /** @private @const {!Window} */ | ||
| this.win = window; | ||
|
|
||
| /** @private @const {!function(!Event)|undefined} */ | ||
| this.boundHandle_ = this.handle_.bind(this); | ||
| this.win.document.documentElement.addEventListener( | ||
| 'submit', this.boundHandle_, true); | ||
| } | ||
|
|
||
| /** | ||
| * Removes all event listeners. | ||
| */ | ||
| cleanup() { | ||
|
There was a problem hiding this comment. There is no caller of this. Can this entire class just be inlined into the function |
||
| if (this.boundHandle_) { | ||
| this.win.document.documentElement.removeEventListener( | ||
| 'submit', this.boundHandle_, true); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Intercept any submit on the current document and polyfill validation check. | ||
| * @param {!Event} e | ||
| */ | ||
| handle_(e) { | ||
| onDocumentFormSubmit_(e); | ||
| } | ||
| } | ||
|
|
||
|
|
||
| /** | ||
| * Intercept any submit on the current document and prevent invalid submits from | ||
| * going through. | ||
| * | ||
| * @param {!Event} e | ||
| */ | ||
| export function onDocumentFormSubmit_(e) { | ||
| if (e.defaultPrevented) { | ||
| return; | ||
| } | ||
|
|
||
| const form = e.target; | ||
|
There was a problem hiding this comment. Is target always FORM here? Worth checking as well? There was a problem hiding this comment. It should always be FORM I believe, but added a check just in case. |
||
| if (!form) { | ||
| return; | ||
| } | ||
|
|
||
| // Safari does not trigger validation check on submission, hence we | ||
|
There was a problem hiding this comment. Let's also test There was a problem hiding this comment. What kind of test you want to do here? I thought we can leave these to the validator, no? Should we just check for their existence here or also the value of There was a problem hiding this comment. Even with validator, we still do these checks in runtime to point out problems sooner.
Target must be either |
||
| // trigger it manually. In other browsers this would never execute since | ||
| // the submit event wouldn't be fired if the form is invalid. | ||
| // TODO: This doesn't display the validation error messages. Safari makes them | ||
| // available per input.validity object. We need to figure out a way of | ||
| // displaying these. | ||
| if (form.checkValidity && !form.checkValidity()) { | ||
| form.classList.remove('amp-form-valid'); | ||
|
There was a problem hiding this comment. So, when we check for validity via There was a problem hiding this comment. :invalid would be applied on load to the form not on submit. Happy to drop all the classes from this PR. There was a problem hiding this comment. Let's do. I think it'd be easier to add them in one go later. |
||
| form.classList.add('amp-form-invalid'); | ||
| e.preventDefault(); | ||
| return; | ||
| } | ||
|
|
||
| form.classList.add('amp-form-valid'); | ||
| form.classList.remove('amp-form-invalid'); | ||
| }; | ||
|
There was a problem hiding this comment. No need in this semicolon |
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,89 @@ | ||
| /** | ||
| * Copyright 2015 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 {onDocumentFormSubmit_} from '../../src/document-submit'; | ||
| import * as sinon from 'sinon'; | ||
|
|
||
| describe('test-document-submit onDocumentFormSubmit_', () => { | ||
| let sandbox; | ||
| let evt; | ||
| let tgt; | ||
| let preventDefaultSpy; | ||
|
|
||
| beforeEach(() => { | ||
| sandbox = sinon.sandbox.create(); | ||
| preventDefaultSpy = sandbox.spy(); | ||
| tgt = document.createElement('form'); | ||
| tgt.action = 'https://www.google.com'; | ||
| tgt.checkValidity = sandbox.stub(); | ||
| evt = { | ||
| target: tgt, | ||
| preventDefault: preventDefaultSpy, | ||
| defaultPrevented: false, | ||
| }; | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| sandbox.restore(); | ||
| }); | ||
|
|
||
|
|
||
| it('should do nothing if already prevented', () => { | ||
| evt.defaultPrevented = true; | ||
| onDocumentFormSubmit_(evt); | ||
| expect(preventDefaultSpy.callCount).to.equal(0); | ||
| expect(tgt.checkValidity.callCount).to.equal(0); | ||
| expect(tgt.classList.length).to.equal(0); | ||
| }); | ||
|
|
||
| it('should do nothing of no target', () => { | ||
| evt.target = null; | ||
| onDocumentFormSubmit_(evt); | ||
| expect(preventDefaultSpy.callCount).to.equal(0); | ||
| expect(tgt.checkValidity.callCount).to.equal(0); | ||
| expect(tgt.classList.length).to.equal(0); | ||
| }); | ||
|
|
||
| it('should prevent submit and add invalid class', () => { | ||
| tgt.checkValidity = sandbox.stub().returns(false); | ||
| onDocumentFormSubmit_(evt); | ||
| expect(preventDefaultSpy.callCount).to.equal(1); | ||
| expect(tgt.checkValidity.callCount).to.equal(1); | ||
| expect(tgt.classList.length).to.equal(1); | ||
| expect(tgt.classList[0]).to.equal('amp-form-invalid'); | ||
| sandbox.restore(); | ||
| preventDefaultSpy.reset(); | ||
| tgt.checkValidity.reset(); | ||
|
|
||
| tgt.checkValidity = sandbox.stub().returns(false); | ||
| tgt.classList.add('amp-form-valid'); | ||
| onDocumentFormSubmit_(evt); | ||
| expect(preventDefaultSpy.callCount).to.equal(1); | ||
| expect(tgt.checkValidity.callCount).to.equal(1); | ||
| expect(tgt.classList.length).to.equal(1); | ||
| expect(tgt.classList[0]).to.equal('amp-form-invalid'); | ||
| }); | ||
|
|
||
| it('should not prevent default and add valid class', () => { | ||
| tgt.checkValidity = sandbox.stub().returns(true); | ||
| tgt.classList.add('amp-form-invalid'); | ||
| onDocumentFormSubmit_(evt); | ||
| expect(preventDefaultSpy.callCount).to.equal(0); | ||
| expect(tgt.checkValidity.callCount).to.equal(1); | ||
| expect(tgt.classList.length).to.equal(1); | ||
| expect(tgt.classList[0]).to.equal('amp-form-valid'); | ||
| }); | ||
| }); | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Tries to submit? Or tries to edit?
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.
Currently submit, left the tries to edit stuff to next PRs specially that those will involve handling events on individual inputs as well.
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.
SG