786172 - change event not fired for file dropped on file select box

Status: VERIFIED FIXED

(Core :: Layout: Form Controls, defect)

Description

[cvwillegen]

For an example of a page showing the behaviour, see http://jsfiddle.net/7wR2L/

Drop a file onto the file select box. The change event is not triggered.

A few versions of FireFox ago, the change event did fire (I wrote a GreaseMonkey script using an .onchange handler some time ago, at that time the event did fire).

Comment 1

[Alice0775 White]

Attachment: sample html

Regression window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/333626f688a4
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/15.0 Firefox/15.0a1 ID:20120507160328
Bad:
http://hg.mozilla.org/mozilla-central/rev/fafd873d469c
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/15.0 Firefox/15.0a1 ID:20120507174628
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=333626f688a4&tochange=fafd873d469c

Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/97bef33bf606
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/15.0 Firefox/15.0a1 ID:20120507095528
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/14aa9de6287f
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/15.0 Firefox/15.0a1 ID:20120507100429
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=97bef33bf606&tochange=14aa9de6287f

Triggered by:
ba79daeeb874	Andrew Quartey — Bug 722599 - Move change event firing to content from layout r=mounir

Comment 2

[Josh Matthews [:jdm]]

Andrew, if you don't have time to look into this just shout and we'll figure something out.

Comment 3

[Alex Keybl [:akeybl]]

This sounds worse than the regressing bug 722599. Does that match other peoples' expectations? One alternative to properly fixing would be to back out bug 722599.

Comment 4

[Mounir Lamouri (:mounir)]

The issue is that nsHTMLInputElement::FireChangeEventIfNeeded() is assuming that it should only work for single line text control which a text field isn't. I wrote a quick patch changing that and it did work. However, it's not as trivial given that mFocusedValue has to be modified in consequence.

Comment 5

[Andrew Quartey [:drexler]]

Just noticed this. I'll be looking into it.

Comment 7

[Andrew Quartey [:drexler]]

Attachment: patch

Passed local tests. Sent to try: https://tbpl.mozilla.org/?tree=Try&rev=fd1fe7c26eeb. Left out appending a test for this. I looked at using sythesizeDrop: https://mxr.mozilla.org/mozilla-central/source/services/sync/tps/extensions/mozmill/resource/stdlib/EventUtils.js#530 but that fell short as i would need to simulate a dragging and dropping a local file - which i can easily QI to obtain it. Mounir ideas??

Comment 8

[Mounir Lamouri (:mounir)]

Comment on attachment 659942 [details] [diff] [review]
patch

Review of attachment 659942 [details] [diff] [review]:
-----------------------------------------------------------------

Your patch is full of trailing whitespaces. Please, make sure to remove them. You might want to configure your editor to have them being highlighted.

I think the solution you are proposing here might work but is a bit hacky. Maybe the best way to do this is to fire the change event from the layout.

You could do:
nsContentUtils::DispatchTrustedEvent(content->OwnerDoc(), content,
                                     NS_LITERAL_STRING("change"), true, false);
instead of calling:
FireChangeEventIfNeeded()

in nsFileControlFrame.cpp.

Comment 9

[Mounir Lamouri (:mounir)]

(In reply to Andrew Quartey [:drexler] from comment #7)
> Created attachment 659942 [details] [diff] [review]
> patch
> 
> Passed local tests. Sent to try:
> https://tbpl.mozilla.org/?tree=Try&rev=fd1fe7c26eeb. Left out appending a
> test for this. I looked at using sythesizeDrop:
> https://mxr.mozilla.org/mozilla-central/source/services/sync/tps/extensions/
> mozmill/resource/stdlib/EventUtils.js#530 but that fell short as i would
> need to simulate a dragging and dropping a local file - which i can easily
> QI to obtain it. Mounir ideas??

Regarding the tests, I'm not sure this can easily be done. I never tried sythesizeDrop for a file picker but it's clearly not working well enough for a text field.

You can try with sythesizeDrop and see how far you can go though.

Comment 10

[Andrew Quartey [:drexler]]

(In reply to Mounir Lamouri (:mounir) from comment #8)
 
> I think the solution you are proposing here might work but is a bit hacky.
> Maybe the best way to do this is to fire the change event from the layout.

Firing from layout seemed the obvious solution but then again with regards to bug 722599, isn't content meant to handle this rather than moving it back to layout?

Comment 11

[Mounir Lamouri (:mounir)]

(In reply to Andrew Quartey [:drexler] from comment #10)
> (In reply to Mounir Lamouri (:mounir) from comment #8)
>  
> > I think the solution you are proposing here might work but is a bit hacky.
> > Maybe the best way to do this is to fire the change event from the layout.
> 
> Firing from layout seemed the obvious solution but then again with regards
> to bug 722599, isn't content meant to handle this rather than moving it back
> to layout?

Given that the changes actually happens from the layout (ie. if there is no layout, you can't do that change), I wouldn't be shocked to have the change event fired from the layout.
I don't know if we should move that to the content in the future but for the moment, lets move that event back to the layout. (for example, SetFiles() could fire the event)

Comment 12

[Andrew Quartey [:drexler]]

Attachment: patch

Fires the change event from layout. Passed locally; sent to Try: https://tbpl.mozilla.org/?tree=Try&rev=ab10a84f9bbb

Comment 13

[Mounir Lamouri (:mounir)]

Andrew, do you need me to push this to mozilla-inbound for you?

Comment 14

[Mozilla RelEng Bot]

Try run for ab10a84f9bbb is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=ab10a84f9bbb
Results (out of 196 total builds):
    success: 178
    warnings: 16
    failure: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-ab10a84f9bbb

Comment 15

[Andrew Quartey [:drexler]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/fc875f57b1dd

Comment 16

[Alex Keybl [:akeybl]]

If we don't see any regression reports over the weekend, please nominate for uplift to Aurora/Beta Monday with a risk evaluation. If we don't land in time for Beta 4 on Tuesday or this is risky, we'll have to first fix in FF17.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

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

Comment 18

[Mounir Lamouri (:mounir)]

Comment on attachment 660526 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 722599
User impact if declined: change event will not be fired when using the file picker
Risk to taking this patch (and alternatives if risky): risks are very small given that this patch only changes the way the change event is fired from the file picker layout to make it quite explicit and direct instead of going trough the content.
String or UUID changes made by this patch: none

Comment 19

[Alex Keybl [:akeybl]]

Comment on attachment 660526 [details] [diff] [review]
patch

[Triage Comment]
Low risk fix for a FF15 regression - approving for branches.

Comment 20

[Alex Keybl [:akeybl]]

(please land today or tomorrow morning to make it into the next beta build)

Comment 21

[Scoobidiver (away)]

Landed in Aurora: http://hg.mozilla.org/releases/mozilla-aurora/rev/337bb2250bd2
Landed in Beta: http://hg.mozilla.org/releases/mozilla-beta/rev/bbe608b96368

Comment 22

[Simona Badau, Desktop QA]

Verified on Firefox 16 beta 4 by following the steps from the Description and the sample from comment 1 that the change event is fired for the files dropped on file select box.

Mozilla/5.0 (Windows NT 6.1; rv:16.0) Gecko/20100101 Firefox/16.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:16.0) Gecko/20100101 Firefox/16.0
Mozilla/5.0 (X11; Linux i686; rv:16.0) Gecko/20100101 Firefox/16.0

Comment 23

[cvwillegen]

Verified as working in my own situation as well.

Comment 24

[Simona Badau, Desktop QA]

Verified on Firefox 17 beta 1 that the change event is fired for the files dropped on file select box.

Verified on Windows 7, Ubuntu 12.04 and Mac OS X 10.7:
Mozilla/5.0 (Windows NT 6.1; rv:17.0) Gecko/17.0 Firefox/17.0
Mozilla/5.0 (X11; Linux i686; rv:17.0) Gecko/17.0 Firefox/17.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:17.0) Gecko/17.0 Firefox/17.0

Comment 25

[Mihai Morar, (:MihaiMorar)]

Verified on Firefox 18 beta 2 on Windows 7 x64, Ubuntu x32 and Mac OS X 10.8 and for all platforms it works as requested. It looks like this issue is no longer reproducible as it hadn't done on FF 17 too from comment 24 .

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/20100101 Firefox/18.0 (20121128060531)
Mozilla/5.0 (X11; Linux i686; rv:18.0) Gecko/20100101 Firefox/18.0 (20121128060531)
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:18.0) Gecko/20100101 Firefox/18.0 (20121128060531)

Please someone with permission set firefox18 flag as verified.

Comment 26

[u279076]

Thank you for the help Mario.