Closed Bug 709448 Opened 13 years ago Closed 12 years ago

File input click() handling should perhaps allow openControlled popups

Categories

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

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla13
Tracking Status
firefox10 --- wontfix
firefox11 - wontfix
firefox12 + verified
firefox13 + verified
firefox-esr10 12+ verified

People

(Reporter: bzbarsky, Assigned: mounir)

References

Details

(Keywords: testcase, Whiteboard: [qa!])

Attachments

(3 files, 1 obsolete file)

We generally allow popups in mouseup handlers, but we don't allow click() on file inputs to work there.  Is this on purpose?
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.
Attached file 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.
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.
Attachment #580628 - Attachment is patch: false
Attachment #580628 - Attachment mime type: text/plain → text/html
Mounir, could you take this. It should be a pretty easy change, right?
Attached patch Patch v1 (obsolete) — Splinter Review
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?).
Assignee: nobody → mounir
Status: NEW → ASSIGNED
Attachment #583526 - Flags: review?(bzbarsky)
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [needs review]
Version: 9 Branch → Trunk
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.
Attachment #583526 - Flags: review?(bzbarsky) → review+
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....
Attached patch fixed patchSplinter Review
Attachment #583526 - Attachment is obsolete: true
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
Flags: in-testsuite+
Flags: in-litmus-
Target Milestone: --- → mozilla13
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?
The same patch should work on aurora and the older patch on beta, but I'll check.
https://hg.mozilla.org/mozilla-central/rev/6ac07d3e3a61
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [needs review]
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 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
Attachment #594844 - Flags: approval-mozilla-beta?
Attachment #594844 - Flags: approval-mozilla-aurora?
(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.
Attachment #594844 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Not taking this on beta means 6 more weeks of bug 681922.
Blocks: 681922
Also, should we take the patch in the ESR given that it seems to break content?
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?
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 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.
Attachment #594844 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
[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
(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.
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.
(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.
Keywords: testcase
Whiteboard: [qa+]
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?
(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.
(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")
(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.
(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.
[Triage Comment]
Removing tracking as per comment 22
[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
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
Attached patch Patch for esr10Splinter Review
I had to do a small update to the test changes.
Attachment #613936 - Flags: approval-mozilla-esr10?
Attachment #613936 - Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
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.
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
Status: RESOLVED → VERIFIED
Whiteboard: [qa+] → [qa!]
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: