709448 - File input click() handling should perhaps allow openControlled popups

Status: VERIFIED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

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)]

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) No longer reading bugmail consistently]

Attachment: 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 [:bzbarsky]]

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) No longer reading bugmail consistently]

Mounir, could you take this. It should be a pretty easy change, right?

Comment 5

[Mounir Lamouri (:mounir)]

Attachment: 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 [:bzbarsky]]

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 (Exited; not receiving bugmail, old account, do not use)]

Can we land this?

Comment 8

[Boris Zbarsky [:bzbarsky]]

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)]

Attachment: fixed patch

Comment 10

[Geoff Lankow (:darktrojan)]

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 [:bzbarsky]]

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)]

The same patch should work on aurora and the older patch on beta, but I'll check.

Comment 13

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/6ac07d3e3a61

Comment 14

[Mounir Lamouri (:mounir)]

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)]

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]]

(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 (Exited; not receiving bugmail, old account, do not use)]

Not taking this on beta means 6 more weeks of bug 681922.

Comment 18

[Mounir Lamouri (:mounir)]

Also, should we take the patch in the ESR given that it seems to break content?

Comment 19

[Alex Keybl [:akeybl]]

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)]

Push in aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/b1ac78935db9

Comment 21

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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]]

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]

[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)]

(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) No longer reading bugmail consistently]

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)]

(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)]

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)]

(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)]

(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)]

(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)]

(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]

[Triage Comment]
Removing tracking as per comment 22

Comment 33

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Lucas, did you see comment 25 and comment 26?

Comment 34

[Lukas Blakk [:lsblakk] use ?needinfo]

[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) No longer reading bugmail consistently]

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)]

Attachment: Patch for esr10

I had to do a small update to the test changes.

Comment 37

[Mounir Lamouri (:mounir)]

https://hg.mozilla.org/releases/mozilla-esr10/rev/77de9abd78e6

Comment 38

[Mihaela Velimiroviciu (:mihaelav)]

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)]

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