Open Bug 1370630 Opened 7 years ago Updated 8 months ago

preventDefault() on form.dispatchEvent(new Event('submit'))?

Categories

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

Product:
Core
Component:
DOM: Forms
Type:
defect

Tracking

()

Project Flags:
Webcompat Priority P3
Tracking Flags:
Tracking Status
relnote-firefox --- -

People

(Reporter: janx, Assigned: edgar)

References

(Blocks 1 open bug,
URL
)

Details

(Keywords: dev-doc-needed, site-compat)

Attachments

(2 files, 2 obsolete files)

Bug 1370630 - Untrusted submit event shouldn't trigger form submission
3.63 KB, patch
smaug
: review+
Details | Diff | Splinter Review
Bug 1370630 - Untrusted submit event shouldn't trigger form submission; r=smaug
47 bytes, text/x-phabricator-request
Details | Review 
When pasting the following address into various browsers, we get different results:

data:text/html,
<form action="https://www.mozilla.org/" id="form">
  <input type="submit" value="Navigate maybe">
</form>
<script>
  var form = document.getElementById("form");
  form.addEventListener("submit", function (event) {
    console.log(event.cancelable);
    event.preventDefault();
  });
  form.dispatchEvent(new Event('submit'));
</script>

- Firefox will navigate away (`event.preventDefault()` won't work)
- Chrome will not navigate away (`event.preventDefault()` will have the last word)

The correct behavior according to specs is to navigate (like Firefox does), because `new Event('submit')` is not cancelable unless you specify `new Event('submit', { cancelable: true })`.

I'm not sure if Chrome's behavior is a bug, or if it's intended to be helpful (my code does look like I'm trying hard not to submit), so I'll file a Chrome bug as well.

See also: https://stackoverflow.com/questions/40916300/how-can-i-preventdefault-on-dispatchevent
Thanks for the bug! It would be useful to know if this breaks sites for us to help us decide what to do here (i.e., implement Chrome's behavior and update the spec, or just close as INVALID).


I added the "hotlist-interop" label to the Chromium bug.
Component: General → DOM: Events
Product: Web Compatibility → Core
Stone may have thoughts here. Otherwise we can wait for Anne to return from PTO.
Flags: needinfo?(sshih)
Flags: needinfo?(annevk)
Like Janx's description, FF follows the spec and Chrome's behavior is helpful to web authors. Considering to be helpful to web authors, maybe we could consider giving the corresponding values of bubbles and cancelable for each kind of event so that users could simply create an event and dispatch it without caring the bubbles and cancelable values. But I don't know if it's worth to do it.
Flags: needinfo?(sshih)
Priority: -- → P3
FYI, the Chromium bug was closed as WONTFIX:

From https://bugs.chromium.org/p/chromium/issues/detail?id=730108#c4
> This is a bug of Firefox. Dispatching non-trusted 'submit' event should not start form submission.  The specification doesn't define such behavior AFAIK.
FYI Safari and Edge don't seem to navigate away either (Chrome's behavior).

Firefox seems to be the only browser that navigates away on the example code.
According to [1], untrusted events shouldn't trigger the UA default actions.

