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

Block any further submissions until the current one finishes. #3582

Merged
merged 3 commits into from Jun 15, 2016
Merged

Block any further submissions until the current one finishes. #3582

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c635d18
Block any further submissions until the current one finishes.
mkhatib Jun 15, 2016
7c92c72
prevent default
mkhatib Jun 15, 2016
c2b9c82
address comments.
mkhatib Jun 15, 2016
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
29 changes: 26 additions & 3 deletions extensions/amp-form/0.1/amp-form.js
View file
Open in desktop
Expand Up @@ -64,6 +64,17 @@ export class AmpForm {
'form action-xhr should not be on cdn.ampproject.org: %s',
this.form_);
}

const submitButtons = this.form_.querySelectorAll('input[type=submit]');
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be more robust and efficient to re-query these when you need them

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 sure where the efficiency and robustness win would come from?

Also, this is the same as earlier, we won't have an early error thrown when there are no submit buttons.

user.assert(submitButtons && submitButtons.length > 0,
'form requires at least one <input type=submit>: %s', this.form_);

/** @const @private {!Array<!Element>} */
this.submitButtons_ = toArray(submitButtons);

/** @private {?string} */
this.state_ = null;

this.installSubmitHandler_();
}

Expand All @@ -80,6 +91,12 @@ export class AmpForm {
if (e.defaultPrevented) {
return;
}

if (this.state_ == FormState_.SUBMITTING) {
e.preventDefault();
Copy link
Member

Choose a reason for hiding this comment

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

Could you capture this in a test 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.

I pointed out where this is being captured in test.

return;
}

if (this.xhrAction_) {
e.preventDefault();
this.setState_(FormState_.SUBMITTING);
Expand All @@ -104,10 +121,16 @@ export class AmpForm {
* @private
*/
setState_(state) {
for (const key in FormState_) {
this.form_.classList.remove(`amp-form-${FormState_[key]}`);
}
this.form_.classList.remove(`amp-form-${this.state_}`);
this.form_.classList.add(`amp-form-${state}`);
this.state_ = state;
this.submitButtons_.forEach(button => {
if (state == FormState_.SUBMITTING) {
button.setAttribute('disabled', '');
} else {
button.removeAttribute('disabled');
}
});
}
}

Expand Down
122 changes: 102 additions & 20 deletions extensions/amp-form/0.1/test/test-amp-form.js
View file
Open in desktop
Expand Up @@ -22,6 +22,38 @@ import {timer} from '../../../../src/timer';
describe('amp-form', () => {

let sandbox;

function getAmpForm(button1 = true, button2 = false) {
return createIframePromise().then(iframe => {
const form = getForm(iframe.doc, button1, button2);
const ampForm = new AmpForm(form);
return ampForm;
});
}

function getForm(doc = document, button1 = true, button2 = false) {
const form = doc.createElement('form');

const nameInput = doc.createElement('input');
nameInput.setAttribute('name', 'name');
nameInput.setAttribute('value', 'John Miller');
form.appendChild(nameInput);
form.setAttribute('action-xhr', 'https://example.com');

if (button1) {
const submitBtn = doc.createElement('input');
submitBtn.setAttribute('type', 'submit');
form.appendChild(submitBtn);
}

if (button2) {
const submitBtn = doc.createElement('input');
submitBtn.setAttribute('type', 'submit');
form.appendChild(submitBtn);
}

return form;
}
beforeEach(() => {
sandbox = sinon.sandbox.create();
});
Expand All @@ -30,8 +62,16 @@ describe('amp-form', () => {
sandbox.restore();
});

it('should assert form has at least 1 submit button', () => {
let form = getForm(document, false, false);
expect(() => new AmpForm(form)).to.throw(
/form requires at least one <input type=submit>/);
form = getForm(document, true, false);
expect(() => new AmpForm(form)).to.not.throw;
});

it('should assert valid action-xhr when provided', () => {
const form = document.createElement('form');
const form = getForm();
form.setAttribute('action-xhr', 'http://example.com');
expect(() => new AmpForm(form)).to.throw(
/form action-xhr must start with/);
Expand All @@ -43,7 +83,7 @@ describe('amp-form', () => {
});

it('should listen to submit event', () => {
const form = document.createElement('form');
const form = getForm();
form.addEventListener = sandbox.spy();
form.setAttribute('action-xhr', 'https://example.com');
new AmpForm(form);
Expand All @@ -52,7 +92,7 @@ describe('amp-form', () => {
});

it('should do nothing if defaultPrevented', () => {
const form = document.createElement('form');
const form = getForm();
form.setAttribute('action-xhr', 'https://example.com');
const ampForm = new AmpForm(form);
const event = {
Expand All @@ -65,17 +105,10 @@ describe('amp-form', () => {
});

it('should call fetchJson with the xhr action and form data', () => {
return createIframePromise().then(iframe => {
const form = iframe.doc.createElement('form');
const nameInput = iframe.doc.createElement('input');
nameInput.setAttribute('name', 'name');
nameInput.setAttribute('value', 'John Miller');
form.appendChild(nameInput);
form.setAttribute('action-xhr', 'https://example.com');
const ampForm = new AmpForm(form);
return getAmpForm().then(ampForm => {
sandbox.stub(ampForm.xhr_, 'fetchJson').returns(Promise.resolve());
const event = {
target: form,
target: ampForm.form_,
preventDefault: sandbox.spy(),
defaultPrevented: false,
};
Expand All @@ -93,27 +126,68 @@ describe('amp-form', () => {
});
});

it('should block multiple submissions and disable buttons', () => {
return getAmpForm(true, true).then(ampForm => {
let fetchJsonResolver;
sandbox.stub(ampForm.xhr_, 'fetchJson').returns(new Promise(resolve => {
fetchJsonResolver = resolve;
}));
const form = ampForm.form_;
const event = {
target: form,
preventDefault: sandbox.spy(),
defaultPrevented: false,
};
const button1 = form.querySelectorAll('input[type=submit]')[0];
const button2 = form.querySelectorAll('input[type=submit]')[1];
expect(button1.hasAttribute('disabled')).to.be.false;
expect(button2.hasAttribute('disabled')).to.be.false;
ampForm.handleSubmit_(event);
expect(ampForm.state_).to.equal('submitting');
expect(ampForm.xhr_.fetchJson.calledOnce).to.be.true;
expect(button1.hasAttribute('disabled')).to.be.true;
expect(button2.hasAttribute('disabled')).to.be.true;
ampForm.handleSubmit_(event);
ampForm.handleSubmit_(event);
expect(event.preventDefault.called).to.be.true;
expect(event.preventDefault.callCount).to.equal(3);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cramforce this captures the prevent default when submitting.

Copy link
Member

Choose a reason for hiding this comment

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

K. I was looking for assertions on defaultPrevented.

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');
expect(form.className).to.not.contain('amp-form-submit-success');
fetchJsonResolver();
return timer.promise(20).then(() => {
expect(button1.hasAttribute('disabled')).to.be.false;
expect(button2.hasAttribute('disabled')).to.be.false;
expect(ampForm.state_).to.equal('submit-success');
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');
});
});
});

it('should manage form state classes (submitting, success)', () => {
return createIframePromise().then(iframe => {
const form = iframe.doc.createElement('form');
form.setAttribute('action-xhr', 'https://example.com');
const ampForm = new AmpForm(form);
return getAmpForm().then(ampForm => {
let fetchJsonResolver;
sandbox.stub(ampForm.xhr_, 'fetchJson').returns(new Promise(resolve => {
fetchJsonResolver = resolve;
}));
const form = ampForm.form_;
const event = {
target: form,
preventDefault: sandbox.spy(),
defaultPrevented: false,
};
ampForm.handleSubmit_(event);
expect(event.preventDefault.called).to.be.true;
expect(ampForm.state_).to.equal('submitting');
expect(form.className).to.contain('amp-form-submitting');
expect(form.className).to.not.contain('amp-form-submit-error');
expect(form.className).to.not.contain('amp-form-submit-success');
fetchJsonResolver();
return timer.promise(20).then(() => {
expect(ampForm.state_).to.equal('submit-success');
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');
Expand All @@ -122,27 +196,35 @@ describe('amp-form', () => {
});

it('should manage form state classes (submitting, error)', () => {
return createIframePromise().then(iframe => {
const form = iframe.doc.createElement('form');
form.setAttribute('action-xhr', 'https://example.com');
const ampForm = new AmpForm(form);
return getAmpForm(true, true).then(ampForm => {
let fetchJsonRejecter;
sandbox.stub(ampForm.xhr_, 'fetchJson')
.returns(new Promise((unusedResolve, reject) => {
fetchJsonRejecter = reject;
}));
const form = ampForm.form_;
const event = {
target: form,
preventDefault: sandbox.spy(),
defaultPrevented: false,
};
const button1 = form.querySelectorAll('input[type=submit]')[0];
const button2 = form.querySelectorAll('input[type=submit]')[1];
expect(button1.hasAttribute('disabled')).to.be.false;
expect(button2.hasAttribute('disabled')).to.be.false;
ampForm.handleSubmit_(event);
expect(button1.hasAttribute('disabled')).to.be.true;
expect(button2.hasAttribute('disabled')).to.be.true;
expect(event.preventDefault.called).to.be.true;
expect(ampForm.state_).to.equal('submitting');
expect(form.className).to.contain('amp-form-submitting');
expect(form.className).to.not.contain('amp-form-submit-error');
expect(form.className).to.not.contain('amp-form-submit-success');
fetchJsonRejecter();
return timer.promise(20).then(() => {
expect(button1.hasAttribute('disabled')).to.be.false;
expect(button2.hasAttribute('disabled')).to.be.false;
expect(ampForm.state_).to.equal('submit-error');
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');
Expand Down