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.

Sign up for GitHub

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

Jump to bottom

Expose submit event with on=submit:el.action syntax. #3739

Merged
merged 5 commits into from
Jun 30, 2016
Merged

Expose submit event with on=submit:el.action syntax. #3739

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
13f7f0e
Expose submit event with on=submit:el.action syntax.
mkhatib Jun 23, 2016
88bb0b8
Fix comments and add submit-success and submit-error events
mkhatib Jun 25, 2016
8349349
update example with multiple listeners
mkhatib Jun 27, 2016
4d2f167
fix merge
mkhatib Jun 28, 2016
d5a6cef
Add tests.
mkhatib Jun 28, 2016
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
38 changes: 37 additions & 1 deletion examples/forms.amp.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
<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>
<script async custom-element="amp-form" src="https://cdn.ampproject.org/v0/amp-form-0.1.js"></script>
<script async custom-element="amp-lightbox" src="https://cdn.ampproject.org/v0/amp-lightbox-0.1.js"></script>
<script async custom-template="amp-mustache" src="https://cdn.ampproject.org/v0/amp-mustache-0.1.js"></script>
<style amp-custom>
input:invalid {
Expand Down Expand Up @@ -40,6 +41,19 @@
form.amp-form-submit-error [submit-error] {
color: #dc4e41;
}

.lightbox {
background: rgba(0,0,0,.8);
}

.lightbox-content {
display: flex;
flex-direction: column;
flex-wrap: nowrap;
justify-content: center;
align-items: center;
color: white;
}
</style>
</head>
<body>
Expand Down Expand Up @@ -72,7 +86,8 @@ <h2>Subscribe to our weekly Newsletter (xhr)</h2>
<form method="post"
action="/form/html/post"
action-xhr="/form/echo-json/post"
target="_blank">
target="_blank"
on="submit-success:success-lightbox;submit-error:error-lightbox">
<fieldset>
<label>
<span>Your name</span>
Expand Down Expand Up @@ -106,5 +121,26 @@ <h2>Subscribe to our weekly Newsletter (xhr)</h2>
</template>
</div>
</form>

<amp-lightbox id="success-lightbox" class="lightbox" layout="nodisplay">
<div class="lightbox-content">
<h1>Thanks for the submission! Here's a cupcake!</h1>
<amp-img id="img0" src="https://lh3.googleusercontent.com/5rcQ32ml8E5ONp9f9-Rf78IofLb9QjS5_0mqsY1zEFc=w400-h600-no-n" width=400 height=600 layout="fixed" on="tap:success-lightbox.close" role="img" tabindex="1"></amp-img>
</div>
</amp-lightbox>

<amp-lightbox id="error-lightbox" class="lightbox" layout="nodisplay">
<div class="lightbox-content">
<h1>Oops! Kittens!</h1>
<amp-anim
src="https://lh3.googleusercontent.com/qNn8GDz8Jfd-s9lt3Nc4lJeLjVyEaqGJTk1vuCUWazCmAeOBVjSWDD0SMTU7x0zhVe5UzOTKR0n-kN4SXx7yElvpKYvCMaRyS_g-jydhJ_cEVYmYPiZ_j1Y9de43mlKxU6s06uK1NAlpbSkL_046amEKOdgIACICkuWfOBwlw2hUDfjPOWskeyMrcTu8XOEerCLuVqXugG31QC345hz3lUyOlkdT9fMYVUynSERGNzHba7bXMOxKRe3izS5DIWUgJs3oeKYqA-V8iqgCvneD1jj0Ff68V_ajm4BDchQubBJU0ytXVkoWh27ngeEHubpnApOS6fcGsjPxeuMjnzAUtoTsiXz2FZi1mMrxrblJ-kZoAq1DJ95cnoqoa2CYq3BTgq2E8BRe2paNxLJ5GXBCTpNdXMpVJc6eD7ceijQyn-2qanilX-iK3ChbOq0uBHMvsdoC_LsFOu5KzbbNH71vM3DPkvDGmHJmF67Vj8sQ6uBrLnzpYlCyN4-Y9frR8zugDcqX5Q=w400-h300-no"
width="400" height="300" on="tap:error-lightbox.close">
<amp-img placeholder
src="https://lh3.googleusercontent.com/qNn8GDz8Jfd-s9lt3Nc4lJeLjVyEaqGJTk1vuCUWazCmAeOBVjSWDD0SMTU7x0zhVe5UzOTKR0n-kN4SXx7yElvpKYvCMaRyS_g-jydhJ_cEVYmYPiZ_j1Y9de43mlKxU6s06uK1NAlpbSkL_046amEKOdgIACICkuWfOBwlw2hUDfjPOWskeyMrcTu8XOEerCLuVqXugG31QC345hz3lUyOlkdT9fMYVUynSERGNzHba7bXMOxKRe3izS5DIWUgJs3oeKYqA-V8iqgCvneD1jj0Ff68V_ajm4BDchQubBJU0ytXVkoWh27ngeEHubpnApOS6fcGsjPxeuMjnzAUtoTsiXz2FZi1mMrxrblJ-kZoAq1DJ95cnoqoa2CYq3BTgq2E8BRe2paNxLJ5GXBCTpNdXMpVJc6eD7ceijQyn-2qanilX-iK3ChbOq0uBHMvsdoC_LsFOu5KzbbNH71vM3DPkvDGmHJmF67Vj8sQ6uBrLnzpYlCyN4-Y9frR8zugDcqX5Q=w400-h300-no-k"
width="400" height="300"></amp-img>
</amp-anim>
</div>
</amp-lightbox>

</body>
</html>
27 changes: 21 additions & 6 deletions extensions/amp-form/0.1/amp-form.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import {installStyles} from '../../../src/styles';
import {CSS} from '../../../build/amp-form-0.1.css';
import {ValidationBubble} from './validation-bubble';
import {vsyncFor} from '../../../src/vsync';
import {actionServiceForDoc} from '../../../src/action';

/** @type {string} */
const TAG = 'amp-form';
Expand All @@ -52,7 +53,7 @@ export class AmpForm {
/** @const @private {!Window} */
this.win_ = element.ownerDocument.defaultView;

/** @const @private {!Element} */
/** @const @private {!HTMLFormElement} */
this.form_ = element;

/** @const @private {!../../../src/service/vsync-impl.Vsync} */
Expand All @@ -64,6 +65,9 @@ export class AmpForm {
/** @const @private {!Xhr} */
this.xhr_ = xhrFor(this.win_);

/** @const @private {!../../../src/action-impl.Action} */
this.actions_ = actionServiceForDoc(this.win_.document.documentElement);

/** @const @private {string} */
this.method_ = this.form_.getAttribute('method') || 'GET';

Expand Down Expand Up @@ -94,23 +98,32 @@ export class AmpForm {

/** @private */
installSubmitHandler_() {
this.form_.addEventListener('submit', e => this.handleSubmit_(e));
this.form_.addEventListener('submit', e => this.handleSubmit_(e), true);
}

/**
* Note on stopImmediatePropagation usage here, it is important to emulate native
* browser submit event blocking. Otherwise any other submit listeners would get the
* event.
*
* For example, action service shouldn't trigger 'submit' event if form is actually
* invalid. stopImmediatePropagation allows us to make sure we don't trigger it
Copy link
Member

Choose a reason for hiding this comment

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

Well, it depends on registration order. With this registered in capture phase, and the action registered in bubble phase, it doesn't though.

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 if I understand but even with capture, how would you stop the event from propagating to the listener in actions-impl?

It seems for tap event we're using e.defaultPrevented to prevent triggering in actions. But we can't use this with submit event because we always preventDefault when we do xhr submission, so we can't use that signal to prevent action from triggering.

Copy link
Member

Choose a reason for hiding this comment

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

For the XHR it might actually make sense to set a property on the event.

However, what I meant is that when stopImmediatePropagation is important over stopPropagation you need to be super careful about order. But the way you currently set up handlers registration order does not make a difference.

*
*
* @param {!Event} e
* @private
*/
handleSubmit_(e) {
if (this.state_ == FormState_.SUBMITTING) {
e.preventDefault();
e.stopImmediatePropagation();
return;
}

const shouldValidate = !this.form_.hasAttribute('novalidate');
if (shouldValidate &&
this.form_.checkValidity && !this.form_.checkValidity()) {
e.preventDefault();
const isInvalid = shouldValidate &&
this.form_.checkValidity && !this.form_.checkValidity();
if (isInvalid) {
e.stopImmediatePropagation();
// TODO(#3776): Use .mutate method when it supports passing state.
this.vsync_.run({
measure: undefined,
Expand All @@ -129,9 +142,11 @@ export class AmpForm {
credentials: 'include',
requireAmpResponseSourceOrigin: true,
}).then(response => {
this.actions_.trigger(this.form_, 'submit-success', null);
this.setState_(FormState_.SUBMIT_SUCCESS);
this.renderTemplate_(response || {});
}).catch(error => {
this.actions_.trigger(this.form_, 'submit-error', null);
this.setState_(FormState_.SUBMIT_ERROR);
this.renderTemplate_(error.responseJson || {});
rethrowAsync('Form submission failed:', error);
Expand Down
37 changes: 33 additions & 4 deletions extensions/amp-form/0.1/test/test-amp-form.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,19 @@ import {timer} from '../../../../src/timer';
import '../../../amp-mustache/0.1/amp-mustache';
import {installTemplatesService} from '../../../../src/service/template-impl';
import {toggleExperiment} from '../../../../src/experiments';
import {installDocService,} from
'../../../../src/service/ampdoc-impl';
import {installActionServiceForDoc,} from
'../../../../src/service/action-impl';

describe('amp-form', () => {

let sandbox;
installTemplatesService(window);

function getAmpForm(button1 = true, button2 = false) {
return createIframePromise().then(iframe => {
const docService = installDocService(iframe.win, /* isSingleDoc */ true);
installActionServiceForDoc(docService.getAmpDoc());
toggleExperiment(iframe.win, 'amp-form', true);
installTemplatesService(iframe.win);
installAmpForm(iframe.win);
Expand Down Expand Up @@ -62,7 +67,12 @@ describe('amp-form', () => {

return form;
}

beforeEach(() => {
installTemplatesService(window);
const docService = installDocService(window, /* isSingleDoc */ true);
installActionServiceForDoc(docService.getAmpDoc());

sandbox = sinon.sandbox.create();
});

Expand Down Expand Up @@ -104,13 +114,14 @@ describe('amp-form', () => {
const ampForm = new AmpForm(form);
ampForm.state_ = 'submitting';
const event = {
stopImmediatePropagation: sandbox.spy(),
target: form,
preventDefault: sandbox.spy(),
};
sandbox.spy(ampForm.xhr_, 'fetchJson');
sandbox.spy(form, 'checkValidity');
ampForm.handleSubmit_(event);
expect(event.preventDefault.called).to.be.true;
expect(event.stopImmediatePropagation.called).to.be.true;
expect(form.checkValidity.called).to.be.false;
expect(ampForm.xhr_.fetchJson.called).to.be.false;
});
Expand All @@ -120,6 +131,7 @@ describe('amp-form', () => {
form.setAttribute('novalidate', '');
const ampForm = new AmpForm(form);
const event = {
stopImmediatePropagation: sandbox.spy(),
target: form,
preventDefault: sandbox.spy(),
};
Expand All @@ -142,6 +154,7 @@ describe('amp-form', () => {
sandbox.spy(ampForm.xhr_, 'fetchJson');

const event = {
stopImmediatePropagation: sandbox.spy(),
target: ampForm.form_,
preventDefault: sandbox.spy(),
};
Expand All @@ -163,7 +176,7 @@ describe('amp-form', () => {
sandbox.spy(validationBubble, 'show');
sandbox.spy(validationBubble, 'hide');
ampForm.handleSubmit_(event);
expect(event.preventDefault.called).to.be.true;
expect(event.stopImmediatePropagation.called).to.be.true;
expect(form.checkValidity.called).to.be.true;
expect(ampForm.xhr_.fetchJson.called).to.be.false;

Expand Down Expand Up @@ -220,6 +233,7 @@ describe('amp-form', () => {
return getAmpForm().then(ampForm => {
sandbox.stub(ampForm.xhr_, 'fetchJson').returns(Promise.resolve());
const event = {
stopImmediatePropagation: sandbox.spy(),
target: ampForm.form_,
preventDefault: sandbox.spy(),
};
Expand All @@ -245,6 +259,7 @@ describe('amp-form', () => {
}));
const form = ampForm.form_;
const event = {
stopImmediatePropagation: sandbox.spy(),
target: form,
preventDefault: sandbox.spy(),
};
Expand All @@ -260,7 +275,8 @@ describe('amp-form', () => {
ampForm.handleSubmit_(event);
ampForm.handleSubmit_(event);
expect(event.preventDefault.called).to.be.true;
expect(event.preventDefault.callCount).to.equal(3);
expect(event.preventDefault.callCount).to.equal(1);
expect(event.stopImmediatePropagation.callCount).to.equal(2);
expect(ampForm.xhr_.fetchJson.calledOnce).to.be.true;
expect(form.className).to.contain('amp-form-submitting');
expect(form.className).to.not.contain('amp-form-submit-error');
Expand All @@ -283,8 +299,10 @@ describe('amp-form', () => {
sandbox.stub(ampForm.xhr_, 'fetchJson').returns(new Promise(resolve => {
fetchJsonResolver = resolve;
}));
sandbox.spy(ampForm.actions_, 'trigger');
const form = ampForm.form_;
const event = {
stopImmediatePropagation: sandbox.spy(),
target: form,
preventDefault: sandbox.spy(),
};
Expand All @@ -300,6 +318,9 @@ describe('amp-form', () => {
expect(form.className).to.not.contain('amp-form-submitting');
expect(form.className).to.not.contain('amp-form-submit-error');
expect(form.className).to.contain('amp-form-submit-success');
expect(ampForm.actions_.trigger.called).to.be.true;
expect(ampForm.actions_.trigger.calledWith(
form, 'submit-success', null)).to.be.true;
});
});
});
Expand All @@ -311,8 +332,10 @@ describe('amp-form', () => {
.returns(new Promise((unusedResolve, reject) => {
fetchJsonRejecter = reject;
}));
sandbox.spy(ampForm.actions_, 'trigger');
const form = ampForm.form_;
const event = {
stopImmediatePropagation: sandbox.spy(),
target: form,
preventDefault: sandbox.spy(),
};
Expand All @@ -336,10 +359,14 @@ describe('amp-form', () => {
expect(form.className).to.not.contain('amp-form-submitting');
expect(form.className).to.not.contain('amp-form-submit-success');
expect(form.className).to.contain('amp-form-submit-error');
expect(ampForm.actions_.trigger.called).to.be.true;
expect(ampForm.actions_.trigger.calledWith(
form, 'submit-error', null)).to.be.true;
});
});
});


it('should allow rendering responses through templates', () => {
return getAmpForm(true).then(ampForm => {
const form = ampForm.form_;
Expand All @@ -364,6 +391,7 @@ describe('amp-form', () => {
resolve(renderedTemplate);
}));
const event = {
stopImmediatePropagation: sandbox.spy(),
target: form,
preventDefault: sandbox.spy(),
};
Expand Down Expand Up @@ -411,6 +439,7 @@ describe('amp-form', () => {
resolve(newRender);
}));
const event = {
stopImmediatePropagation: sandbox.spy(),
target: form,
preventDefault: sandbox.spy(),
};
Expand Down
5 changes: 5 additions & 0 deletions src/service/action-impl.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ export class ActionService {

// Add core events.
this.addEvent('tap');
this.addEvent('submit');
}

/**
Expand All @@ -117,6 +118,10 @@ export class ActionService {
this.trigger(event.target, 'tap', event);
}
});
} else if (name == 'submit') {
this.ampdoc.getRootNode().addEventListener('submit', event => {
this.trigger(event.target, 'submit', event);
});
}
}

Expand Down