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

Update amp-form docs to match validators and runtime updates. #5671

Merged
merged 3 commits into from Oct 19, 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
9 changes: 6 additions & 3 deletions extensions/amp-form/0.1/amp-form.js
Expand Up @@ -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,
Copy link
Member

Choose a reason for hiding this comment

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

Please add a test that throws this exception

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

'Only XHR based (via action-xhr attribute) submissions are support ' +
'for POST requests. %s',
this.form_);
}
}

Expand Down
19 changes: 17 additions & 2 deletions extensions/amp-form/0.1/test/test-amp-form.js
Expand Up @@ -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();
Expand Down Expand Up @@ -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;
});

Expand Down
10 changes: 7 additions & 3 deletions extensions/amp-form/amp-form.md
Expand Up @@ -71,18 +71,22 @@ __required__
Target attribute of the `<form>` 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.
Copy link
Member

Choose a reason for hiding this comment

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

We should at least once explain what an XHR request is

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


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. 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.
Copy link
Member

Choose a reason for hiding this comment

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

No , after request. End first sentence in 'also sometimes called Ajax request.'

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


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.

Expand Down
36 changes: 21 additions & 15 deletions src/document-submit.js
Expand Up @@ -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.
Expand All @@ -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;
Expand Down