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

Add form.requestSubmit() #4597

Merged
merged 6 commits into from
May 16, 2019
Merged

Add form.requestSubmit() #4597

merged 6 commits into from
May 16, 2019

Conversation

domenic
Copy link
Member

@domenic domenic commented May 6, 2019

Closes #4187.

(See WHATWG Working Mode: Changes for more details.)


/cc @manucorporat and @muan as web developers who were interested in this and helped shape the feature.

Things for reviewers to consider:

  • Is the domintro text for web developers helpful enough?
  • Are the "is a submit button" and form owner checks the correct ones? We could loosen these in the future, e.g. per @muan's feedback in Need callback for form submit data WICG/webcomponents#187 (comment) we could allow certain form-associated custom elements to be submit buttons.
  • TypeError vs. other error types for the checks failing. I don't care very much so I went generic.

/acknowledgements.html ( diff )
/forms.html ( diff )

@domenic domenic requested a review from tkent-google May 6, 2019 17:34
source Outdated
button</span>, then throw a <code>TypeError</code>.</p></li>

<li><p>If <var>submitter</var>'s <span>form owner</span> is not this <code>form</code> element,
then throw a <code>TypeError</code>.</p></li>
Copy link
Contributor

Choose a reason for hiding this comment

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

"NotFoundError" might be suitable? It's used for TextTrack-TextTrackCue ownership check.

Copy link
Member

Choose a reason for hiding this comment

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

TypeError is nicer if we ever want to abstract this into IDL.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think we could feasibly put this into the IDL type system... I'm reasonably convinced by the symmetry with the existing ownership check. I'll switch.

@tkent-google
Copy link
Contributor

… TODO. @tkent-google maybe could help write these as part of the Blink implementation?

Yes, I'll make Blink implementation and WPT.

tkent-google added a commit to web-platform-tests/wpt that referenced this pull request May 9, 2019
@tkent-google
Copy link
Contributor

Here is a test PR: web-platform-tests/wpt#16743

source Outdated
<li><p>Otherwise, set <var>submitter</var> to this <code>form</code> element.</p></li>

<li><p><span data-x="concept-form-submit">Submit</span> this <code>form</code> element, from
<var>submitter</var>.</p></li>
Copy link
Member

Choose a reason for hiding this comment

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

I guess we somehow know that reentrancy is not a problem here? Does it rely on the protections added for formdata?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm. I guess this does open up the possibility of

form.onsubmit = () => { form.requestSubmit(); }
form.requestSubmit();

We could add a flag to protect against that. I guess we should do so, for symmetry with reset(). Good catch.

Ping @tkent-google to check my logic, and also update the tests.

Copy link
Contributor

@tkent-google tkent-google May 10, 2019

Choose a reason for hiding this comment

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

We should have the reentrancy check in form submission algorithm, not in requestSubmit()

click() of a submit button already has a reentrancy issue.

form.onsubmit = () => {
  form.querySelector('[type=submit]').click();
}

When a submit button owned by form is clicked, submit event is dispatched, and click() invokes another form submission.

Chrome, Safari, and Firefox already have a reentrancy check for this pattern, and the form submission invoked by click() is aborted.
On the other hand, form.submit() in submit event handler should be allowed for compatibility.

WebKit/Blink implementation is something like:

  • <form> has in-submit-event flag, initially false.
  • It should be set to true before firing submit event, and reset to false after it.
  • Add "If the submitted from() method flag is not set and in-submit-event flag is true, then return." before step 6 of form submission algorithm.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for catching that deeper problem. Do you think this flag might be mergeable with the "constructing entry list" flag?

Either way, we should fix that missing check in a separate PR, and rebase this PR on top of it. I am heading to sleep now, but maybe you could work on a spec and tests PR for it? Or I can do it tomorrow.

Copy link
Contributor

Choose a reason for hiding this comment

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

Filed #4620, and
made a specification PR; #4621

This reverts commit 8c4fee1.

No longer necessary now that it's in the main submit algorithm.
@domenic
Copy link
Member Author

domenic commented May 15, 2019

This is ready for another look, rebased on top of #4621.

@domenic domenic merged commit e0742f8 into master May 16, 2019
@domenic domenic deleted the requestsubmit branch May 16, 2019 19:07
domenic pushed a commit to web-platform-tests/wpt that referenced this pull request May 16, 2019
@domenic
Copy link
Member Author

domenic commented May 16, 2019

Heads-up @whatwg/documentation

@domenic domenic added addition/proposal New features or enhancements impacts documentation Used by documentation communities, such as MDN, to track changes that impact documentation topic: forms labels May 16, 2019
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Jun 19, 2019
…), a=testonly

Automatic update from web-platform-tests
html: Add a test for form.requestSubmit()

Specification issue: whatwg/html#4187
Specification PR: whatwg/html#4597
--

wp5At-commits: 1de2d96f75cdc7a920b74a8f406262f16531ef0f
wpt-pr: 16743
xeonchen pushed a commit to xeonchen/gecko that referenced this pull request Jun 19, 2019
…), a=testonly

Automatic update from web-platform-tests
html: Add a test for form.requestSubmit()

Specification issue: whatwg/html#4187
Specification PR: whatwg/html#4597
--

wp5At-commits: 1de2d96f75cdc7a920b74a8f406262f16531ef0f
wpt-pr: 16743
marcoscaceres pushed a commit to web-platform-tests/wpt that referenced this pull request Jul 23, 2019
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Oct 4, 2019
…), a=testonly

Automatic update from web-platform-tests
html: Add a test for form.requestSubmit()

Specification issue: whatwg/html#4187
Specification PR: whatwg/html#4597
--

wp5At-commits: 1de2d96f75cdc7a920b74a8f406262f16531ef0f
wpt-pr: 16743

UltraBlame original commit: d9fee4b7ddd070aaf762769226681d22b8473f86
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Oct 4, 2019
…), a=testonly

Automatic update from web-platform-tests
html: Add a test for form.requestSubmit()

Specification issue: whatwg/html#4187
Specification PR: whatwg/html#4597
--

wp5At-commits: 1de2d96f75cdc7a920b74a8f406262f16531ef0f
wpt-pr: 16743

UltraBlame original commit: d9fee4b7ddd070aaf762769226681d22b8473f86
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Oct 4, 2019
…), a=testonly

Automatic update from web-platform-tests
html: Add a test for form.requestSubmit()

Specification issue: whatwg/html#4187
Specification PR: whatwg/html#4597
--

wp5At-commits: 1de2d96f75cdc7a920b74a8f406262f16531ef0f
wpt-pr: 16743

UltraBlame original commit: d9fee4b7ddd070aaf762769226681d22b8473f86
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition/proposal New features or enhancements impacts documentation Used by documentation communities, such as MDN, to track changes that impact documentation topic: forms
Development

Successfully merging this pull request may close these issues.

Proposal: allow a mode of form.submit() that behaves like clicking a submit button
3 participants