Last Comment Bug 709448 - File input click() handling should perhaps allow openControlled popups
: File input click() handling should perhaps allow openControlled popups
Status: VERIFIED FIXED
[qa!]
: testcase
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla13
Assigned To: Mounir Lamouri (:mounir)
:
Mentors:
Depends on:
Blocks: 681922
  Show dependency treegraph
 
Reported: 2011-12-09 23:46 PST by Boris Zbarsky [:bz]
Modified: 2012-04-26 06:54 PDT (History)
11 users (show)
geoff: in‑testsuite+
geoff: in‑litmus-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
-
wontfix
+
verified
+
verified
12+
verified


Attachments
testcase (475 bytes, text/html)
2011-12-10 00:54 PST, Jonas Sicking (:sicking) PTO Until July 5th
no flags Details
Patch v1 (3.34 KB, patch)
2011-12-21 09:06 PST, Mounir Lamouri (:mounir)
bzbarsky: review+
Details | Diff | Splinter Review
fixed patch (3.58 KB, patch)
2012-02-06 15:34 PST, Geoff Lankow (:darktrojan)
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta-
Details | Diff | Splinter Review
Patch for esr10 (3.54 KB, patch)
2012-04-11 03:10 PDT, Mounir Lamouri (:mounir)
lukasblakk+bugs: approval‑mozilla‑esr10+
Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] 2011-12-09 23:46:36 PST
We generally allow popups in mouseup handlers, but we don't allow click() on file inputs to work there.  Is this on purpose?
Comment 1 Mounir Lamouri (:mounir) 2011-12-10 00:46:48 PST
Do you mean you are not able to open a popup from a click event handling in a file control or you can't do that if .click() has been called? Both seems to work for me so I really wonder what you are trying to describe.
Comment 2 Jonas Sicking (:sicking) PTO Until July 5th 2011-12-10 00:54:00 PST
Created attachment 580628 [details]
testcase

Actual results:

from click handler: works
from timeout handler: blocked
from mouseup handler: blocked
from mousedown handler: blocked

Expected results:

whereever window.open is allowed to open a popup.
Comment 3 Boris Zbarsky [:bz] 2011-12-10 01:16:19 PST
Mounir, doing window.open() from a mouseup handler works.  Doing .click() on a file input from a mouseup handler on some other element doesn't work.
Comment 4 Jonas Sicking (:sicking) PTO Until July 5th 2011-12-14 14:46:49 PST
Mounir, could you take this. It should be a pretty easy change, right?
Comment 5 Mounir Lamouri (:mounir) 2011-12-21 09:06:15 PST
Created attachment 583526 [details] [diff] [review]
Patch v1

Note that we do not allow popups on mousedown. I don't really know why. I should check what other browsers do and see with jst if he has memories of that (or you bz, maybe?).
Comment 6 Boris Zbarsky [:bz] 2011-12-21 09:48:07 PST
Comment on attachment 583526 [details] [diff] [review]
Patch v1

r=me

I'm not sure what the reasoning was for mousedown, but arguably allowing popups on drag out of the page would be poor.
Comment 7 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-02-02 13:27:41 PST
Can we land this?
Comment 8 Boris Zbarsky [:bz] 2012-02-02 18:16:35 PST
There are test conflicts with the patch for bug 712518.  I'm not quite sure how to merge the test, actually.  Geoff or Mounir, could you please take a look?

Nominating for tracking; in case it wasn't clear this is breaking some Google apps....
Comment 9 Geoff Lankow (:darktrojan) 2012-02-06 15:34:14 PST
Created attachment 594844 [details] [diff] [review]
fixed patch
Comment 10 Geoff Lankow (:darktrojan) 2012-02-11 20:41:36 PST
I've pushed this to inbound, there seems to be no reason to hold it back any longer. (Also khuey told me to, so blame him.)
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ac07d3e3a61
Comment 11 Boris Zbarsky [:bz] 2012-02-11 20:44:02 PST
Yes, this should have landed as soon as the merge conflicts from comment 8 were resolved.

Want to put together patches for beta and aurora too?
Comment 12 Geoff Lankow (:darktrojan) 2012-02-11 20:50:42 PST
The same patch should work on aurora and the older patch on beta, but I'll check.
Comment 13 Marco Bonardo [::mak] (Away 6-20 Aug) 2012-02-13 08:46:57 PST
https://hg.mozilla.org/mozilla-central/rev/6ac07d3e3a61
Comment 14 Mounir Lamouri (:mounir) 2012-02-14 06:39:34 PST
My apologies for not pushing that. I didn't realize it was breaking content and I had another patch before this one in my queue which wasn't ready to be landed...
Comment 15 Mounir Lamouri (:mounir) 2012-02-14 06:41:39 PST
Comment on attachment 594844 [details] [diff] [review]
fixed patch

