918780 - Setting the hidden pref "dom.popup_allowed_events" to nothing breaks file upload buttons

Status: VERIFIED FIXED

(Core :: DOM: Events, defect)

Description

[Nicolas Barbulesco]

In Firefox, I have set the hidden pref "dom.popup_allowed_events" to nothing - for sanity. As indicated in the page kb.mozillazine.org/Dom.popup_allowed_events .

Now, the file upload button "Browse..." of forms is broken, with no workaround.

I have a form with an "input" element for choosing a file. I click on the button "Browse..."

Expected behaviour :

I get the dialog for choosing my file. This is a dialog box, this is not a popup window. This is the normal behaviour of a Web browser.

Actual behaviour :

I don't get the dialog for choosing my file. Instead, I get a yellow bar about some "popup window", but this is wrong. Moreover, when I click on "Options", I don't have a way to show the blocked dialog - contrary to true popup windows. I cannot choose my file. I am blocked in my form.

Thank you for fixing that.

Nicolas

Comment 1

[Ioana (away)]

Mozilla/5.0 (X11; Linux x86_64; rv:27.0) Gecko/20100101 Firefox/27.0 (20130924030202)

I get the notification bar about popups being blocked too, but I can re-enable them from the Preferences button from the same bar. I can choose the file and upload it afterwards.

Can you attach the testcase you are using to this bug? Or perhaps provide a link for the page you reproduced the issue on?

Comment 2

[Nicolas Barbulesco]

