615697 - file input's change event is not propagating during bubble

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Potch [:potch]]

I came across this in one of the recent nightlies. In 3.6.x, I was able to set a change handler on a parent node of a file input, and the event bubbling worked properly. However, in the recent nightlies/beta 7, this is not working.

Selecting a file in the testcase in 3.6.x will trigger 4 handlers- 2 for the capture phase, two for the bubble phase. In 4.x, only 3 handlers are triggered- the delegate is not bubbled to.

I understand that the change event is a capture event by definition, but this is a regression from previous behavior, and I have not been able to find any documentation of when/why the change was made.

Comment 1

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

Regression range would be very useful here.

Comment 2

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

Ah, ok, I think I found it.

Comment 3

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

Probably a regression from Bug 592802.

Comment 4

[Mounir Lamouri (:mounir)]

Indeed, my bad.

This:
>    nsContentUtils::DispatchTrustedEvent(mInput->GetOwnerDoc(),
>                                         static_cast<nsIDOMHTMLInputElement*>(mInput.get()),
>                                         NS_LITERAL_STRING("change"), PR_FALSE,
>                                         PR_FALSE);

Should be changed to:
>    nsContentUtils::DispatchTrustedEvent(mInput->GetOwnerDoc(),
>                                         static_cast<nsIDOMHTMLInputElement*>(mInput.get()),
>                                         NS_LITERAL_STRING("change"), PR_TRUE,
>                                         PR_FALSE);

I very likely misread this: "the user agent must queue a task to first fire a simple event *that bubbles* named change at the input element" because simple events should not bubble by default.

Comment 5

[Mounir Lamouri (:mounir)]

Olli, feel free to assign me the bug.

Comment 6

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

OK, I can review.

Comment 7

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

Oh, and please add a testcase if possible.

Comment 8

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

And btw, I think both parameters should be PR_TRUE.
That is what change event seems to be in other cases in Gecko.
Better to test though.

Comment 9

[Mounir Lamouri (:mounir)]

(In reply to comment #7)
> Oh, and please add a testcase if possible.

Sure, as always.

(In reply to comment #8)
> And btw, I think both parameters should be PR_TRUE.

Indeed but is there a reason? This doesn't follow the specs nor other browser behavior.

Comment 10

[Mounir Lamouri (:mounir)]

> (In reply to comment #8)
> > And btw, I think both parameters should be PR_TRUE.
> 
> Indeed but is there a reason? This doesn't follow the specs nor other browser
> behavior.

Oups, I meant to quote "That is what change event seems to be in other cases in Gecko."

Comment 11

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

I think the second test should be used for a follow up except if there is a reason to let cancel being cancelable.

Comment 12

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

(In reply to comment #9)
> Indeed but is there a reason? This doesn't follow the specs nor other browser
> behavior.
Yeah, the only reason is consistency within Gecko.

Comment 13

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

Comment on attachment 494191 [details] [diff] [review]
Patch v1

So if I read the code right, "change" event is cancelable in other cases
(in Gecko), so for consistency please make it cancelable here too, and
then perhaps file a followup to change is non-cancelable everywhere.
That followup bug doesn't need to block 2.0.

Comment 14

[Mounir Lamouri (:mounir)]

Attachment: Patch v1.1

r=smuag

I will push the test for non file inputs with the follow-up.

Comment 15

[Johnny Stenback (:jst)]

Blocking on this regression.

Comment 16

[Mounir Lamouri (:mounir)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/a1c92bbac6ac