[Approval Request Comment]
User impact if declined: according to comment 8, some Google Apps are failing
Risk to taking this patch (and alternatives if risky): changes content behavior
Comment 16 Alex Keybl [:akeybl] 2012-02-14 11:20:01 PST
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #15)
> Comment on attachment 594844 [details] [diff] [review]
> fixed patch
> 
> [Approval Request Comment]
> User impact if declined: according to comment 8, some Google Apps are failing
> Risk to taking this patch (and alternatives if risky): changes content
> behavior

Mounir - is this a recent regression? If not, I'd prefer to let this ride the train a bit more since this changes content behavior. Approving for Aurora 12 however, because there's enough bake time remaining there.
Comment 17 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-02-14 11:23:07 PST
Not taking this on beta means 6 more weeks of bug 681922.
Comment 18 Mounir Lamouri (:mounir) 2012-02-14 12:11:15 PST
Also, should we take the patch in the ESR given that it seems to break content?
Comment 19 Alex Keybl [:akeybl] 2012-02-14 15:15:54 PST
The regression in bug 681922 doesn't cause huge user pain, has been around since FF7, and most recently was reintroduced by Google backing out their server-side fix. Are there any other motivations for taking this on Beta 11?
Comment 20 Mounir Lamouri (:mounir) 2012-02-14 15:30:24 PST
Push in aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/b1ac78935db9
Comment 21 Jonas Sicking (:sicking) PTO Until July 5th 2012-02-14 19:06:27 PST
Alex: My understanding is that this results in not being able to upload files to google docs, which seems like a pretty bad thing. The bug mostly (only) talks about the high-lighting aspect, but I think the actual problem is bigger than that.
Comment 22 Alex Keybl [:akeybl] 2012-02-16 14:54:05 PST
Comment on attachment 594844 [details] [diff] [review]
fixed patch

