<form>.requestSubmit() not sending data from attached <button> elements
Categories
(Core :: DOM: Forms, defect, P3)
Tracking
()
Tracking | Status | |
---|---|---|
firefox115 | --- | fixed |
People
(Reporter: sergio, Assigned: avandolder)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 3 obsolete files)
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.
Updated•4 years ago
|
Comment 1•4 years ago
|
||
John might have some idea about the behavior of form.requestSubmit()
Comment 2•4 years ago
|
||
Dear reporter,
The spec[1] said, form.requestSubmit() normally the buttons are excluded. Could you provide a reproducible test for me?
Comment 3•4 years ago
|
||
Updated•4 years ago
|
(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?
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
Updated•4 years ago
|
Updated•4 years ago
|
Comment 5•4 years ago
|
||
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•3 years ago
|
||
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•3 years ago
•
|
||
blocks bug 1370630 as Jenkins might run into the same issue per https://github.com/jenkinsci/jenkins/pull/5405#issuecomment-836960364.
Comment 8•3 years ago
|
||
(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 ......
Updated•3 years ago
|
Comment 9•3 years ago
|
||
Comment 10•3 years ago
|
||
(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,
- 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.
Updated•3 years ago
|
Updated•3 years ago
|
Comment 11•3 years ago
|
||
Comment 12•3 years ago
|
||
Comment 13•3 years ago
|
||
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.
Updated•3 years ago
|
Comment 14•3 years ago
|
||
Comment 15•3 years ago
|
||
Comment 16•2 years ago
|
||
Hi Edgar, do you have an update for this particular bug? Thanks!
Updated•2 years ago
|
Comment 17•2 years ago
|
||
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•2 years ago
|
||
I will pick this up again in these days, basically the remaining work is to write more WPT tests and request review again.
Updated•2 years ago
|
Comment 19•2 years ago
|
||
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•1 year ago
|
||
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•1 year ago
|
||
(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•1 year ago
|
||
Adam is happy to put this into his queue . Thanks Adam!
Comment 23•1 year ago
|
||
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•1 year ago
|
||
Could this bug may be considered for interop 2023 for forms (bug 1815183) ?
Comment 25•1 year ago
|
||
(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.
Assignee | ||
Comment 26•1 year ago
|
||
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.
Updated•1 year ago
|
Assignee | ||
Comment 27•1 year ago
|
||
This may require further tests to be added, but hopefully even with that it can be resolved soon.
Comment 28•1 year ago
|
||
Hi Adam. Do you have a rough idea when you can continue working on this bug? Thanks.
Comment 29•1 year ago
|
||
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•1 year ago
|
||
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•1 year ago
|
||
I don't know. But worth to still verify Gecko's implementation works the same way as others.
Comment 32•1 year ago
|
||
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•1 year ago
|
||
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•1 year ago
•
|
||
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•1 year ago
|
||
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•1 year ago
|
||
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•1 year ago
|
||
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•1 year ago
|
||
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•1 year ago
•
|
||
(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.
Updated•1 year ago
|
Comment 40•11 months ago
|
||
Pushed by avandolder@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/aec89f8bbc3c Make the submission from default action of a submit button also deferred. r=edgar
Comment 41•11 months ago
|
||
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/39996 for changes under testing/web-platform/tests
Comment 42•11 months ago
|
||
bugherder |
Comment 43•11 months ago
|
||
Upstream PR merged by moz-wptsync-bot
Comment 44•11 months ago
|
||
(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?
Updated•11 months ago
|
Comment 45•11 months ago
|
||
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.
Assignee | ||
Updated•11 months ago
|
Updated•11 months ago
|
Description
•