From 2e44d9960d4002772670c8ea4e27380de9fae671 Mon Sep 17 00:00:00 2001 From: Mohammad Khatib Date: Wed, 19 Oct 2016 12:33:24 -0700 Subject: [PATCH] Update amp-form docs to match validators and runtime updates. (#5671) --- extensions/amp-form/0.1/amp-form.js | 9 +++-- extensions/amp-form/0.1/test/test-amp-form.js | 19 ++++++++-- extensions/amp-form/amp-form.md | 10 ++++-- src/document-submit.js | 36 +++++++++++-------- 4 files changed, 51 insertions(+), 23 deletions(-) diff --git a/extensions/amp-form/0.1/amp-form.js b/extensions/amp-form/0.1/amp-form.js index 40c78d7ebeaf5..2bcafeb71b255 100644 --- a/extensions/amp-form/0.1/amp-form.js +++ b/extensions/amp-form/0.1/amp-form.js @@ -211,9 +211,12 @@ export class AmpForm { this.renderTemplate_(error.responseJson || {}); rethrowAsync('Form submission failed:', error); }); - } else if (this.target_ == '_top' && this.method_ == 'POST') { - this.cleanupRenderedTemplate_(); - this.setState_(FormState_.SUBMITTING); + } else if (this.method_ == 'POST') { + e.preventDefault(); + user().assert(false, + 'Only XHR based (via action-xhr attribute) submissions are support ' + + 'for POST requests. %s', + this.form_); } } diff --git a/extensions/amp-form/0.1/test/test-amp-form.js b/extensions/amp-form/0.1/test/test-amp-form.js index 49b58d4f9bec4..50bcf716df9cd 100644 --- a/extensions/amp-form/0.1/test/test-amp-form.js +++ b/extensions/amp-form/0.1/test/test-amp-form.js @@ -149,6 +149,22 @@ describe('amp-form', () => { expect(ampForm.xhr_.fetchJson.called).to.be.false; }); + it('should throw error if POST non-xhr', () => { + const form = getForm(); + form.removeAttribute('action-xhr'); + const ampForm = new AmpForm(form); + const event = { + stopImmediatePropagation: sandbox.spy(), + target: form, + preventDefault: sandbox.spy(), + }; + sandbox.spy(ampForm.xhr_, 'fetchJson'); + sandbox.spy(form, 'checkValidity'); + expect(() => ampForm.handleSubmit_(event)).to.throw( + /Only XHR based \(via action-xhr attribute\) submissions are support/); + expect(event.preventDefault).to.be.called; + }); + it('should respect novalidate on a form', () => { setReportValiditySupported(true); const form = getForm(); @@ -176,14 +192,13 @@ describe('amp-form', () => { }; sandbox.spy(form, 'checkValidity'); sandbox.spy(emailInput, 'reportValidity'); - ampForm.xhrAction_ = null; + ampForm.handleSubmit_(event); // Check validity should always be called regardless of novalidate. expect(form.checkValidity.called).to.be.true; // However reporting validity shouldn't happen when novalidate. expect(emailInput.reportValidity.called).to.be.false; - expect(event.preventDefault.called).to.be.false; expect(form.hasAttribute('amp-novalidate')).to.be.true; }); diff --git a/extensions/amp-form/amp-form.md b/extensions/amp-form/amp-form.md index 7457f6cb73556..4078c45ab37da 100644 --- a/extensions/amp-form/amp-form.md +++ b/extensions/amp-form/amp-form.md @@ -71,18 +71,22 @@ __required__ Target attribute of the `
` must be either `_blank` or `_top`. **action** -__required__ +__invalid__ when `method=POST` +__required__ when `method=GET` Action must be provided, `https` and is non-cdn link (does **NOT** link to https://cdn.ampproject.org). __Note__: `target` and `action` will only be used for non-xhr GET requests. AMP runtime will use `action-xhr` to make the request and will ignore `action` and `target`. When `action-xhr` is not provided AMP would make a GET request to `action` endpoint and use `target` to open a new window (if `_blank`). AMP runtime might also fallback to using action and target in cases where `amp-form` extension fails to load. **action-xhr** -__(optional)__ for `GET` __required__ for `POST` requests +__required__ when `method=POST` +__optional__ when `method=GET` + You can also provide an action-xhr attribute, if provided, the form will be submitted in an XHR fashion. -This attribute can be the same or a different endpoint than `action` and has the same action requirements above. +An XHR request is where the browser would make the request without a full load of the page or opening a new page also sometimes called Ajax request. Browsers will send the request in the background using [Fetch API](https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API) when available and fallback to [XMLHttpRequest API](https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest) for older browsers. +This attribute can be the same or a different endpoint than `action` and has the same action requirements above. **Important**: See [Security Considerations](#security-considerations) for notes on how to secure your forms endpoints. diff --git a/src/document-submit.js b/src/document-submit.js index 371efd5605ef8..ea66a686d5188 100644 --- a/src/document-submit.js +++ b/src/document-submit.js @@ -60,29 +60,38 @@ export function onDocumentFormSubmit_(e) { const actionXhr = form.getAttribute('action-xhr'); const method = (form.getAttribute('method') || 'GET').toUpperCase(); if (method == 'GET') { + // TODO(#5670): Make action optional for method=GET when action-xhr is provided. user().assert(action, 'form action attribute is required for method=GET: %s', form); assertHttpsUrl(action, dev().assertElement(form), 'action'); user().assert(!startsWith(action, urls.cdn), 'form action should not be on AMP CDN: %s', form); checkCorsUrl(action); - } else if (action) { - e.preventDefault(); - user().assert(false, - 'form action attribute is invalid for method=POST: %s', form); - } else if (!actionXhr) { - e.preventDefault(); - user().assert(false, - 'Only XHR based (via action-xhr attribute) submissions are support ' + - 'for POST requests. %s', - form); + } else if (method == 'POST') { + if (action) { + e.preventDefault(); + user().assert(false, + 'form action attribute is invalid for method=POST: %s', form); + } + + if (!actionXhr) { + e.preventDefault(); + user().assert(false, + 'Only XHR based (via action-xhr attribute) submissions are support ' + + 'for POST requests. %s', + form); + } } - checkCorsUrl(actionXhr); + // TODO(#5607): Only require this with method=GET. const target = form.getAttribute('target'); - user().assert(target, 'form target attribute is required: %s', form); + 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); + if (actionXhr) { + checkCorsUrl(actionXhr); + } // amp-form extension will add novalidate to all forms to manually trigger // validation. In that case `novalidate` doesn't have the same meaning. @@ -97,9 +106,6 @@ export function onDocumentFormSubmit_(e) { // Safari does not trigger validation check on submission, hence we // 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 (shouldValidate && form.checkValidity && !form.checkValidity()) { e.preventDefault(); return;