[1] https://www.w3.org/TR/uievents/#trusted-events
Assignee: nobody → sshih
To close my needinfo: Ming-Chou is correct (although note that there's an exception for the click event, as defined by https://dom.spec.whatwg.org/ in the Events section).
Flags: needinfo?(annevk)
Attachment #8879069 - Flags: review?(bugs)
Comment on attachment 8879069 [details] [diff] [review]
Bug 1370630 - Untrusted submit event shouldn't trigger form submission

Need to change also HTMLFormElement::GetEventTargetParent and maybe also HTMLFormElement::WillHandleEvent
Attachment #8879069 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] from comment #9)
> Comment on attachment 8879069 [details] [diff] [review]
> Bug 1370630 - Untrusted submit event shouldn't trigger form submission
> 
> Need to change also HTMLFormElement::GetEventTargetParent and maybe also
> HTMLFormElement::WillHandleEvent
I didn't change WillHandleEvent because it's related to the event propagation of nested form elements and I don't see a reason to have different behaviors of trusted or untrusted events.
Attachment #8883489 - Flags: review?(bugs)
Attachment #8883489 - Flags: review?(bugs) → review+
Pushed by sshih@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/367b6f947f87
Untrusted submit event shouldn't trigger form submission. r=smaug.
https://hg.mozilla.org/mozilla-central/rev/367b6f947f87
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
So, I have a mozilla-internal jenkins site (that uses YUI), that has been broken by this. I used mozregression to bisect the site problem back to exactly this change. 

Unfortunately, that site is not available to mozilla employees in general, so not sure how to give you something you can debug. Although, perhaps since this change might have impact on sites, this may be just an example of one (Jenkins).
reopened & marked as regression to catch folks eyes sooner.
Status: RESOLVED → REOPENED
Has Regression Range: --- → yes
Keywords: regression
Resolution: FIXED → ---
Note: the site does work Chrome Version 59.0.3071.115 on macOS 10.12.5
This one is fixed. Please file a followup bug or ask this one to be backed out (and once backed out, reopen).
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
Resolution: --- → FIXED
John, do you know why the site works on Chrome? I assume Jenkins uses some Gecko specific code it shoudn't use.
Flags: needinfo?(jrgm)
Mozregression shows this bug breaks https://webcompat.com/issues/8128 or any jenkins login, any idea?
Backout by sshih@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c7c8b33ba324
Backed out changeset 367b6f947f87 for breaking mozilla-internal jenkins site. r=backout.
Added an URL of a public Jenkins server
URL: https://jenkins.qa.ubuntu.com/login?f...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/c7c8b33ba324
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
Resolution: --- → FIXED
reopen again
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Change Chrome's user agent to be the same as Firefox, Chrome doesn't work anymore. Apply my patch and change the user agent to be the same as Chrome, button submission works.
(In reply to Ming-Chou Shih [:stone] from comment #25)
> Change Chrome's user agent to be the same as Firefox, Chrome doesn't work
> anymore. Apply my patch and change the user agent to be the same as Chrome,
> button submission works.

Hi Eric,
According to the testing results, it's because the website behaves differently for different user agents and I have no idea how to proceed. Any suggestions about it?
Flags: needinfo?(etsai)
hi :stone, could you help to update some more detail about different behavior from which website? the public Jenkins server or mine private one?
Flags: needinfo?(etsai)
I verified on http://jenkins2.legacyserver.in/

I can verify it on the public server later.
(In reply to Ming-Chou Shih [:stone] from comment #28)
> I verified on http://jenkins2.legacyserver.in/
> 
> I can verify it on the public server later.
Oops. I don't have an account nor find a link to sign up.
(In reply to Ming-Chou Shih [:stone] from comment #29)
> (In reply to Ming-Chou Shih [:stone] from comment #28)
> > I verified on http://jenkins2.legacyserver.in/
> > 
> > I can verify it on the public server later.
> Oops. I don't have an account nor find a link to sign up.

Hi Eric,
I only can confirm that the problem on [1] is caused by the user agent. I don't have access to the public jenkins website [2]. Any suggestions?

[1] http://jenkins2.legacyserver.in/
[2] https://jenkins.qa.ubuntu.com/login?from=%2F
Flags: needinfo?(etsai)
Per-talk with :stone, I think if it's solved in an internal jenkins server, we should land first to make sure the behavior is correct.
If there is any UA detection makes this not work, please file another bug in webcompat general or Tech_Evangelism to outreach.
Flags: needinfo?(etsai)
See Also: → 1399783
See Also: → 1477286
Depends on: 1399783
Keywords: regression
See Also: 1399783 → 
https://github.com/jenkinsci/jenkins/pull/3689 is a PR to make Jenkins work again as soon the fix has landed in FF, however that means the server needs to use the newest version of jenkins or patch their version of hudson-behavior.js as demonstrated in the pr.
Blocks: 1487705
Assignee: stone123456 → echen

:stone, would you mind updating the patch? As https://issues.jenkins-ci.org/browse/JENKINS-53462 states the depending Jenkins issue will be fixed soon. As such it would be great to have a build for testing the fix. Thanks!

Flags: needinfo?(stone123456)

:stone is no longer active. I have rebased the patch and also triggered a try run, https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f12e17b558a36ef5b65235d663c4e617c3d328e

Flags: needinfo?(stone123456)

Thanks Edgar! I will forward that link to the Jenkins people for testing.

So the Jenkins patch was tested with the above try build of Firefox, and they were able to confirm that their fix works with the patch on this bug. So it will go live in Jenkins 2.174. When the LTS release will get the fix isn't clear yet. But here the comment from one of the maintainers:

https://issues.jenkins-ci.org/browse/JENKINS-53462?focusedCommentId=364824&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-364824

2.164.3, scheduled for May 8, the earliest, if accepted as a backport. The next .1, scheduled for early June, otherwise. Assuming of course we don't cause regressions again.

As such we might have to wait until early June at latest before we can push this patch. I will keep an eye on the releases, and will let you know.

Actually including this patch in Firefox in the near future might not be easy given the following reply from Daniel Beck about the usage statistics of Jenkins LTS versions:

https://issues.jenkins-ci.org/browse/JENKINS-53462?focusedCommentId=365324&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-365324

Sylvestre, what is the take from Release managers in how best we can get this fixed without annoying users of Jenkins?

Flags: needinfo?(sledru)
Target Milestone: mozilla56 → ---

We would need to get the patch in nightly.

Depends when Edgar can get it landed but it will be probably too late for 67

Flags: needinfo?(sledru)

(In reply to Sylvestre Ledru [:sylvestre] from comment #40)

We would need to get the patch in nightly.

That for sure...

Depends when Edgar can get it landed but it will be probably too late for 67

No, we don't clearly want it for 67 and maybe also not for 68, whereby this would be great. Please see the link from my last comment in why this is a bit more complicated to land without affecting Jenkins users. If we don't care we could land after the next merge. Then there are still 12 weeks left before we reach release, but some users might not have updated their Jenkins installation by then. From my experience I can say it's not always easy and takes time.

So landing this patch for Firefox 69 affected Jenkins folks could still use our upcoming Firefox 68 ESR release with older versions of Jenkins. Maybe that could be a compromise?

Flags: needinfo?(sledru)

Forwarding needinfo for the last comment from Sylvestre to Mike.

Flags: needinfo?(sledru) → needinfo?(miket)

So landing this patch for Firefox 69 affected Jenkins folks could still use our upcoming Firefox 68 ESR release with older versions of Jenkins. Maybe that could be a compromise?

That sounds reasonable to me.

Flags: needinfo?(miket)

Great! So lets wait for May 14th or 15th when the next merge has been done. I will set a reminder for it.

Flags: needinfo?(jrgm)

FYI a new Jenkins LTS got released 2 days ago:
https://jenkins.io/changelog-stable/#v2.164.3

And it contains:

Make form submit buttons on the Jenkins classic UI compatible with potentially upcoming Firefox bug fix. (issue 53462, Firefox bug 1370630)

Hopefully we will be able to land this patch after our next merge. I will continue to monitor.

After 14 days there are still no reports for Jenkins which claim that their patch has been caused any kind of problems. As such do we want to move forward on this bug and get the patch landed for Firefox 69 only?

Flags: needinfo?(echen)

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

After 14 days there are still no reports for Jenkins which claim that their patch has been caused any kind of problems. As such do we want to move forward on this bug and get the patch landed for Firefox 69 only?

Yes, lets land this patch for 69. Thanks.

Flags: needinfo?(echen)
Pushed by echen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ab9d5eeaa90e
Untrusted submit event shouldn't trigger form submission; r=smaug

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

Status: REOPENED → RESOLVED
Closed: 7 years ago → 5 years ago
status-firefox69: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla69

Edgar, is that a change we actually should put into the developer release notes for the 69 release?

Flags: needinfo?(echen)

Release Note Request (optional, but appreciated)
[Why is this notable]: Form submission now won't be triggered by un-trusted event which is generated from web page, some web page might rely on this special behavior of Firefox, like bug 1399783. It's better to put into release notes.
[Affects Firefox for Android]: Yes
[Suggested wording]:
[Links (documentation, blog post, etc)]:

relnote-firefox: --- → ?
Flags: needinfo?(echen)
Regressions: 1556980

That's a developer note that is needed, so MDN works. Adjusting flags accordingly.

relnote-firefox: ? → -
Keywords: dev-doc-needed
Keywords: site-compat

Is there any effective difference between patch landed here and that in bug 1535988?

The patch in bug 1535988 caused bug 1537555 and thus was reverted. Would this change cause the exactly the same issue again?

And it seems that the answer is yes. Bug 1556980 is just the new bug 1537555 :/

Regressions: 1564080

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #54)

Is there any effective difference between patch landed here and that in bug 1535988?

The patch in bug 1535988 caused bug 1537555 and thus was reverted. Would this change cause the exactly the same issue again?

Right. Need to back out.

ni? Edgar re: Comment #57

Flags: needinfo?(echen)

Actually, it seems that Edgar is on leave. Oops.

Olli, can you help w/ the backout?

Flags: needinfo?(echen) → needinfo?(bugs)

Oh wait, I can just ask a Sheriff. Sorry for the mailspam.

Flags: needinfo?(bugs)

Backed out changeset ab9d5eeaa90e (bug 1370630) on dev's request

Backout:
https://hg.mozilla.org/integration/autoland/rev/08a4f86a9afa4b2957e0438f9fa8f86778eb3b7f

Status: RESOLVED → REOPENED
status-firefox69: fixed → ---
Resolution: FIXED → ---
Target Milestone: mozilla69 → ---

Ryan, how do I request uplift for a backout changeset? The original patch landed in 69, but the backout happened in 70.

Flags: needinfo?(ryanvm)

Yeah, let's get the backout on Beta too before we build 69.0b4.

Flags: needinfo?(ryanvm) → needinfo?(aryx.bugmail)

So do I understand correctly that landing of this patch is blocked by pages still using YUI2 which is deprecated since 2011? Or is there something more?

status-firefox56: fixed → ---
Depends on: 1537555
Severity: normal → S3
Component: DOM: Events → DOM: Forms

Hi Dennis, we would like to remove this firefox-only behaviour, but Jenkins does some UA detection and does different things, idealy they should just do the same thing as other browsers. Per https://github.com/jenkinsci/jenkins/pull/3689, it seems Jenkins already landed the issue, but https://issues.jenkins-ci.org/browse/JENKINS-53462 was reopen? Could someone from webcompat team help to check the status of Jenkins? Thanks

In the mean time, I will try to add Telemetry to see the usage of this.

Flags: needinfo?(dschubert)

This is probably something for Henrik, as he was in touch with the Jenkins folk in their issue tracker. Henrik, can you ping them again? :)

Flags: needinfo?(dschubert) → needinfo?(hskupin)

I asked on the Jenkins issue. Will report back when I get an update.

Flags: needinfo?(hskupin)

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

Hi Dennis, we would like to remove this firefox-only behaviour, but Jenkins does some UA detection and does different things, idealy they should

So what about the YUI 2.x issues that were detected in comment 54 and following? As a result of that your patch on this bug has been backed out from 69 beta. Did anything change here in the last year?

Flags: needinfo?(echen)
Flags: needinfo?(bugs)

Edgar it would be great to get your feedback here. The Selenium project cannot make the change without that bug fixed first.

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

So what about the YUI 2.x issues that were detected in comment 54 and following?

Yes, the issue still there, not sure how YUI3 handle this.

As a result of that your patch on this bug has been backed out from 69 beta. Did anything change here in the last year?

No, just try to make some progress, what I plan to do is

  • Land the fix, but behind a pref and off by default.
  • Add Telemetry to see the usage of this.
  • Maybe show some warnning on console saying this is deprecated.
Flags: needinfo?(echen)
Flags: needinfo?(bugs)

That sounds fine. I can reach out to Jenkins again whenever we know that we want to turn this feature on by default. Note that the best time might be again after the next major ESR release whatever this will be?

ni myself for comment# 72.

Flags: needinfo?(echen)
Flags: needinfo?(echen)
Keywords: leave-open
Pushed by echen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/49f51744024c
Untrusted submit event shouldn't trigger form submission; r=smaug
Attachment #9201179 - Attachment description: Bug 1370630 - Add UserCounter and deprecate warnning message for form submission from untrusted event; → Bug 1370630 - Add UserCounter and deprecate warnning message for form submission via untrusted submit event;

Edgar please let me know when I should reach out to the Jenkins community again, and with which information. Thanks.

Depends on: 1691357

Comment on attachment 9201179 [details]
Bug 1370630 - Add UserCounter and deprecate warnning message for form submission via untrusted submit event;

Revision D104070 was moved to bug 1691357. Setting attachment 9201179 [details] to obsolete.

Attachment #9201179 - Attachment is obsolete: true

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

No, just try to make some progress, what I plan to do is

  • Land the fix, but behind a pref and off by default.
  • Add Telemetry to see the usage of this.
  • Maybe show some warnning on console saying this is deprecated.

Hi Edgar, would you mind to give an update of where we are right now? Thanks

Flags: needinfo?(echen)

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

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

  • Land the fix, but behind a pref and off by default.

Fix is landed but behind a pref, dom.forms.submit.trusted_event_only, in https://hg.mozilla.org/mozilla-central/rev/49f51744024c.

  • Add Telemetry to see the usage of this.
  • Maybe show some warnning on console saying this is deprecated.

Telemetry and warning on console are handled in bug 1691357.

Flags: needinfo?(echen)

I just submitted a comment to the Jenkins issue to let them know that a preference exists to turn on the feature. Lets see if that helps them to get the remaining issues solved.

Regarding Telemetry is there a place that can be checked to get the current count? As I read the telemetry probe will be around for 6 months and we are already 1/3 through that time. Thanks!

URL: https://jenkins.qa.ubuntu.com/login?f... → https://issues.jenkins.io/browse/JENK...
Flags: needinfo?(echen)

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

I just submitted a comment to the Jenkins issue to let them know that a preference exists to turn on the feature. Lets see if that helps them to get the remaining issues solved.

Thanks.

Regarding Telemetry is there a place that can be checked to get the current count? As I read the telemetry probe will be around for 6 months and we are already 1/3 through that time. Thanks!

You can check the data in
https://telemetry.mozilla.org/new-pipeline/evo.html#!aggregates=False%2520percentage!True%2520percentage!Invalid%2520value%2520percentage&cumulative=0&end_date=2021-04-09&include_spill=0&keys=!__none__!__none__&max_channel_version=nightly%252F89&measure=USE_COUNTER2_DEPRECATED_FormSubmissionUntrustedEvent_PAGE&min_channel_version=nightly%252F86&processType=*&product=Firefox&sanitize=1&sort_keys=submissions&start_date=2021-03-22&trim=1&use_submission_date=0

Flags: needinfo?(echen)

A PR for Jenkins got merged end of last week that fixes the problem on their side. It has been tested with both configurations and so far it seems to be fine. Note that they don't want to rush a patch like that again into a LTS release, so having it properly fixed would mean a time around early June or at maximum late August.

Depends on: 1653625
Webcompat Priority: --- → ?

Edgar, I wanted to give a quick update regarding Jenkins. Given my last comment the changes as pushed with the above PR are stable now and even reached the LTS release by June this year. As such Jenkins is no longer a blocker.

So next will be bug 1653625 then?

Flags: needinfo?(echen)
Status: REOPENED → NEW
Webcompat Priority: ? → P3

Thanks for the update! Yeah, I think we should fix bug 1653625 first and then we could try to enable the pref on Nightly again.

Flags: needinfo?(echen)

The leave-open keyword is there and there is no activity for 6 months.
:edgar, maybe it's time to close this bug?
For more information, please visit auto_nag documentation.

Flags: needinfo?(echen)

The severity field for this bug is relatively low, S3. However, the bug has 3 duplicates.
:edgar, could you consider increasing the bug severity?

For more information, please visit auto_nag documentation.

Flags: needinfo?(echen)
Flags: needinfo?(echen)
Duplicate of this bug: 1826582

Summarize the timeline discussion from Bug 1653625 comment 45 - the webcompat risk of landing bug 1653625 in 115 (and will be in ESR 115) is thought to be small, while this bug 1370630 is the main risk, that we don't want to ship in 115 (as well as ESR) without longer baking time.

Now that Firefox 115 is on beta will the patches on this bug land soon so that there is enough baking time before the next to beta merge? Thanks.

Flags: needinfo?(htsai)

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

Now that Firefox 115 is on beta will the patches on this bug land soon so that there is enough baking time before the next to beta merge? Thanks.

Hey, no, I don't expect any timeline planned for this yet (but happy to have this being picked up when we have bandwidth); there are other tasks with higher priorities than this in the form area. I think S3/P3 is still the right rating, as commented here. Let me know if I misunderstood anything.

Flags: needinfo?(htsai)
Blocks: 1850198

(I don't think this is a regression)

Keywords: leave-open, regression
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: