-
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
Allow basic usage of form and polyfill validate check globally #3373
Changes from 7 commits
fee39ae
143fc7d
621a2f5
33f0f68
fa320f2
ddf1ffe
9117519
8f1c297
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 |
---|---|---|
@@ -0,0 +1,56 @@ | ||
<!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> | ||
input:invalid { | ||
background-color: #dc4e41; | ||
} | ||
|
||
form .valid-message, | ||
form .invalid-message | ||
{ | ||
display: none; | ||
} | ||
|
||
form:invalid .invalid-message { | ||
display: block; | ||
color: #dc4e41; | ||
} | ||
|
||
form:valid .valid-message { | ||
display: block; | ||
color: #4CAF50; | ||
} | ||
</style> | ||
</head> | ||
<body> | ||
<h2>Subscribe to our weekly Newsletter</h2> | ||
|
||
<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="valid-message"> | ||
Everything looks good, you can submit now! | ||
</div> | ||
</form> | ||
</body> | ||
</html> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,68 @@ | ||
/** | ||
* 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 {startsWith} from '../src/string'; | ||
import {user} from '../src/log'; | ||
import {assertHttpsUrl} from '../src/url'; | ||
|
||
|
||
/** | ||
* @param {!Window} window | ||
*/ | ||
export function installGlobalSubmitListener(window) { | ||
window.document.documentElement.addEventListener( | ||
'submit', onDocumentFormSubmit_, true); | ||
} | ||
|
||
|
||
/** | ||
* 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is target always FORM here? Worth checking 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 should always be FORM I believe, but added a check just in case. 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. |
||
if (!form || form.tagName != 'FORM') { | ||
return; | ||
} | ||
|
||
const action = form.getAttribute('action'); | ||
user.assert(action, 'form action attribute is required: %s', form); | ||
assertHttpsUrl(action, form, 'action'); | ||
user.assert(!startsWith(action, 'https://cdn.ampproject.org'), | ||
'form action should not be on cdn.ampproject.org: %s', form); | ||
|
||
const target = form.getAttribute('target'); | ||
user.assert(target, 'form target attribute is required: %s', form); | ||
user.assert(target == '_blank' || target == '_top', | ||
'form target=%s is invalid can only be _blank or _top: %s', target, form); | ||
|
||
// Safari does not trigger validation check on submission, hence we | ||
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. Let's also test 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. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Even with validator, we still do these checks in runtime to point out problems sooner.
Target must be either 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. |
||
// 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()) { | ||
e.preventDefault(); | ||
return; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -39,6 +39,19 @@ export function endsWith(string, suffix) { | |
return string.indexOf(suffix, index) == index; | ||
} | ||
|
||
/** | ||
* Polyfill for String.prototype. startsWith. | ||
* @param {string} string | ||
* @param {string} prefix | ||
* @return {boolean} | ||
*/ | ||
export function startsWith(string, prefix) { | ||
if (prefix.length > string.length) { | ||
return false; | ||
} | ||
return string.indexOf(prefix) == 0; | ||
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 can optimize this using a rather nifty trick: return string.lastIndexOf(prefix, 0) === 0; This prevents a full scan of the string, only checking index 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. Woah, that's pretty cool 👍 . Done. |
||
} | ||
|
||
/** | ||
* Expands placeholders in a given template string with values. | ||
* | ||
|
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 {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.target = '_blank'; | ||
tgt.checkValidity = sandbox.stub(); | ||
evt = { | ||
target: tgt, | ||
preventDefault: preventDefaultSpy, | ||
defaultPrevented: false, | ||
}; | ||
}); | ||
|
||
afterEach(() => { | ||
sandbox.restore(); | ||
}); | ||
|
||
|
||
it('should check target and action attributes', () => { | ||
tgt.removeAttribute('action'); | ||
expect(() => onDocumentFormSubmit_(evt)).to.throw( | ||
/form action attribute is required/); | ||
|
||
tgt.setAttribute('action', 'http://example.com'); | ||
expect(() => onDocumentFormSubmit_(evt)).to.throw( | ||
/form action must start with "https:/); | ||
|
||
tgt.setAttribute('action', 'https://cdn.ampproject.org'); | ||
expect(() => onDocumentFormSubmit_(evt)).to.throw( | ||
/form action should not be on cdn\.ampproject\.org/); | ||
|
||
tgt.setAttribute('action', 'https://valid.example.com'); | ||
tgt.removeAttribute('target'); | ||
expect(() => onDocumentFormSubmit_(evt)).to.throw( | ||
/form target attribute is required/); | ||
|
||
tgt.setAttribute('target', '_self'); | ||
expect(() => onDocumentFormSubmit_(evt)).to.throw( | ||
/form target=_self is invalid/); | ||
|
||
tgt.setAttribute('target', '_blank'); | ||
expect(() => onDocumentFormSubmit_(evt)).to.not.throw; | ||
}); | ||
|
||
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); | ||
}); | ||
|
||
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); | ||
}); | ||
|
||
it('should prevent submit', () => { | ||
tgt.checkValidity = sandbox.stub().returns(false); | ||
onDocumentFormSubmit_(evt); | ||
expect(preventDefaultSpy.callCount).to.equal(1); | ||
expect(tgt.checkValidity.callCount).to.equal(1); | ||
sandbox.restore(); | ||
preventDefaultSpy.reset(); | ||
tgt.checkValidity.reset(); | ||
|
||
tgt.checkValidity = sandbox.stub().returns(false); | ||
onDocumentFormSubmit_(evt); | ||
expect(preventDefaultSpy.callCount).to.equal(1); | ||
expect(tgt.checkValidity.callCount).to.equal(1); | ||
}); | ||
|
||
it('should not prevent default', () => { | ||
tgt.checkValidity = sandbox.stub().returns(true); | ||
onDocumentFormSubmit_(evt); | ||
expect(preventDefaultSpy.callCount).to.equal(0); | ||
expect(tgt.checkValidity.callCount).to.equal(1); | ||
}); | ||
}); |
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.
I think we should move this to
src/services
to ensure forbidden deps help avoid bad inclusions.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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not entirely sure what should be added to
dep-check-config
, what is a bad inclusion in this case?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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack. Do I need to add anything to
dep-check-config
?