[Triage Comment]
Confirmed that this does not affect uploading in Google Docs - only highlighting the menu item. Should be good with getting this fixed for the first time on FF12.
Comment 23 Lukas Blakk [:lsblakk] use ?needinfo 2012-03-20 11:45:39 PDT
[Triage Comment]
Is this fix ready to land on ESR?  If so, please nominate it as per https://wiki.mozilla.org/Release_Management/ESR_Landing_Process
Comment 24 Mounir Lamouri (:mounir) 2012-03-21 03:10:03 PDT
(In reply to Lukas Blakk [:lsblakk] from comment #23)
> [Triage Comment]
> Is this fix ready to land on ESR?  If so, please nominate it as per
> https://wiki.mozilla.org/Release_Management/ESR_Landing_Process

According to comment 22, it seems like we are not going to land this to ESR.
Comment 25 Jonas Sicking (:sicking) PTO Until July 5th 2012-03-21 04:10:35 PDT
So my understanding of the current situation is that google docs is currently working around this bug by using flash for uploads rather than HTML5 features.

It's entirely possible that google docs will drop this workaround once marketshare of versions of firefox with this bug fixed grows large enough, at that point uploads in Firefox-ESR will stop working.

We could of course ask them to keep the workaround which they might do depending on how much they care about ESR support. But it would also be nice to get rid of flash use in such a commonly used website.

So I do think that it'd be nice to take this on ESR assuming we feel confident about the patch.
Comment 26 Mounir Lamouri (:mounir) 2012-03-21 06:09:18 PDT
(In reply to Jonas Sicking (:sicking) from comment #25)
> So I do think that it'd be nice to take this on ESR assuming we feel
> confident about the patch.

FWIW, the patch is a one-liner and is very trivial. I don't think there is any reason to not be confident.
Comment 27 Ioana (away) 2012-04-02 06:50:25 PDT
Verified using the instructions from comment 2 on:
Mozilla/5.0 (Windows NT 6.1; rv:12.0) Gecko/20100101 Firefox/12.0
Mozilla/5.0 (X11; Linux x86_64; rv:12.0) Gecko/20100101 Firefox/12.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:12.0) Gecko/20100101 Firefox/12.0
20120328051619

When clicking on "from click", "from setTimeout" and, on "from mouseup", the pop-up is opened without any issues.

When clicking "from mousedown", the pop-up is opened but the "Firefox prevented this site from opening a pop-up window" notification is also displayed. After allowing notifications and reloading the page, the behavior becomes the same as for the other buttons. 

Mounir, would you prefer I opened a new bug for this issue or can I just reopen this bug?
Comment 28 Mounir Lamouri (:mounir) 2012-04-03 04:35:10 PDT
(In reply to Ioana Budnar [QA] from comment #27)
> Mounir, would you prefer I opened a new bug for this issue or can I just
> reopen this bug?

There is no reason to open a bug for that, it's on purpose.
Comment 29 Ioana (away) 2012-04-03 04:49:14 PDT
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #28)

> 
> There is no reason to open a bug for that, it's on purpose.

Mounir, I'd just like to make sure I got it right. It's on purpose for the "Firefox prevented this site from opening a pop-up window" notification to be displayed although the pop-up has actually been opened? (only when using "mousedown")
Comment 30 Mounir Lamouri (:mounir) 2012-04-03 04:53:40 PDT
(In reply to Ioana Budnar [QA] from comment #29)
> Mounir, I'd just like to make sure I got it right. It's on purpose for the
> "Firefox prevented this site from opening a pop-up window" notification to
> be displayed although the pop-up has actually been opened? (only when using
> "mousedown")

Uh? There is a test in m-c checking exactly that the pop up isn't opened. Indeed, if the behavior you described in comment 29 happens, you should definitely open a bug with a test case and CC me.
Comment 31 Ioana (away) 2012-04-03 05:10:04 PDT
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #30)
> 
> Uh? There is a test in m-c checking exactly that the pop up isn't opened.
> Indeed, if the behavior you described in comment 29 happens, you should
> definitely open a bug with a test case and CC me.

I filed bug 741744 and I used the testcase from this bug in the STR. Already cc'ed you.
Comment 32 Lukas Blakk [:lsblakk] use ?needinfo 2012-04-06 14:53:03 PDT
[Triage Comment]
Removing tracking as per comment 22
Comment 33 Jonas Sicking (:sicking) PTO Until July 5th 2012-04-06 16:08:40 PDT
Lucas, did you see comment 25 and comment 26?
Comment 34 Lukas Blakk [:lsblakk] use ?needinfo 2012-04-09 15:05:52 PDT
[Triage Comment]
Thanks Jonas for pointing that out, please go ahead and nominate for esr landing as per https://wiki.mozilla.org/Release_Management/ESR_Landing_Process
Comment 35 Jonas Sicking (:sicking) PTO Until July 5th 2012-04-09 16:22:35 PDT
Mounir, if this patch applies to the ESR branch, can you:

Set approval-mozilla-esr10 to ? on the patch
Set status-esr10 to checkin-pending on this bug
Comment 36 Mounir Lamouri (:mounir) 2012-04-11 03:10:33 PDT
Created attachment 613936 [details] [diff] [review]
Patch for esr10

I had to do a small update to the test changes.
Comment 37 Mounir Lamouri (:mounir) 2012-04-12 01:48:04 PDT
https://hg.mozilla.org/releases/mozilla-esr10/rev/77de9abd78e6
Comment 38 Mihaela Velimiroviciu (:mihaelav) 2012-04-17 05:21:56 PDT
Mozilla/5.0 (Windows NT 6.1; rv:10.0.4esrpre) Gecko/20120416 Firefox/10.0.4esrpre
Mozilla/5.0 (X11; Linux i686; rv:10.0.4esrpre) Gecko/20120416 Firefox/10.0.4esrpre
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:10.0.4esrpre) Gecko/20120416 Firefox/10.0.4esrpre

Verified using the testcase from comment #2: click, timeout and mouseup handlers open the file picker pop-up, but for mousedown the pop-up is still blocked.

Marking verified for ESR.
Comment 39 Ioana (away) 2012-04-26 06:54:18 PDT
Verified as fixed on:
Mozilla/5.0 (X11; Linux x86_64; rv:13.0) Gecko/20100101 Firefox/13.0
Mozilla/5.0 (Windows NT 6.1; rv:13.0) Gecko/20100101 Firefox/13.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:13.0) Gecko/20100101 Firefox/13.0
BuildID: 20120425123149

Note You need to log in before you can comment on or make changes to this bug.