(In reply to Ioana Budnar, QA [:ioana] from comment #1)

Ioana, compare the menu you get on the yellow bar in the file upload case with the one you get when a real popup window is blocked. In the last case, at the end of the menu you get an item for opening the blocked window. This would be a - poor - workaround, but it is not even available in the file upload case.

> Or perhaps provide a
> link for the page you reproduced the issue on?

All pages with a file upload button. Like this page : 
http://cgi-lib.berkeley.edu/ex/fup.html

Comment 3

[Ioana (away)]

(In reply to Nicolas Barbulesco from comment #2)
> (In reply to Ioana Budnar, QA [:ioana] from comment #1)
> 
> Ioana, compare the menu you get on the yellow bar in the file upload case
> with the one you get when a real popup window is blocked. In the last case,
> at the end of the menu you get an item for opening the blocked window. This
> would be a - poor - workaround, but it is not even available in the file
> upload case.

As I specified in my previous comment, that item (Options/Preferences button) is displayed for me. When trying to reproduce this bug, I get the exact same notification bar the is displayed when a real popup is blocked.

> 
> > Or perhaps provide a
> > link for the page you reproduced the issue on?
> 
> All pages with a file upload button. Like this page : 
> http://cgi-lib.berkeley.edu/ex/fup.html

It works the same for me

Comment 4

[Nicolas Barbulesco]

(In reply to Ioana Budnar, QA [:ioana] from comment #3)

> > Ioana, compare the menu you get on the yellow bar in the file upload case
> > with the one you get when a real popup window is blocked. In the last case,
> > at the end of the menu you get an item for opening the blocked window. This
> > would be a - poor - workaround, but it is not even available in the file
> > upload case.
> 
> As I specified in my previous comment, that item (Options/Preferences
> button) is displayed for me.

The item I speak about is not the button, it is the fourth item in the menu, that you can see - selected - here : 
http://cw.tandf.co.uk/browsesintroductions/help/index_clip_image011.jpg
The menu item starting with "Show".

> When trying to reproduce this bug, I get the
> exact same notification bar the is displayed when a real popup is blocked.

With the same possibilities in the menu ?

 - By the way, a button giving a menu is weird UI, but this is another story. -

Comment 5

[Ioana (away)]

Nicolas, I do see what you are experiencing on
Mozilla/5.0 (X11; Linux i686; rv:27.0) Gecko/20100101 Firefox/27.0

Thanks for all your input :)

Comment 7

[:Gijs (he/him)]

AFAICT having the popup be subject to popup blocking was deliberately implemented in bug 36619 for Firefox 4, because Sicking was afraid it'd be used to hold users hostage otherwise.

I wonder if fixing the code here to be OK with actual user-triggered clicks/keypresses irrespective of the allowed_events value would be enough and/or if that'd trigger anybody's "too hacky for my liking" thresholds.

Jonas, please also see the discussion at https://groups.google.com/forum/?fromgroups=&hl=en#!topic/firefox-dev/jvXY-vydBK0 where this came up before. The idea is that we could implement something like what IE11 provides ('no but seriously no popup windows ever') by having pref UI for emptying out allowed_events - but that currently also prohibits the file inputs from working (without whitelists) which is annoying.

Comment 8

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #7)
> AFAICT having the popup be subject to popup blocking was deliberately
> implemented in bug 36619 for Firefox 4, because Sicking was afraid it'd be
> used to hold users hostage otherwise.
> 
> I wonder if fixing the code here to be OK with actual user-triggered
> clicks/keypresses irrespective of the allowed_events value would be enough
> and/or if that'd trigger anybody's "too hacky for my liking" thresholds.
> 
> Jonas, please also see the discussion at
> https://groups.google.com/forum/?fromgroups=&hl=en#!topic/firefox-dev/jvXY-
> vydBK0 where this came up before. The idea is that we could implement
> something like what IE11 provides ('no but seriously no popup windows ever')
> by having pref UI for emptying out allowed_events - but that currently also
> prohibits the file inputs from working (without whitelists) which is
> annoying.

I forgot to add an actual question, namely: does that sound OK to you? Thoughts on de-popup-blocking this (in part or whole)?

Comment 9

[alex]

How about a first quickfix: Add another keyword to allow xul-popups?

Comment 10

[Cork]

(In reply to Alexander Schier from comment #9)
> How about a first quickfix: Add another keyword to allow xul-popups?

I think i actually prefer this approach if the solution isn't too costly. Meaning just splitting the key into one for content and one for chrome.

Cause that way you could still limit this again if for some reason you needed to handle a evil site for a little while.

Comment 11

[Nicolas Barbulesco]

(In reply to Alexander Schier from comment #9)

> How about a first quickfix: Add another keyword to allow xul-popups?

(In reply to Cork from comment #10)

> Meaning just splitting the key into one for content and one for chrome.

I agree with Cork. The keywords for JavaScript events should not be mixed with one keyword for the file dialog.

Comment 12

[Nicolas Barbulesco]

(In reply to :Gijs Kruitbosch from comment #7)

> Jonas, please also see the discussion at
> https://groups.google.com/forum/?fromgroups=&hl=en#!topic/firefox-dev/jvXY-
> vydBK0 where this came up before. The idea is that we could implement
> something like what IE11 provides ('no but seriously no popup windows ever')
> by having pref UI for emptying out allowed_events - but that currently also
> prohibits the file inputs from working (without whitelists) which is
> annoying.

On IE 11 on Windows, I have no popup windows, but I have the file dialogs. It would be nice to have this behaviour in Firefox.

Comment 13

[marshallx7a+ff]

Apologies for another "me too" comment, but this bug was filed over a year ago and it has been 2 months since the last update, and the issue is assigned to nobody. Is this not getting fixed?

At the moment we have to allow popups in order to allow file uploads. Pretty major flaw in my opinion.
Ok, so we trigger this by telling Firefox we consider all popup windows to be "popups", but quite frankly, why are popups triggered by click et al not treated as popups in a default FF install?
Name one website that uses a click-triggered popup window for anything other than to spam, scam, advertise or otherwise annoy the user?
I am also extremely incredulous about the prospect of a nefarious site using the file upload dialog to cause problems, what exactly is their incentive to use it? In any case, this "security feature" is not effective by default because click-triggered popups are *allowed* by default.

The suggestion of another keyword or setting to represent file uploads as opposed to click events sounds like a great solution, so why has this issue gone quiet?

Comment 14

[Jim Black]

Another annoying consequence of this decision to regard file pickers as popups is that under Firefox's default settings it is impossible for a website to open a file dialog via a keyboard shortcut.  For example, if someone tries to use Alt+A to attach a file in a webmail application, by having the event listener call click() on an <input type="file"> element, the file dialog will be blocked because keyboard events are not among the events which are allowed to create popups.  And because neither an exception is thrown nor a notification displayed, the user will be left in the dark as to why it doesn't work in Firefox and how to fix it.

Comment 16

[Nicolas Barbulesco]

Now, on Firefox 35 Mac, the situation is worse.

Clicking the button "Attach a file" does nothing, the yellow bar does not even come. 
So the user is left in wondering…

Comment 17

[Florens Verschelde :fvsch]

I can confirm on OSX that there is no yellow bar showing (Firefox 43), and no "blocked popup" indicator showing in the address bar either.

Know workarounds:
- re-enable the "click" event in dom.popup_allowed_events (which allows nefarious sites from using click events anywhere on a page to show pop-ups).
- use drag-and-drop to fill the input[type=file] (which is what I tend to do but it’s a pain).

Comment 18

[arni2033]

Third workaround (I thought it's a bug):
3) Click identity block before text in urlbar, then click button ">" in the opened popup,
   then click "More Information". In the opened window switch to tab "Permissions", scroll down,
   uncheck checkbox "Use Default" under "Open Pop-up Windows", then click "Allow" radio button

Comment 19

[Gingerbread Man]

(In reply to arni2033 from comment #18)
> Third workaround

That's the same as adding an entry under about:preferences#content → Exceptions. The “blocked pop-up” location bar icon allowed opening pop-ups on a case-by-case basis, without whitelisting the site in question.

Comment 21

[Kestrel]

(In reply to :Gijs Kruitbosch from comment #7)
> I wonder if fixing the code here to be OK with actual user-triggered
> clicks/keypresses irrespective of the allowed_events value would be enough
> and/or if that'd trigger anybody's "too hacky for my liking" thresholds.

I tried removing the popup blocker from the input element and using IsHandlingUserInput() instead but it does not work when clicking on links that call javascript functions as they are not treated as user input. The popup blocker requires user input for most events but not for mouse/touch/keyboard events which instead need to be trusted. The input element is not suited to identify popup abuse, the popup blocker already works well and just needs to be adjusted to accommodate the picker.

Comment 22

[Kestrel]

Attachment: bug918780.patch - Add new PopupControlState

This patch adds another PopupControlState between openControlled and openAbused for events that are not abused (ie handling user input or trusted event) but still not allowed (according to dom_allowed_events) which can be used by the picker.

User input keydown events now open the picker regardless of dom_allowed_events like Chrome/Edge/IE11. There is a possibility keyboard-only users could get trapped when every keydown shows the picker but it is the same with other browsers and they should be able to figure out how to escape. For example one way is to press and hold Ctrl which causes the picker to appear, keep holding Ctrl and press Alt+F4 to close the picker, then with Ctrl still held press W to close the tab.

The tests for the picker now work without having to disable the popup blocker and without any dom_allowed_events. Extra tests added for link javascript call and keydown event.

Whitelisting decrements the abuse level which doesn't work with the new control state so I had to make an exception for it.

Passed some relevant mochitests locally, will need help with a try push.

Comment 23

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

Comment on attachment 8744682 [details] [diff] [review]
bug918780.patch - Add new PopupControlState

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

I'm not really familiar with this code any more. Trying baku.

If you really can't find anyone else I can try to swap this code back in.

Comment 24

[Andrea Marchesini [:baku]]

Comment on attachment 8744682 [details] [diff] [review]
bug918780.patch - Add new PopupControlState

Smaug, are you familiar with this code?

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8744682 [details] [diff] [review]
bug918780.patch - Add new PopupControlState

openBlocked name is perhaps not the best possible, but can't figure out anything better either.

But yes, this should work. Took a bit time to figure out what the setup is.

Comment 27

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Gentle ping about landing,
as it has r+ from Olli Pettay [:smaug]

Comment 28

[Andrea Marchesini [:baku]]

This patch doesn' apply anymore to m-i. Can you rebase it?

Then, when you receive a positive review, you should push the patch to try/treeherder to see if it breaks existing tests.
If it doesn't, write a comment with the treeherder URL of your push, and mark the bug as checkin-needed.

See: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Getting_the_patch_checked_into_the_tree

Comment 29

[:Gijs (he/him)]

Attachment: Rebased patch

I took the liberty of rebasing this locally. Not sure I got it right, though, as this is hairy code and it was refactored at least once since the patch was written (bug 1321903). Kestrel, could you check?

Meanwhile, as Kestrel asked for trypush help:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1b20b75d0e1ded52fc2073e7ef500929b79d85cb


Kestrel: it might be worth asking for level 1 commit access (ie trypush access), if you haven't already. I would vouch. :-)

Comment 30

[:Gijs (he/him)]

(In reply to :Gijs from comment #29)
> Created attachment 8828006 [details] [diff] [review]
> Rebased patch
> 
> I took the liberty of rebasing this locally. Not sure I got it right,
> though, as this is hairy code and it was refactored at least once since the
> patch was written (bug 1321903). Kestrel, could you check?
> 
> Meanwhile, as Kestrel asked for trypush help:
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=1b20b75d0e1ded52fc2073e7ef500929b79d85cb
> 

This trypush looks /very/ good, at least...

Comment 31

[Jan Tojnar]

Does the patch also fix input[type=color]? Can it be merged?

Comment 32

[:Gijs (he/him)]

Attachment: Patch rebased to recent m-c

Carrying over r+ again.

Comment 33

[:Gijs (he/him)]

Comment on attachment 8891513 [details] [diff] [review]
Patch rebased to recent m-c

(In reply to Jan Tojnar from comment #31)
> Does the patch also fix input[type=color]? Can it be merged?

I have no idea.

Boris, can you doublecheck my rebase of the earlier patch that smaug r+'d, and if it looks good to land to you, set checkin-needed?

Comment 34

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8891513 [details] [diff] [review]
Patch rebased to recent m-c

Why not have smaug do that, since he looked at it already?

Comment 35

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8891513 [details] [diff] [review]
Patch rebased to recent m-c

The merge looks ok to me.

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1dc8ed2fde7
Add new PopupControlState for permitting file/color picker popup regardless of dom_allowed_events. r=smaug

Comment 37

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 38

[tustamido]

Could be uplifted to 56, please?

Comment 39

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

I'm confirming that bug is fixed, starting in Mozilla Firefox Nightly 57.0a1 (2017-08-03), so I'm marking this bug as VERIFIED. Thanks.

Comment 40

[tustamido]

It looks like the fix is incomplete.
Fixed for most cases, but not all. Gmail attach button click is still treated as if it were a popup, so it is blocked.
Firefox 58.0b4

Comment 41

[Boris Zbarsky [:bzbarsky]]

Thank you for reporting that.  I filed bug 1419262 on that issue; gmail is playing setTimeout tricks, not letting you click on an actual file input, which causes some complications.