Bug 234069

Summary: A FormData constructed in the form's submit event listener shouldn't include the submitter
Product: WebKit Reporter: Andreu Botella <andreu>
Component: FormsAssignee: Nobody <webkit-unassigned>
Severity: Normal CC: akeerthi, andreu, cdumez, changseok, clopez, commit-queue, darin, esprehn+autocc, ews-watchlist, gyuyoung.kim, mifenton, ntim, webkit-bug-importer, wenson_hsieh, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://github.com/web-platform-tests/wpt/pull/32916
Bug Depends on: 236006, 236324    
Bug Blocks:    
Description Flags
Patch ews-feeder: commit-queue-

Description Andreu Botella 2021-12-08 23:37:48 PST
WPT test: https://wpt.fyi/results/html/semantics/forms/form-submission-0/constructing-form-data-set.html?label=experimental&label=master&aligned

In the spec, the "construct the entry list" algorithm is passed an optional "submitter" argument, which it uses to determine whether to include a button in the entry list. And the FormData constructor calls "construct the entry list" with no optional parameters. But in WebKit, a FormData object constructed in the form's submit event listener will contain the submitter in its entry list.

This happens because in WebKit, the submitter is tracked by setting an input's m_isActivatedSubmit state, and when submit buttons are activated, the state is set, the form is submitted, and the state is then unset (in HTMLButtonElement::defaultEventHandler, ImageInputType::handleDOMActivateEvent, SubmitInputType::handleDOMActivateEvent). So the submitter state stays on during the entire form submission algorithm, including when firing the submit event.

However, it seems like HTMLFormElement::submit also sets the submitter's m_isActivatedSubmit state, since otherwise no submitter would show up for implicit submission and form.requestSubmit() with no arguments. So it seems like setting the state in the activation handlers would be unnecessary.
Comment 1 Andreu Botella 2021-12-13 11:57:06 PST
Created attachment 447040 [details]
Comment 2 EWS 2021-12-13 15:32:30 PST
Committed r286988 (?): <https://commits.webkit.org/r286988>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 447040 [details].
Comment 3 Radar WebKit Bug Importer 2021-12-13 15:33:21 PST
Comment 4 WebKit Commit Bot 2022-02-01 23:29:24 PST
Re-opened since this is blocked by bug 236006
Comment 5 Aditya Keerthi 2022-02-01 23:36:59 PST
This change is being reverted in https://bugs.webkit.org/show_bug.cgi?id=236006 as it broke form submission for forms containing multiple <input type="image">.

This is the failure scenario:

<form enctype="multipart/form-data" method="post">
  <input name="foo" type="image" alt="foo">
  <input name="bar" type="image" alt="bar">

Before this change, the following (correct) request was generated:

Content-Disposition: form-data; name="bar.x"

Content-Disposition: form-data; name="bar.y"


After the change, the following (incorrect) request was generated:

Content-Disposition: form-data; name="foo.x"

Content-Disposition: form-data; name="foo.y"

Comment 6 Andreu Botella 2022-02-08 13:08:07 PST
Blocked by bug 236324.
Comment 7 Andreu Botella 2022-02-13 10:12:00 PST
Created attachment 451822 [details]
Comment 8 Darin Adler 2022-02-13 11:06:00 PST
Comment on attachment 451822 [details]

View in context: https://bugs.webkit.org/attachment.cgi?id=451822&action=review

r=me except we need a patch with a corrected ChangeLog. So please don’t commit this without fixing that. Also please consider my comment about the new test.

> LayoutTests/fast/forms/image/multiple-image-inputs.html:6
> +      description("Multiple image inputs should work correctly.");

We’re missing a change log entry for this test and its expected result. That would be in LayoutTests/ChangeLog.

Also wondering why we need our own WebKit-specific test for this. Could we have added this testing to WPT?
Comment 9 Andreu Botella 2022-02-16 10:59:55 PST
> Also wondering why we need our own WebKit-specific test for this. Could we
> have added this testing to WPT?

I didn't add this test to WPT because Firefox doesn't seem to use the position of mouse events when determining the selected coordinate. The spec doesn't say anything about mouse events in that regard, so WebKit and Blink might be wrong here, but that's not what I meant to test.

In any case, I'll rewrite it as a WebDriver WPT test.
Comment 10 Andreu Botella 2022-02-20 06:23:41 PST
Created attachment 452693 [details]
Comment 11 EWS Watchlist 2022-02-20 06:26:16 PST
This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess
Comment 12 Darin Adler 2022-02-21 12:23:14 PST
Looks like the test is not working yet on the ios-wk2 EWS, which runs tests on the iOS simulator.
Comment 13 Tim Nguyen (:ntim) 2022-03-01 10:50:21 PST
(In reply to Darin Adler from comment #12)
> Looks like the test is not working yet on the ios-wk2 EWS, which runs tests
> on the iOS simulator.

iOS doesn't support test_driver.Actions, so in this case having a special iOS baseline is fine.
Comment 14 Tim Nguyen (:ntim) 2022-06-01 13:33:39 PDT

*** This bug has been marked as a duplicate of bug 239070 ***