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

Allow basic usage of form and polyfill validate check globally #3373

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

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions build-system/server.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ var bodyParser = require('body-parser');
var clr = require('connect-livereload');
var finalhandler = require('finalhandler');
var fs = BBPromise.promisifyAll(require('fs'));
var formidable = require('formidable');
var jsdom = require('jsdom');
var path = require('path');
var request = require('request');
Expand Down Expand Up @@ -69,6 +70,25 @@ app.use('/api/echo/post', function(req, res) {
res.end(JSON.stringify(req.body, null, 2));
});

app.use('/form/echo-html/post', function(req, res) {
var form = new formidable.IncomingForm();
form.parse(req, function(err, fields) {
res.setHeader('Content-Type', 'text/html');
if (fields['email'] == 'already@subscribed.com') {
res.statusCode = 500;
res.end(`
<h1 style="color:red;">Sorry ${fields['name']}!</h1>
<p>The email ${fields['email']} is already subscribed!</p>
`);
} else {
res.end(`
<h1>Thanks ${fields['name']}!</h1>
<p>Please make sure to confirm your email ${fields['email']}</p>
`);
}
});
});

// Fetches an AMP document from the AMP proxy and replaces JS
// URLs, so that they point to localhost.
function proxyToAmpProxy(req, res, minify) {
Expand Down
2 changes: 2 additions & 0 deletions build-system/tasks/presubmit-checks.js
Original file line number Diff line number Diff line change
Expand Up @@ -289,6 +289,8 @@ var forbiddenTerms = {
'validator/tokenize-css.js',
'validator/validator-full.js',
'validator/validator.js',
// exports.startsWith occurs in babel generated code.
'dist.3p/current/integration.js',
]
},
'\\.endsWith': {
Expand Down
56 changes: 56 additions & 0 deletions examples/forms.amp.html
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>
1 change: 1 addition & 0 deletions gulpfile.js
Original file line number Diff line number Diff line change
Expand Up @@ -325,6 +325,7 @@ function buildExamples(watch) {
buildExample('metadata-examples/video-microdata.amp.html');
buildExample('everything.amp.html');
buildExample('font.amp.html');
buildExample('forms.amp.html');
buildExample('facebook.amp.html');
buildExample('instagram.amp.html');
buildExample('jwplayer.amp.html');
Expand Down
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
"eslint": "1.10.3",
"eslint-plugin-google-camelcase": "0.0.2",
"finalhandler": "0.4.0",
"formidable": "1.0.17",
"fs-extra": "0.27.0",
"gulp": "3.9.1",
"gulp-closure-compiler": "0.4.0",
Expand Down
5 changes: 5 additions & 0 deletions src/amp.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import {installPullToRefreshBlocker} from './pull-to-refresh';
import {templatesFor} from './template';
import {installCoreServices} from './amp-core-service';
import {installGlobalClickListener} from './document-click';
import {installGlobalSubmitListener} from './document-submit';
import {installImg} from '../builtins/amp-img';
import {installVideo} from '../builtins/amp-video';
import {installPixel} from '../builtins/amp-pixel';
Expand All @@ -30,6 +31,7 @@ import {adopt} from './runtime';
import {cssText} from '../build/css';
import {maybeValidate} from './validator-integration';
import {maybeTrackImpression} from './impression';
import {isExperimentOn} from './experiments';

// We must under all circumstances call makeBodyVisible.
// It is much better to have AMP tags not rendered than having
Expand Down Expand Up @@ -57,6 +59,9 @@ try {

installPullToRefreshBlocker(window);
installGlobalClickListener(window);
if (isExperimentOn(window, 'form-submit')) {
installGlobalSubmitListener(window);
}

maybeValidate(window);
makeBodyVisible(document, /* waitForExtensions */ true);
Expand Down
68 changes: 68 additions & 0 deletions src/document-submit.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
/**
Copy link
Contributor

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.

Copy link
Member

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 :)

Copy link
Contributor Author

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?

Copy link
Contributor

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.

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. Do I need to add anything to dep-check-config?

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

Choose a reason for hiding this comment

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

Is target always FORM here? Worth checking as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

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 (!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
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's also test action and target?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 target being either _top or _blank?

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

  1. action attribute must be specified
  2. assertHttpsUrl(action)
  3. action NOT a 'https://cdn.ampproject.org' URL.
  4. Possibly even, we could test that action != currentUrl, though this would be a bit questionable.

Target must be either blank_ or top_ when resolved from default.

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.

// 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;
}
}
13 changes: 13 additions & 0 deletions src/string.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor

@jridgewell jridgewell Jun 6, 2016

Choose a reason for hiding this comment

The 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 0 (and possibly 1, 2, up to prefix.length if they match).

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.
*
Expand Down
11 changes: 7 additions & 4 deletions src/url.js
Original file line number Diff line number Diff line change
Expand Up @@ -123,18 +123,21 @@ export function addParamsToUrl(url, params) {
*
* @param {?string|undefined} urlString
* @param {!Element|string} elementContext Element where the url was found.
* @param {string=} sourceName Used for error messages.
* @return {string}
*/
export function assertHttpsUrl(urlString, elementContext) {
user.assert(urlString != null, '%s source must be available', elementContext);
export function assertHttpsUrl(
urlString, elementContext, sourceName = 'source') {
user.assert(urlString != null, '%s %s must be available',
elementContext, sourceName);
const url = parseUrl(urlString);
user.assert(
url.protocol == 'https:' || /^(\/\/)/.test(urlString) ||
url.hostname == 'localhost' || endsWith(url.hostname, '.localhost'),
'%s source must start with ' +
'%s %s must start with ' +
'"https://" or "//" or be relative and served from ' +
'either https or from localhost. Invalid value: %s',
elementContext, urlString);
elementContext, sourceName, urlString);
return urlString;
}

Expand Down
106 changes: 106 additions & 0 deletions test/functional/test-document-submit.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 {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);
});
});
5 changes: 5 additions & 0 deletions tools/experiments/experiments.js
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,11 @@ const EXPERIMENTS = [
name: 'AMP carousel using horizontal scroll',
spec: '',
},
{
id: 'form-submit',
name: 'Global document form submit handler',
spec: 'https://github.com/ampproject/amphtml/issues/3343',
},
];


Expand Down