1653625 - <form>.requestSubmit() not sending data from attached <button> elements

Status: RESOLVED FIXED

(Core :: DOM: Forms, defect, P3)

Description

[sergio]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:78.0) Gecko/20100101 Firefox/78.0

Steps to reproduce:

Given a <form> with a <button> inside, I added an event listener for the 'click' event on the <button> in order to call <form>.requestSubmit(). I can show a simple demo:

<form id="form" method="POST" action="">
<button name="foo" value="bar">submit</button>
</form>

<script>
(function () {
'use strict';

    for (const button of document.querySelectorAll('#form button')) {
        button.addEventListener('click', onClick);
    }

    function onClick(evt) {
        const form = evt.currentTarget.form;

        if (form.requestSubmit) {
            form.requestSubmit();
        } else {
            form.submit();
        }
    }
}());    

</script>

Actual results:

The 'value' field from the <button> was not included in the form submission.

Expected results:

The 'value' field from the <button> should be submitted. That's the current behaviour on Chrome 84.

Comment 1

[Edgar Chen [:edgar]]

John might have some idea about the behavior of form.requestSubmit()

Comment 2

[John Dai[:jdai]]

Dear reporter,
The spec[1] said, form.requestSubmit() normally the buttons are excluded. Could you provide a reproducible test for me?

[1] https://html.spec.whatwg.org/#the-form-element

Comment 3

[John Dai[:jdai]]

Attachment: Bug 1653625 - Add wpt for requestSubmit should not contain an entry for the submit button

Comment 4

[sergio]

