Closed Bug 428135 Opened 17 years ago Closed 17 years ago

form submission event shouldn't bubble to parent forms

Categories

(Core :: DOM: Events, defect)

Product:
Core
Component:
DOM: Events
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: homebrew100775, Assigned: smaug)

References

(
URL
)

Details

(Keywords: regression, testcase)

Attachments

(4 files)

Form onsubmit testcase (works fine)
527 bytes, text/html
Details
Testcase.
577 bytes, text/html
Details
something like this
5.61 KB, patch
Details | Diff | Splinter Review
with tests
12.29 KB, patch
jst
: review+
sicking
: superreview+
beltzner
: approval1.9+
Details | Diff | Splinter Review
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.13) Gecko/20080311 Firefox/2.0.0.13 Build Identifier: Firefox 3 Beta 5 When clicking continue button, after entering user name and password for the web page specified, the browser doesn't proceed to next page like it's supposed to. Works in Firefox 2. Use user name "UMUC Guest" and password "umuc" as example. Reproducible: Always Steps to Reproduce: 1. Go to website 2. Enter user name and password 3. press enter 4. Doesn't proceed to next web page like it should
Confirmed on Windows XP that it doesn't work. Contents of error console: Login Manager: onStateChange accepted: req = http://tychousa8.umuc.edu/WebTycho.nsf, flags = 196612 Login Manager: onStateChange: adding dom listeners Login Manager: domEventListener: got event DOMContentLoaded Login Manager: Counting logins matching host: http://tychousa8.umuc.edu, formSubmitURL: , httpRealm: null Login Manager: onStateChange accepted: req = https://tychousa8.umuc.edu/sys/login.html?http://tychousa8.umuc.edu/WebTycho.nsf&0, flags = 196612 Login Manager: onStateChange: adding dom listeners Login Manager: domEventListener: got event DOMContentLoaded Login Manager: Counting logins matching host: https://tychousa8.umuc.edu, formSubmitURL: , httpRealm: null Login Manager: onStateChange accepted: req = about:blank, flags = 196612 Login Manager: onStateChange: adding dom listeners Login Manager: domEventListener: got event DOMContentLoaded Login Manager: observer notified for form submission. Login Manager: Checking if logins to https://tychousa8.umuc.edu can be saved. Login Manager: (form -- no username field found) Login Manager: Searching for logins matching host: https://tychousa8.umuc.edu, formSubmitURL: https://tychousa8.umuc.edu, httpRealm: null Pwmgr Prompter: ===== initialized ===== Pwmgr Prompter: Adding new password-save notification bar Login Manager: onStateChange accepted: req = about:blank, flags = 196612 Login Manager: onStateChange: adding dom listeners Login Manager: domEventListener: got event DOMContentLoaded
Status: UNCONFIRMED → NEW
Component: General → Password Manager
Ever confirmed: true
QA Contact: general → password.manager
Keywords: regression
Version: unspecified → Trunk
Bouncing back to General... This doesn't seem to be password manager related. It also fails with the PM disabled, and either way I don't see any errors being logged. I was initially suspicious of the <form onsubmit="SubmitIt();">, since no value is being returned (returning false cancels the form submittion). But it looks like we handle this right in a simple testcase, so that's not it. My other suspicion would be the <body onUnload="..."> handler. Things that would help here: * Regression range from when this stopped working on trunk * Minimized testcase
Component: Password Manager → General
OS: Windows XP → All
QA Contact: password.manager → general
Hardware: PC → All
This submits to itself, showing the current time. Clicking the "no return value" and "returns true" buttons works (the time increments), Clicking the "returns false" works as expected too (no time change since submission is canceled).
[Oh, err, you'll need to save that locally for the time. Otherwise a submittion results in a bugzilla error page. Same conclusion, though. :-)]
Oh, forgot to note: Another thing I noticed was that the page has a table interleaved with the forms. Basically: <form> <table> [gob of <tr> and <td>] <td> </form> <form name="realform"> </td> [gob of <tr> and <td>] </table> </form> The second form is the one that's expected to be used (and is being used), although there have been issues with this kind of thing in that past. So, this may or may not be part of the problem. :)
I'd bet it is. Minimal testcase strongly desired... I won't be able to create one myself for several weeks, unfortunately. :(
Keywords: qawanted
So when the submit event comes through to the form, its status is nsEventStatus_eConsumeNoDefault. So no submission happens. A smaller testcase would really help here; somehow someone is canceling that submit event, and there are a _lot_ of event handlers that fire when I click that button on that page. :(
Uh... So submit events seem to bubble. The page has two forms, as comment 5 says. In the DOM, one ends up a descendant of the other. The outer one has a "return false;" in its onsubmit handler. Looking at the patch in bug 234455, we used to have this code in nsHTMLFormElement::HandleDOMEvent: - // If this is the bubble stage, there is a nested form below us which received - // a submit event. We do *not* want to handle the submit event for this form - // too. So to avert a disaster, we stop the bubbling altogether. - if ((aFlags & NS_EVENT_FLAG_BUBBLE) && - (aEvent->message == NS_FORM_RESET || aEvent->message == NS_FORM_SUBMIT)) { - return NS_OK; - } It's gone away, with nothing to replace it that I can see. Per DOM spec, the event does bubble. So maybe we should just evangelize the site. But I wonder what IE does here, exactly...
Component: General → DOM: Events
Product: Firefox → Core
QA Contact: general → events
My gut instinct is that this is enough of a web-compat issue that we should really fix it for 1.9 final. But the fact that this is the first time it's been filed argues that maybe it's not as bad as I think....
Blocks: 234455
Flags: blocking1.9?
Keywords: qawanted
(In reply to comment #9) > Uh... So submit events seem to bubble. Oh, fascinating. I suppose that makes sense from a DOM purity point-of-view, but it seems wrong (or at least delightfully unexpected) for an outer form's onsubmit to be invoked when it's a different form being submitted...
This really does seem like something we really shouldn't change, no matter what the spec says. Unless we're now the odd kid on the block and work entirely differently than other browsers do. Testcase showing difference between Firefox 2 and 3 coming up.
Attached file Testcase. —
Using this testcase, only the first button displays the "foo" alert in Firefox 2, in Firefox 3 all buttons show the alert.
IE 7 appears to follow what Firefox 2 does here. I really think we should fix this for Firefox 3.
What Firefox 2 does is to stop the submit/reset event bubbling once it gets to a form that is not its target. As far as I can tell, browsers generally _do_ bubble the event, though. For IE7, are the forms ending up as kids of each other? Does IE7 bubble submit events at all (e.g., does an onsubmit attribute on <body> in that testcase get triggered)? And yeah, my gut feeling is that we should do what Gecko 1.8 did unless there's something that's very simple and gives better compat. I sort of doubt there is something like that....
It doesn't look like the forms nest in IE.
Blocking based on comment 14 and comment 15.
Flags: blocking1.9? → blocking1.9+
Summary: When clicking continue button, after entering un and pw, doesn't proceed to next page → form submission event shouldn't bubble to parent forms
This needs still mochitests etc.
Not nesting the forms was what I was sort of expecting in our parser too, to be honest... But that's not a change we can make for 1.9.
Keywords: testcase
If we make that parser change in the future, would we then make form submits bubble again (e.g. for XHTML)?
Webkit doesn't bubble submit/reset in XHTML either. (Not that I rely on webkit's event handling at all)
Attached patch with tests — — Splinter Review
1.8 passes all the tests, current 1.9 gives 16 failures. With the patch same results as with 1.8.
Assignee: nobody → Olli.Pettay
Status: NEW → ASSIGNED
Attachment #315844 - Flags: superreview?(jst)
Attachment #315844 - Flags: review?(jst)
Comment on attachment 315844 [details] [diff] [review] with tests I think we should do this, but requesting sr from Jonas to get another set of eyes on this, just in case. r=jst
Attachment #315844 - Flags: superreview?(jst)
Attachment #315844 - Flags: superreview?(jonas)
Attachment #315844 - Flags: review?(jst)
Attachment #315844 - Flags: review+
So what did we change between FF2 and FF3? Did we start bubbling these events whereas we didn't used to? Or did we change the parsing such that the forms are now nested? It's definitely a bummer that we're stopping the actual event propagation (which is what happens if I understand the patch right?) since that breaks the DOM events spec. It'd be somewhat nicer if we at the <form> element didn't execute the onsubmit/onreset attribute if target != currentTarget.
See comment #9. Event dispatch was rewritten and the old hack was removed. That patch puts it back. (In reply to comment #24) > It's definitely a bummer that we're stopping the actual event propagation > (which is what happens if I understand the patch right?) since that breaks the > DOM events spec. I know, but if the web requires the behavior... Webkit does it too.
(In reply to comment #24) > It'd be somewhat nicer if we at the <form> element didn't > execute the onsubmit/onreset attribute if target != currentTarget. Possible to do, but yet another behavior for submit/reset
(In reply to comment #26) > (In reply to comment #24) > > It'd be somewhat nicer if we at the <form> element didn't > > execute the onsubmit/onreset attribute if target != currentTarget. > Possible to do, but yet another behavior for submit/reset Yeah, but spec wise I'd say it deviates less from the DOM events spec. It means that you can still reliably listen for submit events on the document etc. Anyhow, it's definitely not something we want to do now this close to this release, but something to consider for next.
Attachment #315844 - Flags: superreview?(jonas) → superreview+
Attachment #315844 - Flags: approval1.9?
Comment on attachment 315844 [details] [diff] [review] with tests a1.9=beltzner
Attachment #315844 - Flags: approval1.9? → approval1.9+
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: