Closed Bug 786172 Opened 7 years ago Closed 7 years ago

change event not fired for file dropped on file select box

Categories

(Core :: Layout: Form Controls, defect)

15 Branch
x86
All
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla18
Tracking Status
firefox16 + verified
firefox17 + verified
firefox18 + verified

People

(Reporter: cvwillegen, Assigned: drexler)

References

Details

(Keywords: regression, testcase, Whiteboard: [mentor=mounir][lang=c++])

Attachments

(2 files, 1 obsolete file)

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).
Attached file 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
Blocks: 722599
Status: UNCONFIRMED → NEW
Component: Untriaged → Layout: Form Controls
Ever confirmed: true
Keywords: regression
OS: Mac OS X → All
Product: Firefox → Core
Assignee: nobody → andrew.quartey
Andrew, if you don't have time to look into this just shout and we'll figure something out.
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.
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.
Whiteboard: [mentor=mounir][lang=c++]
Keywords: testcase
Just noticed this. I'll be looking into it.
Duplicate of this bug: 789959
Attached patch patch (obsolete) — Splinter Review
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??
Attachment #659942 - Flags: review?(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.
Attachment #659942 - Flags: review?(mounir) → review-
(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.
(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?
(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)
Attached patch patchSplinter Review
Fires the change event from layout. Passed locally; sent to Try: https://tbpl.mozilla.org/?tree=Try&rev=ab10a84f9bbb
Attachment #659942 - Attachment is obsolete: true
Attachment #660526 - Flags: review?(mounir)
Attachment #660526 - Flags: review?(mounir) → review+
Andrew, do you need me to push this to mozilla-inbound for you?
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/andrew.quartey@gmail.com-ab10a84f9bbb
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.
https://hg.mozilla.org/mozilla-central/rev/fc875f57b1dd
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
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
Attachment #660526 - Flags: approval-mozilla-beta?
Attachment #660526 - Flags: approval-mozilla-aurora?
Comment on attachment 660526 [details] [diff] [review]
patch

[Triage Comment]
Low risk fix for a FF15 regression - approving for branches.
Attachment #660526 - Flags: approval-mozilla-beta?
Attachment #660526 - Flags: approval-mozilla-beta+
Attachment #660526 - Flags: approval-mozilla-aurora?
Attachment #660526 - Flags: approval-mozilla-aurora+
(please land today or tomorrow morning to make it into the next beta build)
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
QA Contact: simona.marcu
Verified as working in my own situation as well.
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
Assignee: andrew.quartey → simona.marcu
Assignee: simona.marcu → andrew.quartey
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.
Thank you for the help Mario.
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.