(In reply to John Dai[:jdai] from comment #2)

Dear reporter,
The spec[1] said, form.requestSubmit() normally the buttons are excluded. Could you provide a reproducible test for me?

[1] https://html.spec.whatwg.org/#the-form-element

Hi, thanks for looking into this. I think that paragraph in the spec about excluding the buttons refers to any <button> except the submitter. I don't know how the algorithm for constructing the entry list [1] for the form submission was implemented in Firefox, but I think it makes sense to include the submitter button as part of that list. That's actually the behaviour using the method form.submit(). Also Google Chrome shows that behaviour when using either form.requestSubmit() or form.submit().

I think form.requestSubmit() was mainly created to fix a couple of issues with form.submit():

• It triggers the submit signal.
• It runs native form validation.

Apart from that, I think you should expect the same behaviour than form.submit().

Here is a simple demo to reproduce this issue:

<form method="POST">
    <button name="foo" value="bar">Submit</button>
</form>
<script>
    (function () {
        'use strict';

        document.querySelector('button').addEventListener('click', (evt) => {
            evt.currentTarget.form.requestSubmit();
        });
    }());
</script>

Expected behaviour:

The entry list submitted within the form must include the name-value pair "foo=bar". That's the case replacing form.requestSubmit() with form.submit().
Google Chrome submits the name-value pair using both methods.

[1] https://html.spec.whatwg.org/#constructing-the-form-data-set

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

So the click event listener in the examples doesn't call event.preventDefault(), so there is a kind of double submitting happening. The default action for clicking the submit button, and then the explicit requestSubmit().
Need to check the spec which submit should "win".

Comment 6

[Edgar Chen [:edgar]]

John, I would like to refactor some code for bug 1709499 and realize that might touch some code related to this bug. I wonder if you are still working on this?

Comment 7

[Edgar Chen [:edgar]]

blocks bug 1370630 as Jenkins might run into the same issue per https://github.com/jenkinsci/jenkins/pull/5405#issuecomment-836960364.

Comment 8

[Edgar Chen [:edgar]]

(In reply to Edgar Chen [:edgar] from comment #6)

John, I would like to refactor some code for bug 1709499 and realize that might touch some code related to this bug. I wonder if you are still working on this?

I guess the answer is no ......

Comment 9

[Edgar Chen [:edgar]]

Attachment: test.html

Comment 10

[Edgar Chen [:edgar]]

(In reply to Olli Pettay [:smaug] from comment #5)

So the click event listener in the examples doesn't call event.preventDefault(), so there is a kind of double submitting happening. The default action for clicking the submit button, and then the explicit requestSubmit().
Need to check the spec which submit should "win".

Per https://html.spec.whatwg.org/#planned-navigation,

3. If the form has a non-null planned navigation, remove it from its task queue.

The latest submit action should win. So for the test case, the entry list should include the submitter value as the submission is from the default action of clicking.

Comment 11

[Edgar Chen [:edgar]]

Attachment: Bug 1653625 - Part 2: Make the submission from default action of a submit button is also deferred;

Comment 12

[Edgar Chen [:edgar]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1dbe5aff3eec75920bfffb832584052030f6a15a

Comment 13

[Edgar Chen [:edgar]]

Attachment: Bug 1653625 - Part 1: Simplify how we handle the pending submission;

Allow to override the unflushed pending submission, and add some assertion to
ensure form don't submit a submission if it is in defer state or there is an
unflushed pending submission.

Comment 14

[Edgar Chen [:edgar]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b301adf2c7b86d2745256db11a0e28e2440ab148

Comment 15

[Edgar Chen [:edgar]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=46974a34eca9a5eae6705abcd5705ac839058124

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Hi Edgar, do you have an update for this particular bug? Thanks!

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I want to ask again if there is a chance to proceed. If not do we have other bugs that actually should block this one? Thanks.

Comment 18

[Edgar Chen [:edgar]]

I will pick this up again in these days, basically the remaining work is to write more WPT tests and request review again.

Comment 19

[Phabricator Automation]

Comment on attachment 9234732 [details]
Bug 1653625 - Part 1: Simplify how we handle the pending submission;

Revision D121765 was moved to bug 1793075. Setting attachment 9234732 [details] to obsolete.

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Hi Edgar, do you plan to pick up the work on this bug given that bug 1793075 has been fixed about 3 months ago?

Comment 21

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #20)

Hi Edgar, do you plan to pick up the work on this bug given that bug 1793075 has been fixed about 3 months ago?

Hello Henrik, I will see if I can have alternatives to make progress on this issue.

Comment 22

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Adam is happy to put this into his queue . Thanks Adam!

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Hi Adam, I want to ask if you have a rough idea when you might be able to get started on this bug. Thanks!

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Could this bug may be considered for interop 2023 for forms (bug 1815183) ?

Comment 25

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Henrik Skupin [:whimboo][⌚️UTC+1] from comment #24)

Could this bug may be considered for interop 2023 for forms (bug 1815183) ?

I am not quite sure if the bug will resolve interop-2023 form wpts, that's why I just added "See Also" but not blocking the interop bug yet. As for comment 23, I talked with Adam yesterday, he has WIPs locally - I will leave his NI in case he has more clarity on timeline.

Comment 26

[Adam Vandolder [:avandolder]]

Attachment: Bug 1653625 - Make the submission from default action of a submit button also deferred. r?edgar

The submission from requestSubmit() call is non-deferred due to
mDeferSubmission
being cleared in HTMLFormElement::PostHandleEvent when handling a submit
event. The
mDeferSubmission clearing is also to make the submission from the default
action
of a submit button non-deferred.

This patch makes the submission from default action of a submit button
deferred
which could fix this bug, and also simplify the logic a bit.

Since gecko would also trigger submission for untrusted submit event,
see
bug 1370630, we need to handle that case specially.

Comment 27

[Adam Vandolder [:avandolder]]

This may require further tests to be added, but hopefully even with that it can be resolved soon.

Comment 28

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Hi Adam. Do you have a rough idea when you can continue working on this bug? Thanks.

Comment 29

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Hello Henrik,
Per our best understanding, in the past jenkins added a workaround to help with the transition to the new behavior (this bug and bug 1370630). Do these bugs still block them, do they still require these? We appreciate if you can help us get in touch with Jenkins and ask for the current status on their side. That will help us prioritize this work more properly. Thank you.

Comment 30

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I had a look over the bugs and the Jenkins issue again and here is the current state:

• Jenkins is fine as I've reported two years ago on bug 1370630 comment 84.
• I don't know the status with YUI 2.x given that caused problems in the past (see bug 1537555). Maybe Olli knows if we can do this change?
• Could we expect any other external regression?

Comment 31

[Olli Pettay [:smaug][[e-mail]]]

I don't know. But worth to still verify Gecko's implementation works the same way as others.

Comment 32

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I wonder if there are still pages out there that still use YUI2, which got deprecated in 2011. Are there some stats available somewhere?

Dennis, has the webcompat team a way to figure that out?

Comment 33

[Dennis Schubert [:denschub]]

Since I'll be on vacation next week, but Simon is out today, I'll redirect this needinfo to him. While we can't tell you for sure that no site is using YUI2, we can probably check the HTTP Archive for usages of YUI2 within the indexed pages.

Comment 34

[Simon Pieters [:zcorpan]]

16,436 pages out of 12,810,249 (~0.13%) use YUI 2.x in table httparchive.technologies.2023_04_01_desktop

SELECT
  COUNT(DISTINCT url) AS num
FROM
  `httparchive.technologies.2023_04_01_desktop`
WHERE
  app = "YUI"
  AND REGEXP_CONTAINS(info, r"^2\.")

(Same without WHERE to get the total.)

Comment 35

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Thanks Simon! Not sure if it matters or not but do you actually know if these are pages from the x number of popular (ranked) domains?

Comment 36

[Simon Pieters [:zcorpan]]

WITH technologies AS (
  SELECT DISTINCT
    url
  FROM
    `httparchive.technologies.2023_04_01_desktop`
  WHERE
    app = "YUI"
    AND REGEXP_CONTAINS(info, r"^2\.")
), ranks AS (
  SELECT
    url,
    rank
  FROM
    `httparchive.summary_pages.2023_04_01_desktop`
)
SELECT
  _rank AS rank,
  COUNT(0) AS pages
FROM
  technologies
JOIN
  ranks
USING
  (url),
  UNNEST([1000, 5000, 10000, 50000, 100000, 500000, 1000000, 5000000, 10000000, 50000000]) AS _rank
WHERE
  rank <= _rank
GROUP BY
  rank
ORDER BY
  rank

Result:

rank,pages
1000,1
5000,5
10000,13
50000,88
100000,196
500000,965
1000000,1857
5000000,6836
10000000,10899
50000000,16436

Comment 37

[Simon Pieters [:zcorpan]]

The pages with rank <= 10000 are

url,rank
https://www.geny.com/,1000
https://www.bathandbodyworks.com/,5000
http://dnd5e.wikidot.com/,5000
https://www.tagged.com/,5000
https://gosupermodel.com/,5000
http://www.planetsuzy.org/,10000
https://www.forum.hr/,10000
https://www.hi5.com/,10000
https://vipergirls.to/,10000
https://onlinebanking.bancogalicia.com.ar/,10000
https://secure.tagged.com/,10000
https://www.orientaltrading.com/,10000
https://www.gaiaonline.com/,10000

Comment 38

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Thank you Simon for the additional information. I think that the decision if such a change can or should be made needs to be done by the DOM team? If we want to do that maybe we could wait until Firefox 116 so that the 115 ESR release is not affected.

Comment 39

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Henrik Skupin [:whimboo][⌚️UTC+1] from comment #38)

Thank you Simon for the additional information. I think that the decision if such a change can or should be made needs to be done by the DOM team? If we want to do that maybe we could wait until Firefox 116 so that the 115 ESR release is not affected.

:whimboo, thank you for trying to gather all the separate pieces together. I discussed this with the team, given there are several duplicates of bug 1370630 ; generally it makes sense to fix this and bug 1370630. According to the current information and reports, we still think S3/P3 is the right rating. Landing this no early than Fx 116 sounds reasonable; for now we have to focus on other form priorities and not having a clear timeline for this issue yet.

Comment 40

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/aec89f8bbc3c
Make the submission from default action of a submit button also deferred. r=edgar

Comment 41

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/39996 for changes under testing/web-platform/tests

Comment 42

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/aec89f8bbc3c

Comment 43

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 44

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Hsin-Yi Tsai (she/her) [:hsinyi] from comment #39)

Landing this no early than Fx 116 sounds reasonable; for now we have to focus on other form priorities and not having a clear timeline for this issue yet.

Now this patch got landed for Firefox 115. Was that on purpose or by accident?

Comment 45

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Clearing NI as we chatted on Matrix - the webcompat risk of landing this bug in 115 (and will be in ESR 115) is thought to be small, while bug 1370630 is the main risk, that we don't want to ship in 115 (as well as ESR) without longer baking time.