36619 - [HLP][FILE]calling click() on file upload control should bring up file picker

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect, P4)

Description

[rsalesas]

Add the following elements to a form. If you click on the "click" button, it 
should display the browse dialog and populate the hidden file element. This has 
the expected behaviour under Internet Explorer, but fails on Mozilla. 
Furthermore, setting the READONLY attribute in the file element does nothing, 
but properly makes the edit portion readonly in Internet Explorer.

=====

<input type=button value="click" onclick="document.getElementById
('fileObj').click()"><br>
<input type=file value="" style="display:none">

Comment 1

[Doron Rosenberg (IBM)]

are you still seeing this with a recent build?

Comment 2

[Asa Dotzler [:asa]]

setting to new

Comment 3

[rods (gone)]

This bug has been marked "future" because the original netscape engineer working 
on this is over-burdened. If you feel this is an error, that you
or another known resource will be working on this bug,or if it blocks your work 
in some way 
-- please attach your concern to the bug for reconsideration. 

Comment 4

[rsalesas]

This bug results in data loss. I find it hard to believe that this is 
acceptable behaviour in any release. Should such bugs not be given priority? If 
you are doing any kind of dynamic pages, a requirement of a web application, 
this bug will bite.

Comment 5

[rods (gone)]

I don't understand why this causes data loss. My understanding of the issue, is 
that invoking "Click" from script makes the dialog appear. How does making the 
dialog apear cause data loss.

The DOM Level 2 spec: 
http://www.w3.org/TR/2000/CR-DOM-Level-2-20000510/html.html#ID-6043025

Doesn't specifiy that click should work for the input type=file

Comment 6

[rods (gone)]

Also, this appears to only work in IE. Here is my test that didn't work in Nav:
<input type=button value="click" onclick="document.myform.myfile.click()"><br>
<input type=file value="" name=myfile >

Comment 7

[rsalesas]

Generous apologies, it seems I have egg on my face. I pasted my reply a little 
too quickly, this was meant for 36633. This bug indeed does not cause data 
loss, although it is odd that the click is not processed. You are right that 
this works "only in IE". Sigh. I certainly do not want to get into that 
argument.

Comment 8

[bsharma]

Updating QA contact.

Comment 9

[rods (gone)]

Setting to P2 and removing future

Comment 10

[rods (gone)]

futuring for now

Comment 11

[Vladimir Ermakov]

QA Contact Update

Comment 12

[Blair Ireland]

Hello, this feature would allow a work-around for uploading files by way of a 
Macromedia Flash Form & some Framesets & some JavaScript. Obviously this only 
works in Internet Explorer right now, but with the growing popularity of flash 
it would become a definite asset to netscape. Though you are correct to say the 
click() method was not listed in the DOM for the input type="file" element, I 
believe it was merely overlooked and should be revamped as well.

Comment 13

[Jesse Ruderman]

*** Bug 178693 has been marked as a duplicate of this bug. ***

Comment 14

[Boris Zbarsky [:bzbarsky]]

> If you click on the "click" button, it should display the browse dialog and
> populate the hidden file element.

Jesse, dveditz, caillon, is this acceptable security-wise?  My gut instinct is
"absolutely not."

Comment 15

[Jesse Ruderman]

You can already disguise file-upload controls, so I don't see how supporting
click() would hurt security.

Comment 16

[Daniel Veditz [:dveditz]]

Do we really want to be adding more modal popups to our product?

I don't see a security issue per se: the user could still cancel the dialog, and
a filepicker popping up out of nowhere could be made equally obscure with the
current implementation. But putting the creation of the dialog under script
control gives a malicious site one more tool with which to bludgeon the user.

Comment 17

[Christopher Aillon (sabbatical, not receiving bugmail)]

From the "Joe Random teaches typing" website:

  Let's practice typing some computer terminology now.  In the textarea below,
  please type the following paragraph as quickly as possible (NOTE: there is
  no period after the paragraph), and press return when finished:

    I called technical support, and the response I received was helpful.
    After a few minutes, the technician found a solution to my problem
    simply by looking at a file named c:\autoexec.bat


The website could strategically call click() right before the filename is typed,
and then strategically call submit()....

No it is not a direct security hole, but it is an aid to a security hole if the
user is tricked into quickly typing in a filename and closing the dialog (which
could conceivably happen as demonstrated above).  I think this should be marked
wontfix.

Comment 18

[Stephen Deken]

caillon, any situation that shows the file picker to a clueless enough user has
the potential to gather sensitive data.  Typing tests are unnecessary -- just
put the file input element on a page that purports to scan for viruses, and tell
the user to select the target file and press the 'scan' (submit) button.

If you want to take your typing test a bit further, though, don't bother to show
the file picker, just use the file input element and some creative javascript to
hijack the keystrokes you want.  I have a simple page that gets the string
"c:\autoexec.bat" into a file input element without showing the dialog to the
user and without them even needing to know it's there.  I'll attach it.

It's more useful to allow the click method for file inputs than to ban it
altogether.  The HTML DOM does not specifically say that click events are
prohibited for file inputs, only that it must be implemented for button,
checkbox, radio, reset, and submit.  Supporting the click method allows greater
flexibility without sacrificing security.

And anyway, as far as I can tell, it's only a few lines of code added to
content/html/content/src/nsHTMLInputElement.cpp, around line 1610:

      case NS_FORM_INPUT_FILE:
            nsFormEvent event( ... );
            event.originator = this;
            nsEventStatus status = nsEventStatus_eIgnore;
            nsIPresShell *presShell = aPresContext->GetPresShell();
            if (presShell)
            {
                  nsIFormControlFrame* formControlFrame =
GetFormControlFrame(PR_FALSE);
                  if (formControlFrame)
                        rv = formControlFrame->MouseClick( event );
            }
            break;

Comment 19

[Stephen Deken]

Attachment: "Typing Tutor" exploit example

Comment 20

[Jesse Ruderman]

Stephen: your exploit is similar to the one on bug 56236.

Comment 21

[Jon Hall]

This isn't a security issue, anyone wanting to trick a user into uploading a
file isn't going to show the user the file selector in the first place. 

Why this bug should be fixed:

I once built a web app for offsite employee's that had to upload 100's of
pictures a day. Let me tell you, 100 less click's makes a happy auto mechanic
(and  was worth a few free rounds at the bar for me!). None of those guys is
going to risk carpal to switch from IE to Mozilla without click().


Why this bug does not need to be fixed:

This bug is from an older era. The reason I wanted it (and I believe most
developers) was simply to be able to customize the look and feel of file upload
UI. New developments in multiple areas are going to obviate the need for this
moving forward. CSS3, XUL, XAML, and Dashboard are all going to provide better
way's web developers can solve this problem without needing external apps. I
don't see a burning need for this bug anymore and have removed my vote.

If a patch was offered for this bug from Stephen or anyone else, I'd be willing
to compile and test it fwiw.

Comment 22

[Jon Hall]

I'm very sorry for the spam, but I forgot to add this and I think it's important.


WHATWG's Web Forms 2.0 spec is going is directly affect file uploading, and that
would be the ideal place to advocate standardizing click(). Given that the the
new spec is most likely going to end up being supported by Mozilla, any work on
this bug should probably wait to see what changes come from them.

Comment 23

[DX]

If the problem is security, why not ask for confirmation after choose the file
and before uploading it? Simple...

Comment 24

[Phil Ringnalda (:philor)]

*** Bug 272248 has been marked as a duplicate of this bug. ***

Comment 25

[Stephen Deken]

Attachment: Allows click() to launch filepicker

Here's a working patch -- I had to break out an interface for
nsFileControlFrame in order to expose the MouseClick() method.	Not sure about
the nsDOMMouseEvent instantiation, but it gets freed upon return from
MouseClick().  Testcase follows.

Comment 26

[Stephen Deken]

Attachment: Launch filepicker via javascript.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Just from a technical perspective, that patch at least leaks the event object,
and crashes if click() is called on a display:none file control.

Past that, I'm still not convinced how OK this is to do security-wise...

Comment 28

[Stephen Deken]

Attachment: Allows click() to launch filepicker

This is a non-crashing, non-leaking patch.  It duplicates code from
nsFileControlFrame::MouseClick(), but could be consolodated.  Better testcase
forthcoming.

Comment 29

[Stephen Deken]

Attachment: Call click on a visible or display:none input element

Comment 30

[timeless]

i'd like to vote against this (in all forms, including any whatwg actions).

Comment 31

[Stephen Deken]

If the file browser component could be extended to have a forced delay in this
case (similar to how the install boxes work), would that help allay the fears
over this being insecure?

Comment 32

[timeless]

no

Comment 33

[Stephen Deken]

Could someone with actual authority make a call on this bug, please?  If it's a
security issue that is unacceptable, close it as WONTFIX.  If it's an acceptable
risk, we have a valid, working patch that needs review.

Either way, is there any reason this bug isn't moving?

Comment 34

[Peter van der Woude [:Peter6]]

*** Bug 296699 has been marked as a duplicate of this bug. ***

Comment 35

[Stephen Deken]

Comment on attachment 178686 [details] [diff] [review]
Allows click() to launch filepicker

(sheepishly flagging patch for review)

Comment 36

[Boris Zbarsky [:bzbarsky]]

Stephen, you want to put the email address of the person you're requesting
review from in that textfield, not your own email address.  See
http://www.mozilla.org/owners.html for the list of module owners; you probably
need an "addition review" from one of the security module owners too.

Comment 37

[Stephen Deken]

Comment on attachment 178686 [details] [diff] [review]
Allows click() to launch filepicker

Boris, thanks for not making well-deserved fun of me.  Flagged for review by
the DOM owners, addtl review by callion.

Sorry for the spam.

Comment 38

[Elmar Ludwig]

*** Bug 297282 has been marked as a duplicate of this bug. ***

Comment 39

[Johnny Stenback (:jst)]

Comment on attachment 178686 [details] [diff] [review]
Allows click() to launch filepicker

This looks good in general, but we should be dispatching a synthetic click
event at the button (an anonymous child of the <input type=file> element) to
get to the existing code that shows the file picker.

sr- based on that, otherwise I'm in favor of adding this functionality, now
that I finally got around to thinking about it. Sorry for the far too long
delay here.

Comment 40

[Stephen Deken]

Dispatching a synthetic click was the first thing I tried, but I couldn't get it
to work.  Since I submitted my last patch (two days after my son was born), I've
torn down my mozdev environment, so I'll have to build it back up in order to
take another crack at this.  Seems like this line in nsHTMLInputElement.cpp
needs to be changed from:

1260         rv = HandleDOMEvent(context, &event, nsnull, NS_EVENT_FLAG_INIT,
1261                             &status);

To something like this:

if (mType == NS_FORM_INPUT_FILE) {
  nsHTMLButtonElement* btn = magicallyGetReferenceToAnonymousButtonElement();
  if (btn)
    rv = btn->HandleDOMEvent(context, &event, nsnull, NS_EVENT_FLAG_INIT,
                             &status);
}
else {
  rv = HandleDOMEvent(context, &event, nsnull, NS_EVENT_FLAG_INIT,
                      &status);
}

...except I haven't the slightest idea how to get the reference to the
nsHTMLButtonElement.  I suspect it has something to do with GetFormControlFrame
and CallQueryInterface, but the way that jibberjabber works is the equivalent to
deep voodoo for me at this point.  Some more experienced developer could
probably roll their eyes and accidentally sneeze on their keyboard to type it
out, but apparently they all hate this bug.

It probably doesn't matter anyway, since it's extremely unlikely to get into
Firefox 1.5.  It won't be available in Firefox for at least another year, and
probably won't be mainstream for another two years after that while people
upgrade and resist upgrading.  The whole point was to allow javascript to launch
the filepicker so that the ugly-ass widget could be hidden for pages that
actually care about design.  Any benefit derived from this bug will be
completely irrelevant due to advancements in CSS and form processing.

I'm sorry for rambling on, but this bug has been opened for five years and never
fixed because the people who really care about it are designers, and the people
who are loathe it are the people who could fix it in two seconds.  It's ****
like this that turns people away from wanting to develop for Mozilla.  I know it
did for me.

(Well, that and becoming a father and all.)

Comment 41

[Rodrigo Ruiz]

The report talks about two problems. Although people do not agree in the click() part, at least the readonly attribute problem could be fixed. Perhaps it should be separated into another bug report?

My understanding is that the click() feature is only relevant as long as there are problems to correctly apply styles to file fields. Perhaps, the efforts should be directed in that direction (bug 52500), and not this one.

Comment 42

[Boris Zbarsky [:bzbarsky]]

One bug per problem, yes.

Comment 43

[Jesse Ruderman]

There's already a bug report about readonly not working on file upload controls: bug 7258.  It's marked as invalid (see bug 7258 comment 27).

Comment 44

[Rodrigo Ruiz]

Please, could somebody reopen bug 7258?

The HTML specification states that only text and password fields support the readonly attribute. That's OK.

But the reason to not include file fields in this list is that the specification does not state how to implement this field type. If a browser implements it as a simple button, with no associated text field, there is no sense in a readonly attribute.

Common sense should be applied here. If Mozilla implements the file field as an "extended text field" (from a final user point of view), the readonly attribute should be supported.

Comment 45

[Jesse Ruderman]

Rodrigo, I replied in bug 7258.  There's no need to spam this bug with an unrelated comment.

Comment 46

[Sugam Jain]

I'd be very interested in seeing this fixed.  What needs to still be done?  Does the patch need enhancement?

Comment 47

[Boris Zbarsky [:bzbarsky]]

See comment 39.

Comment 48

[Martijn Wargers (dead)]

Attachment: testcase2

Testcase for various form elements, except input type = file.

Comment 49

[Martijn Wargers (dead)]

Attachment: patch

So something like this, I guess.
Except this won't work when <input type="file"> is display:none, since in that case there are no frames to work with.

Comment 50

[Stephen Deken]

Some observations and questions about this patch:

The patch requires that the second child of the primary frame be a button, and doesn't check to make sure that it is.  This would cause a problem if someone ever decided that form control fields should be rendered differently.  That's unlikely to happen since there's a large amount of inertia around the Browse button, but it would be good to check to make sure the event is being dispatched to a place that makes sense.

That aside, the code that actually displays the filepicker is attached to the frame's MouseClick() method (in nsFileControlFrame.cpp), meaning that if there is no frame then there really is no way to display a filepicker.  However, a quick glance seems to indicate that all it really needs from the frame is:

* GetContent(), so that it can call GetDocument() to attach the filepicker to the window of the document the input element is in;

* GetFormProperty(), so that it can prepopulate the filepicker with the current value of the input element; and

* the mTextFrame member, so that it can update the frame with the selected value of the input element (as duplicated in the frame).

So, couldn't we move the event listener somewhere else?  This basically what I was doing with my last patch.  The code duplication could be eliminated by having nsFileControlFrame attach the content as the listener for the click event, something like**:

nsIContent *content = GetContent();
if (content /* && content is a input type=file element */ ) {
  receiver->AddEventListenerByIID(content, NS_GET_IID(nsIDOMMouseListener));
}

**: note that I'm assuming the content can't change without the frame being recreated.  If it can, then this won't work right or at all.

Then we can just chop out the MouseClick method and the nsIDOMMouseListener inheretance from nsFileControlFrame, and the code duplication goes away.  While this isn't dispatching a synthetic click event onto the button, the end result is the same (since a click event on the button calls precisely the same function).

Stenback, your thoughts please?

Comment 51

[Boris Zbarsky [:bzbarsky]]

> This would cause a problem if someone ever decided that form control fields
> should be rendered differently.

Which will almost certainly happen for Gecko 1.9, fwiw.

We should probably just consider moving the click handling, etc, into the content node completely....

Comment 52

[Stephen Deken]

Attachment: moves mouseclick into content node

Ok, here we go -- a non-crashing, non-code duplication patch, one year later.  Happy birthday to my son Ryan.  :)

Comment 53

[Stephen Deken]

I should point out that I haven't tested this patch at all beyond a) making sure it compiles, and b) making sure that the filepicker is launched when Browse... is clicked or when the click() method is called.

In particular, I don't know that the value is ever properly set, whether it submits correctly, whether it works when disabled, etc., but I wanted to post it before I went to work today.  I probably won't have a chance to play with it tonight due to the festivities.

(Sorry for the spam.)

Comment 54

[Stephen Deken]

Comment on attachment 216009 [details] [diff] [review]
moves mouseclick into content node

(egh, the review requests got bungled. sorry again)

Comment 55

[Stephen Deken]

Whoops, it should be SetValueInternal(unicodePath,nsnull) in the MouseClick method, because the click isn't trusted.  Otherwise it works like a champ.

Comment 56

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Do we really want Web pages to be able to throw up dialogs in the user's face?

Comment 57

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

And are there any other potential security issues here if the user doesn't understand what the dialog is?

I tend to think we shouldn't take this.

Comment 58

[Martijn Wargers (dead)]

IE6 allows this thing for years, and I haven't heard of any security issues involved with it.
Note that in Mozilla you can hide the file control with opacity:0, so you already could 'devise' an evil webpage, where with one click a filepicker pops up.

Comment 59

[Stephen Deken]

dbaron, as to your first concern, we already allow webpages to display modal informational messages with alert(), modal ok/cancel dialogs with confirm(), and under some circumstances to pop up new modeless windows with open().  I submit that there are ways to annoy a user already, and popping up a file picker dialog is no worse than any of the existing methods (which are also not seeing widespread abuse).

As to any potential security issue, I think that even the most naïve computer user knows what a file picker is and what it does.  The only thing they may not understand is that the file they select in that dialog will be transmitted to the server.  If we're really concerned about that, we ought to put a confirmation message during the submit process that informs the user:

  +--------------------------------------
  | Confirm File Transfer
  |
  | You are submitting a form which contains
  | the following files:
  |
  |    C:\WINDOWS\system32\passwords.ini
  |    C:\My Pictures\Me_Naked_And_Tied_Up.jpg
  |
  | The contents of these files will be shown
  | to the owner of www.hackz.co.tv.
  |
  | [ ] Don't show this dialog again.
  |
  |     [ Transfer Files ] [ Cancel Form ]
  +---------------------------------------------

And this confirmation would always be displayed, whether click() was called or not.

And finally, the response of a user who is presented with something they don't understand is to find and press the 'cancel' button, or to hit 'escape'.  With the patches already in trunk that don't allow text to be typed into the file input control, there is really very little risk here.

Comment 60

[Boris Zbarsky [:bzbarsky]]

> And finally, the response of a user who is presented with something they don't
> understand is to find and press the 'cancel' button

Actually, from what I understand from the folks who've done usability studies the typical response of an average user is to press a random button.  They don't bother looking for a Cancel button, and they definitely don't hit 'escape' -- they just click frantically until the thing they don't understand goes away.  Note that this is not in any way an expression of support or non-support for the patch; just a correction of a misperception on user behavior.

Comment 61

[Martijn Wargers (dead)]

(In reply to comment #60)
> Actually, from what I understand from the folks who've done usability studies
> the typical response of an average user is to press a random button.  They
In that case, we're still safe, because the 'Open' doesn't do anything unless a file has been selected. And that's not something they're going to do, they want the dialog to go away, so they press the 'Cancel' button next.

Comment 62

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Why should the frame be registering the content as an event listener?  Shouldn't the content node register itself?

(I'm still not a fan of the whole idea, though.)

Comment 63

[Stephen Deken]

The frame needs to register the content as a listener because the frame creates and retains references to the anonymous content (browse button and textbox).  There is no way (so far as I can tell) for the 'real' content (the input type="file" element) to get a reference to the anonymous content (the browse button and the textbox).  If it could, it would be easier just to dispatch a synthetic click on the browse button in the first place.

Comment 64

[Stephen Deken]

Attachment: Updated version of 216009 to apply cleanly (also with setValueInternal)

Here's an updated version, just so it applies cleanly to trunk.

Comment 65

[Ryan Flint [:rflint] (ping via IRC for reviews)]

*** Bug 339187 has been marked as a duplicate of this bug. ***

Comment 66

[Kevin Kragenbrink]

Is any progress being made on this? Has anyone made a final decision?  I'm avidly waiting for this bug to be resolved, as a number of desktop-level (application-replacement) pages I want to write will rely on using this, rather than a cludgy manually built interface.  I was utterly shocked to find out Mozilla did not support this, and for a brief instant contemplated actually forcing my users to use IE for my applications.  Perish the thought.

Please put this in?

Comment 67

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 217676 [details] [diff] [review]
Updated version of 216009 to apply cleanly (also with setValueInternal)

I suppose I'm ok with this, since there are other hacky ways to force the file picker to come up.  I'm not really the best reviewer for this code, but here are my comments:


>-    if (shell) {
>+    if (shell)
>+    {
>       nsCOMPtr<nsPresContext> context = shell->GetPresContext();
> 
>-      if (context) {
>+      if (context)
>+      {

Please leave these as they were.

>+        if (mType == NS_FORM_INPUT_FILE)
>+        {
>+          nsDOMMouseEvent *domMouseEvent = new nsDOMMouseEvent( context, &event );
>+          MouseClick( (nsIDOMMouseEvent*)domMouseEvent );
>+        }
>+        else
>+        {
>+          nsEventDispatcher::Dispatch(NS_STATIC_CAST(nsIContent*, this), context,
>+                                      &event, nsnull, &status);
>+        }

Why should this not work by dispatching a DOM event?  Does it not dispatch a DOM event in IE?

>+nsresult 

You need NS_IMETHODIMP, not nsresult

>+nsHTMLInputElement::MouseClick(nsIDOMEvent* aMouseEvent)
>+{

If this is intended only to be called for file inputs, it should at the very least check that in an assertion, perhaps even at runtime for all builds.

>Index: layout/forms/nsFileControlFrame.cpp
>+  nsCOMPtr<nsIDOMMouseListener> mouseListener = do_QueryInterface(fileContent);

This added variable is unused.



Looking at the changes you made to the function you moved:

-  GetFormProperty(nsHTMLAtoms::value, defaultName);
+  NS_CONST_CAST(nsHTMLInputElement*, this)->GetValue(defaultName);

You certainly don't need the NS_CONST_CAST.  At it looks like the old code here was equivalent to GetFileName, not GetValue.  GetValue should be equivalent for file controls, but GetFileName is simpler.

Comment 68

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Stephen, are you still willing to update the patch?

Comment 69

[Stephen Deken]

I'm willing to update it, but it may have to wait a few weeks.  I tore down my build environment back in July before my daughter was born.

I'll get a new patch out as soon as I can.

Comment 71

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 216009 [details] [diff] [review]
moves mouseclick into content node

Marking to reflect earlier comments.

Comment 72

[Jesse Costello-Good]

Any chance this can still make it into Firefox 3?

Comment 73

[Ken Tozier]

Enabling programatic invocation of the click() command on <input type='file'> nodes would be extremely useful for in-browser Javascript applications. 

I can't see that there is any harm in popping up a file chooser dialog. The user can only select files already on their system and they have to manually click the dialog's OK button. No one is suggesting that Javascripts be given access to the file system only that scripters be allowed to summon a dialog.

Please implement this functionality. This bug has been around for more than eight years and it's long past time to put it to bed.

Comment 74

[Tobias Buschor]

I vote for this Future!

Comment 75

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Jonas, Zack said you had some security-related comments here.

Comment 76

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Jonas, please respond!!!

Comment 77

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

My concern is that you can spam the user and basically force him/her to attach a file, or even a file which contents is to your liking:

myFileControl = doc.getElementById('file');
do {
  myFileControl.click();
} while (!fileContainsDirectionsToSecretVolcanoLair(myFileControl.files[0]));

If we added some code to detect spamming and made .click() a no-op if we think we're being spammed, then that's ok with me. The only concern there would be that we have to be very sure that we're not blocking proper use of the API. So we couldn't simply impose a 5 second limit for example, since then if the user clicked on the button in comment 0, and accidentally dismissed the dialog, clicking again within 5 seconds would appear to not work.

Maybe subjecting the API to popup-blocker logic, and ensured that you can only open one dialog per popup-whitelisted-codesection, would work?

Comment 78

[lucas rancez]

Ok. Here is my situation.
In my website I load part of the content from templates. This templates are as simple as posible and they are made by designers.
Now they need to start uploading images from the rendered templates. The image will be saved in a webserver for http access. Ofcourse that in my "file" textbox I´ll need to show the relative virtual http path uploaded.
The nicest way to do this will be that in the template only goes a writeable textbox for the url and a simple button that will call my javascript. In that javascript I need to show the filepicker, upload the file and update the textbox.
The dirty way will be force the designers to learn javascript and ajax so they can implement everything in the templates...... and I'll need to start looking for another job...
So, this realy easy and works great in IE.... please... add the click event!
OH MY GOD!!!!!! This BUG its almost NINE years old!!!!!!

PD: sorry, my english is not that good....

Comment 79

[Marc Lavergne]

I'd like to add my vote to have this addressed since virtually every other browser supports this and the CSS overlay workarounds used to get around this (http://www.quirksmode.org/dom/inputfile.html) are inefficient.

Comment 80

[Jeff Schiller]

I don't think votes really matter here, but I'd like to sum up my thoughts on this matter, reiterating most of what is said in above comments:

1) evil web pages can already trick users into bringing up a File
Picker (see http://www.quirksmode.org/dom/inputfile.html )

2) the user must click a file, then Ok for anything to happen

3) it's already (apparently) possible in IE 

We want the Open Web stack to get web app-friendlier.  Right now I have to disguise the file input control just to get a standard application 'file open' button.  This works great except for the mouse cursor looking a little funny (my original reason for Bug 538761 - feel free to dupe).

It is really easy to bring up a File Chooser dialog in desktop app frameworks.  It is equally possible for a desktop app to send any file over the internet to some evil server (once the user has clicked 'allow access to the internet', which most would do).

There is the argument that web pages are easier for people to stumble upon compared to users having to install desktop apps, but #2 still applies above.

For the Typing Tutor problem, wouldn't there be a way to not allow focus to the filename edit box (I don't even have one on OSX)?

Comment 81

[Jesse Ruderman]

Adding this API won't make "typing tutor" exploits easier, since a web page can already force a file picker to appear by getting you to press spacebar while a file upload form control has focus.

Jonas's concern in comment 77, where an attacker spams you with a modal security dialog until you give him what he wants, isn't specific to file pickers.  It's worse for many other security dialogs, such as in bug 331334.  We should limit all such dialogs to "one per click if the user hits cancel".  But that might be hard and should happen in another bug.  For this dialog, for now, normal popup blocking logic is fine.

Let's add this feature.

Comment 82

[Jeff Schiller]

I'm guessing this 4-year old patch doesn't still apply cleanly :)

Stephen?

Comment 83

[Stephen Deken]

I'd be happy to clean up the patch if the drivers will accept it -- shouldn't take very long to do.

Comment 85

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

Yes, if a patch was provided it would be accepted.

Comment 88

[Matt Fetig]

This is ridiculous that this has been listed as a bug for 10 years. I believe that the majority of people now want this to be able to style a field.  As people have mentioned the quirksmode workaround is ineffective and actually causes more usability problems because of the inability to navigate using the keyboard and even when you have focus on what appears to be the browse button, you really have focus on a button underneath the invisible real browse button.  Perhaps the real problem is that a new file upload spec needs to be added to HTML5 in order to allow a more seamless upload style that is less invasive(non-modal) and not require a plug-in.  Dojo actually has a file upload with 2 implementations, 1 with flash and 1 with the overlay workaround.  I suggest 1 of 3 solutions; allow click(), enable styling and multiple upload in the current implementation of input:file or a new spec should be added to HTML5 to allow for styling, multi-file uploads and possibly embedded file selection.  Can we please see a little action on this bug?

Comment 89

[Boris Zbarsky [:bzbarsky]]

> enable styling and multiple upload in the current implementation of input:file

We've supported the latter since Firefox 3.6 shipped.  Styling is doable, but the problem is that most of what you can style is the box that contains the textfield and button; there's no good solution for this, unfortunately.

Comment 90

[Matt Fetig]

(In reply to comment #89)
> Styling is doable, but the problem is that most of what you can style is the
> box that contains the textfield and button; there's no good solution
> for this, unfortunately.

This is why click() needs to be supported so that you can style the button and the textbox independently and just make the field input hidden.  Alternatively, what if the textfield and button were decoupled.  The textfield's purpose is not a functional one.  It is only to show the user what file they selected(which is useless in most cases because the path is too long for the box.  I know this isn't a solution that can be implemented right now, but just an idea on how the field could be styled without allowing click().  I don't believe allowing click() would cause any security problems that aren't already there(namely the opacity:0 workaround).  Any usability concerns that were raised in the comments back in 2004 about an unexpected dialog showing up are moot points due to the ability to pop up a modal dialog anyway using alert().  Although, on a similar topic, I do believe that is an issue that needs to be solved.  If someone where to write while(1)alert(); You would have to close down the entire browser because the modality doesn't allow you to close the tab.

Comment 91

[Christopher Blizzard (:blizzard)]

Jonas has told me that we'll be adding .click() for Firefox 4.

Comment 92

[Mike Beltzner [:beltzner, not reading bugmail]]

Moving to betaN - requires beta coverage, no specific beta milestone targetted at this time.

Comment 93

[Biju]

(In reply to comment #91)
> Jonas has told me that we'll be adding .click() for Firefox 4.

Then what are we planning to avoid SPAM?

If we implement .click() feature, I wish we only allow to call it only from another onMouseLeftClick() event. As well as only one time it is allowed, like the way do for window.open()

Jonas, instead of 5sec delay (in Comment 77), a 3sec(about:config) delay is OK
Also we could add a 1 or 2 sec delay after .click() called in javascript
before actually showing the file open dialog.

Comment 94

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

David: Would you be interested in taking this bug?

What I think we should do for now is to check with the popup blocking API to see if popups are allowed and if so display the dialog.

You should definitely start with the patches in the bug and go from there.

Comment 95

[David Zbarsky (:dzbarsky)]

Do we want this for Firefox 4?

Comment 96

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

Yes, if possible :)

Comment 97

[Johnny Stenback (:jst)]

I don't think we should treat the native file picker as a popup at all, we should treat it like we will be treating modal dialogs, per bug 61098. David, I have a patch in my patch queue that's up to current trunk, I'll attach that to the bug so that you can play around with that in this case as well.

Comment 98

[David Zbarsky (:dzbarsky)]

Attachment: Patch

Comment 99

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

Comment on attachment 462659 [details] [diff] [review]
Patch

This only works if the <input> has a frame. I think we want .click() to work even on display:none <input>s.

One solution is to move the filepicker code from the frame to the element. And have the frame call some function on the element when the user clicks the frame.

Comment 100

[David Zbarsky (:dzbarsky)]

Attachment: Patch v2

I moved the code to the input element and verified that it works for display:none inputs.

Comment 101

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

Comment on attachment 464915 [details] [diff] [review]
Patch v2

>+class AsyncClickHandler : public nsRunnable {
>+public:
>+  AsyncClickHandler(nsHTMLInputElement* aInput, PRBool aCheckPopupPermission)
>+   : mCheckPopupPermission(aCheckPopupPermission),
>+     mInput(aInput) { };
>+
>+  NS_IMETHOD Run();
>+
>+protected:
>+  nsHTMLInputElement* mInput;
>+  PRBool mCheckPopupPermission;
>+};

You should hold a strong reference to mInput here to avoid the element getting deleted while you're waiting for Run() to get called. So use nsRefPtr<nsHTMLInputElement> mInput.

>+NS_IMETHODIMP
>+AsyncClickHandler::Run()
>+{
>+  nsresult rv;
>+
>+  // Get parent nsIDOMWindowInternal object.
>+  nsCOMPtr<nsIDocument> doc = mInput->GetDocument();
>+  if (!doc)
>+    return NS_ERROR_FAILURE;
>+
>+  // Check if page is allowed to open the popup
>+  if (aCheckPopupPermission)
>+  {

Shouldn't this be mCheckPopupPermission? Also, please use |if (...) {| as whitespace style as that is the prevalent style in this file.

>+    nsCOMPtr<nsIPopupWindowManager> pm =
>+      do_GetService(NS_POPUPWINDOWMANAGER_CONTRACTID);
>+ 
>+    if (!pm) {
>+      return NS_OK;
>+    }
>+
>+    PRUint32 permission;
>+    pm->TestPermission(doc->GetDocumentURI(), &permission);
>+    if (permission == nsIPopupWindowManager::DENY_POPUP)
>+      return NS_OK;
>+  }

I had to ask help from jst here, but it appears this is not the right API to use here. This will only check if popups are explicitly enabled for the given site. I.e. if the user has clicked the "always allow popups from this site". It doesn't take into account weather we're currently firing click events and such things.

So what you want to do is get the window and call GetPopupControlState. Something like

nsCOMPtr<nsPIDOMWindow> window = do_QI(mInput->GetOwnerDoc()->GetScriptGlobalObject());
window->GetPopupControlState();

With appropriate null-checks of course.

>+nsHTMLInputElement::InitUploadLastDir() {
>+  gUploadLastDir = new UploadLastDir();
>+  NS_IF_ADDREF(gUploadLastDir);

new never fails, so use NS_ADDREF

>+nsHTMLInputElement::DestroyUploadLastDir() {
>+  if (gUploadLastDir)
>+    NS_RELEASE(gUploadLastDir);
>+}

However here you can simply use NS_IF_RELEASE() which has the |if| built-in :)

>+nsHTMLInputElement::FileInputClick(PRBool aCheckPopupPermission)
>+{
>+  nsCOMPtr<nsIRunnable> event = new AsyncClickHandler(this, aCheckPopupPermission);
>+  return NS_DispatchToMainThread(event);
>+}

Call this function |FireAsyncClickHandler| or some such. It's generally better to name functions for what they do, than when they are called (callbacks being a notable exception of course).


> nsFileControlFrame::BrowseMouseListener::MouseClick(nsIDOMEvent* aMouseEvent)
...
>+  nsHTMLInputElement* input = static_cast<nsHTMLInputElement*>(mFrame->GetContent());
>+  NS_ASSERTION(input, "Content of FileControlFrame is not input element?");
>+  return input->FileInputClick(PR_FALSE);

The assertion here doesn't actually check that you have an input element. Even if the element isn't an input element the static_cast will happily cast it.

Instead check that IsHTML() returns true and that Tag() return nsGkAtoms::input. Only then do the static_cast. We probably always want to do that, not just in debug builds.

r- due to the popup blocker issue. But with these things fixed it looks good to me!

Yay! Almost there!

Comment 102

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

David: jst tells me that you shouldn't use nsIDocument->GetScriptGlobalObject(), you should instead use nsIDocument->GetWindow(). That also means that you won't have to QI. So it'll be something as simple as:

mInput->GetOwnerDoc()->GetWindow()->GetPopupControlState()

(again, with appropriate null-checks)

Comment 103

[David Zbarsky (:dzbarsky)]

Attachment: Patch v2.1

I took out mCheckPopupPermission because nsPIDOMWindow::GetPopupControlState ends up doing the same thing

Comment 104

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

Comment on attachment 468159 [details] [diff] [review]
Patch v2.1

Yay!!

Comment 105

[David Zbarsky (:dzbarsky)]

Attachment: Display notification when file picker is blocked

Comment 106

[David Zbarsky (:dzbarsky)]

Attachment: Patch v2.1 refreshed

Comment 107

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

Comment on attachment 468739 [details] [diff] [review]
Patch v2.1 refreshed

Assuming this is just updating to tip, r=me

Comment 108

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

Comment on attachment 468736 [details] [diff] [review]
Display notification when file picker is blocked

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js
...
>       for (var i = 0; i < pageReport.length; ++i) {
>+
>+        // popupWindowURI will be null if the file picker popup is blocked.
>+        // xxxdz this should make the option say "Show file picker" and do it

No need for the empty line. Please file a bug on the xxx comment and refer to that bug here.

>+        if (!pageReport[i].popupWindowURI)
>+        {

Use |if (...) {| as that is the style used in this file.

>+          blockedPopupAllowSite.setAttribute("hidden", "true");

Why are you doing this? It seems like we *really* want this option to be available so that the user can whitelist the whole site to allow the filepicker to be shown, as a workaround for the "Show file picker" option not being available.

>diff --git a/content/html/content/src/nsHTMLInputElement.cpp b/content/html/content/src/nsHTMLInputElement.cpp
...
>     if (permission == nsIPopupWindowManager::DENY_POPUP)
>+    {

Use |if (...) {| as that is the style used in this file. (It's the style used in almost all of our codebase).

Looks great otherwise. r=me with those fixes. Please include a checkin comment when you're attaching the final patch to make it easier for the person pushing the patch.

Comment 109

[David Zbarsky (:dzbarsky)]

Attachment: To checking part 1 r=sicking

Comment 110

[David Zbarsky (:dzbarsky)]

Attachment: To checkin part 2 r=sicking

Comment 111

[David Zbarsky (:dzbarsky)]

Filed Bug 590306.

> >+          blockedPopupAllowSite.setAttribute("hidden", "true");
> 
> Why are you doing this? It seems like we *really* want this option to be
> available so that the user can whitelist the whole site to allow the filepicker
> to be shown, as a workaround for the "Show file picker" option not being
> available.

blockedPopupAllowSite is actually the option that allows you to open the popup anyways.  The option to edit prefs is still there.

Comment 112

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

> > Why are you doing this? It seems like we *really* want this option to be
> > available so that the user can whitelist the whole site to allow the
> > filepicker to be shown, as a workaround for the "Show file picker" option not
> > being available.
> 
> blockedPopupAllowSite is actually the option that allows you to open the popup
> anyways.  The option to edit prefs is still there.

If that's the case, why doesn't the |if| statement below your code do the same thing?

Comment 113

[David Zbarsky (:dzbarsky)]

Attachment: test

Comment 114

[David Zbarsky (:dzbarsky)]

Attachment: To checkin part 2 r=sicking

Comment 115

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

Comment on attachment 468856 [details] [diff] [review]
To checkin part 2 r=sicking

>-static
>-void FirePopupBlockedEvent(nsIDOMDocument* aDoc,
>-                           nsIDOMWindow *aRequestingWindow, nsIURI *aPopupURI,
>-                           const nsAString &aPopupWindowName,
>-                           const nsAString &aPopupWindowFeatures)
>+void 
>+nsGlobalWindow::FirePopupBlockedEvent(nsIDOMDocument* aDoc,
>+                                      nsIDOMWindow *aRequestingWindow, nsIURI *aPopupURI,
>+                                      const nsAString &aPopupWindowName,
>+                                      const nsAString &aPopupWindowFeatures)

Please add a
/* static */
comment on the line before the 'void'. We do this as documentation that the function is static in a lot of places.

Comment 116

[David Zbarsky (:dzbarsky)]

Attachment: Test

Comment 117

[Mounir Lamouri (:mounir)]

Comment on attachment 469205 [details] [diff] [review]
Test

>+function checkFirst()
>+{
>+  netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect');
>+  ok(shown,
>+     "File picker show method should have been called");
>+  pbi.setBoolPref("dom.disable_open_during_load", true);
>+  shown = false;
>+  document.getElementById("a").click();
>+  setTimeout(finishTest, 1000);
>+}

That means there is no test checking that the filepicker is shown when the popup blocker is enabled?

>diff --git a/layout/forms/test/test_bug377624.html b/layout/forms/test/test_bug377624.html
>--- a/layout/forms/test/test_bug377624.html
>+++ b/layout/forms/test/test_bug377624.html
>@@ -87,23 +87,25 @@ FilePickerService.prototype = {
>   // methods
>   appendFilter: function(val)
>   {
>     this._obs.notifyObservers(null, "TEST_FILEPICKER_APPENDFILTER", val);
>   },
> 
>   appendFilters: function(val)
>   {
>+    netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect');
>     this._obs.notifyObservers(null, "TEST_FILEPICKER_APPENDFILTERS", val);
>   },
> 
>   init: function() {},
> 
>   show: function()
>   {
>+    netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect');
>     this._obs.notifyObservers(null, "TEST_FILEPICKER_SHOW", this.filterIndex);
>     return this.returnOK;
>   }
> };

Why are these changes needed?

Comment 118

[David Zbarsky (:dzbarsky)]

Attachment: To checkin part 2 r=sicking

With the /* static */

Comment 119

[David Zbarsky (:dzbarsky)]

(In reply to comment #117)
> That means there is no test checking that the filepicker is shown when the
> popup blocker is enabled?

Ah. Good point.

> Why are these changes needed?

I'm not sure why, but with this patch these methods were complaining about permission being denied.

Comment 120

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

> That means there is no test checking that the filepicker is shown when the
> popup blocker is enabled?

Yeah, I was a bit concerned about this too.

David, it'd probably be worth enabling the popup blocker during the whole test, and synthesizing a click for when you want to check that the popup is shown.

Comment 121

[David Zbarsky (:dzbarsky)]

Attachment: Test modifications and new test

Comment 122

[Mounir Lamouri (:mounir)]

Comment on attachment 469301 [details] [diff] [review]
Test modifications and new test

>diff --git a/layout/forms/test/bug536567_subframe.html b/layout/forms/test/bug536567_subframe.html
>--- a/layout/forms/test/bug536567_subframe.html
>+++ b/layout/forms/test/bug536567_subframe.html
>@@ -62,17 +64,17 @@ function test() {
> 
>   mockFilePickerRegisterer.register();
>   try {
>     wu.sendMouseEvent("mousedown", rect.left + 5, rect.top + 5, 0, 1, 0);
>     wu.sendMouseEvent("mouseup", rect.left + 5, rect.top + 5, 0, 1, 0);
>   } catch (e) {
>     Cu.reportError(e);
>   } finally {
>-    mockFilePickerRegisterer.unregister();
>+    setTimeout(mockFilePickerRegisterer.unregister, 100);

Can't you find a way to do that without the setTimeout?

>diff --git a/layout/forms/test/test_bug36619.html b/layout/forms/test/test_bug36619.html
>+var mockFilePickerRegisterer =
>+  new MockObjectRegisterer("@mozilla.org/filepicker;1",MockFilePicker);
>+
>+mockFilePickerRegisterer.register();
>+
>+// enable popups the first time
>+var pbi = Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefBranch2);
>+var oldAllow = pbi.getBoolPref("dom.disable_open_during_load");
>+var oldShowBar = pbi.getBoolPref("privacy.popups.showBrowserMessage");
>+pbi.setBoolPref("dom.disable_open_during_load", false);
>+pbi.setBoolPref("privacy.popups.showBrowserMessage", false);

You should not disable the popup blocker
Use .click() when the popup blocker should block the file picker and synthesizeMouse when it shouldn't.

>+var shown = false;
>+
>+function checkFirst()
>+{
>+  netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect');
>+  ok(shown,
>+     "File picker show method should have been called");
>+  pbi.setBoolPref("dom.disable_open_during_load", true);
>+  shown = false;
>+  document.getElementById("a").click();
>+  setTimeout(finishTest, 1000);

Don't use setTimeout but a click event listener so it will prevent random orange.

Comment 123

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

> >+function checkFirst()
> >+{
> >+  netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect');
> >+  ok(shown,
> >+     "File picker show method should have been called");
> >+  pbi.setBoolPref("dom.disable_open_during_load", true);
> >+  shown = false;
> >+  document.getElementById("a").click();
> >+  setTimeout(finishTest, 1000);
> 
> Don't use setTimeout but a click event listener so it will prevent random
> orange.

That won't work since the click event is dispatched synchronously, but the file dialog is opened asynchronously.

Comment 124

[Mounir Lamouri (:mounir)]

(In reply to comment #123)
> > >+function checkFirst()
> > >+{
> > >+  netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect');
> > >+  ok(shown,
> > >+     "File picker show method should have been called");
> > >+  pbi.setBoolPref("dom.disable_open_during_load", true);
> > >+  shown = false;
> > >+  document.getElementById("a").click();
> > >+  setTimeout(finishTest, 1000);
> > 
> > Don't use setTimeout but a click event listener so it will prevent random
> > orange.
> 
> That won't work since the click event is dispatched synchronously, but the file
> dialog is opened asynchronously.

Then, at least, when the file picker is expected to be shown, a method from "show" should be called instead of setTimeout then looking at the boolean.

Comment 125

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

You can probably simply use executeSoon there.

Comment 126

[Mounir Lamouri (:mounir)]

Maybe be better to re-set checkin-needed keyword when the tests will be ready.

Comment 127

[David Zbarsky (:dzbarsky)]

They are, I just want to run it through try one more time to make sure (there were about 100 tests it didn't run last time due to timeout issues)

Comment 128

[David Zbarsky (:dzbarsky)]

Attachment: Test modifications and new test

Comment 129

[Mounir Lamouri (:mounir)]

Comment on attachment 469617 [details] [diff] [review]
Test modifications and new test

>diff --git a/content/html/content/test/test_bug500885.html b/content/html/content/test/test_bug500885.html
>--- a/content/html/content/test/test_bug500885.html
>+++ b/content/html/content/test/test_bug500885.html
>@@ -47,26 +47,26 @@ MockFilePicker.prototype = {
>   get files() {
>     throw Cr.NS_ERROR_NOT_IMPLEMENTED;
>   },
>   show: function MFP_show() {
>     return Ci.nsIFilePicker.returnOK;
>   }
> };
> 
>-var mockFilePickerRegisterer =
>+var MockFilePickerRegisterer = 

Why are you renaming this variable?

>+// enable popups the first time
>+var pbi = Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefBranch2);
>+var oldAllow = pbi.getBoolPref("dom.disable_open_during_load");
>+var oldShowBar = pbi.getBoolPref("privacy.popups.showBrowserMessage");
>+pbi.setBoolPref("dom.disable_open_during_load", false);
>+pbi.setBoolPref("privacy.popups.showBrowserMessage", false);

You should not disable the popup blocker.

Comment 130

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

Comment on attachment 469617 [details] [diff] [review]
Test modifications and new test

># HG changeset patch
># Parent 01781995b79366e655078db9b1c7d7bae6215318
># User David Zbarsky <[e-mail]>
>Bug 36619 - Tests
>
>diff --git a/content/html/content/test/test_bug500885.html b/content/html/content/test/test_bug500885.html
>--- a/content/html/content/test/test_bug500885.html
>+++ b/content/html/content/test/test_bug500885.html
>@@ -47,26 +47,26 @@ MockFilePicker.prototype = {
>   get files() {
>     throw Cr.NS_ERROR_NOT_IMPLEMENTED;
>   },
>   show: function MFP_show() {
>     return Ci.nsIFilePicker.returnOK;
>   }
> };
> 
>-var mockFilePickerRegisterer =
>+var MockFilePickerRegisterer = 

Why this name change? We generally use fooBar as naming convention for instances and FooBar for classes.

>diff --git a/layout/forms/test/bug536567_subframe.html b/layout/forms/test/bug536567_subframe.html
...
>@@ -62,17 +64,17 @@ function test() {
> 
>   mockFilePickerRegisterer.register();
>   try {
>     wu.sendMouseEvent("mousedown", rect.left + 5, rect.top + 5, 0, 1, 0);
>     wu.sendMouseEvent("mouseup", rect.left + 5, rect.top + 5, 0, 1, 0);
>   } catch (e) {
>     Cu.reportError(e);
>   } finally {
>-    mockFilePickerRegisterer.unregister();
>+    setTimeout(mockFilePickerRegisterer.unregister, 100);
>   }
> }

In this file it might make more sense to move the unregister call to the show() method implementation. (In the other test it makes more sense to keep the executeSoon though).

Alternatively change this to use executeSoon. I'm fine either way.

>diff --git a/layout/forms/test/test_bug36619.html b/layout/forms/test/test_bug36619.html
...
>+MockFilePicker.prototype = {
>+  QueryInterface: XPCOMUtils.generateQI([Ci.nsIFilePicker]),
>+
>+  // constants
>+  modeOpen: 0,
>+  modeSave: 1,
>+  modeGetFolder: 2,
>+  modeOpenMultiple: 3,
>+  returnOK: 0,
>+  returnCancel: 1,
>+  returnReplace: 2,
>+  filterAll: 1,
>+  filterHTML: 2,
>+  filterText: 4,
>+  filterImages: 8,
>+  filterXML: 16,
>+  filterXUL: 32,
>+  filterApps: 64,
>+  filterAllowURLs: 128,
>+  filterAudio: 256,
>+  filterVideo: 512,

You can probably remove the constants from here.

>+// enable popups the first time
>+var pbi = Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefBranch2);
>+var oldAllow = pbi.getBoolPref("dom.disable_open_during_load");
>+var oldShowBar = pbi.getBoolPref("privacy.popups.showBrowserMessage");
>+pbi.setBoolPref("dom.disable_open_during_load", false);
>+pbi.setBoolPref("privacy.popups.showBrowserMessage", false);
...
>+function runTests()
>+{
>+  document.getElementById("a").click();
>+  SimpleTest.executeSoon(checkFirst);
>+}

It'd be better to test that things work even with the popup blocker enabled. You can do this by changing runTests to do something like this:

function runTests() {
  var dummy = $("dummy");
  cont.addEventListener("click", function() {
    document.getElementById("a").click();
  }, false);
  netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
  var wu = window.QueryInterface(Ci.nsIInterfaceRequestor)
                 .getInterface(Ci.nsIDOMWindowUtils);
  var rect = fileInput.getBoundingClientRect();
  wu.sendMouseEvent("mousedown", rect.left + 5, rect.top + 5, 0, 1, 0);
  wu.sendMouseEvent("mouseup", rect.left + 5, rect.top + 5, 0, 1, 0);
  SimpleTest.executeSoon(checkFirst);
}

and adding a <div id=dummy style="width:10px;height:10px"> somewhere in the document.

r=me with those changes!

Comment 131

[David Zbarsky (:dzbarsky)]

(In reply to comment #130)
> Why this name change? We generally use fooBar as naming convention for
> instances and FooBar for classes.

Oops, that's from trying all kinds of things to get this code to work.

> In this file it might make more sense to move the unregister call to the show()
> method implementation. (In the other test it makes more sense to keep the
> executeSoon though).

Moving unregister to show will be really bad, because we will then call GetFile on the real nsIFilePicker, which we did not Init, which will lead to null pointer derefs.

> Alternatively change this to use executeSoon. I'm fine either way.

I'll try this.

> It'd be better to test that things work even with the popup blocker enabled.
> You can do this by changing runTests to do something like this:
> 
> function runTests() {
>   var dummy = $("dummy");
>   cont.addEventListener("click", function() {
>     document.getElementById("a").click();
>   }, false);
>   netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
>   var wu = window.QueryInterface(Ci.nsIInterfaceRequestor)
>                  .getInterface(Ci.nsIDOMWindowUtils);
>   var rect = fileInput.getBoundingClientRect();
>   wu.sendMouseEvent("mousedown", rect.left + 5, rect.top + 5, 0, 1, 0);
>   wu.sendMouseEvent("mouseup", rect.left + 5, rect.top + 5, 0, 1, 0);
>   SimpleTest.executeSoon(checkFirst);
> }

Should that be 
dummy.addEventListener... and var rect = dummy.getBoundingClineRect()? Otherwise, I think you are simply clicking the input.

Comment 132

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

(In reply to comment #131)
> > In this file it might make more sense to move the unregister call to the
> > show() method implementation. (In the other test it makes more sense to
> > keep the executeSoon though).
> 
> Moving unregister to show will be really bad, because we will then call
> GetFile on the real nsIFilePicker, which we did not Init, which will lead to
> null pointer derefs.

That really shouldn't be happening as the code should already be holding an instance of nsIFilePicker and be making all subsequent calls after show() on it. The code doesn't grab a new filepicker after every call.

But I'm fine with executeSoon too.

> > It'd be better to test that things work even with the popup blocker enabled.
> > You can do this by changing runTests to do something like this:
> > 
> > function runTests() {
> >   var dummy = $("dummy");
> >   cont.addEventListener("click", function() {
> >     document.getElementById("a").click();
> >   }, false);
> >   netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
> >   var wu = window.QueryInterface(Ci.nsIInterfaceRequestor)
> >                  .getInterface(Ci.nsIDOMWindowUtils);
> >   var rect = fileInput.getBoundingClientRect();
> >   wu.sendMouseEvent("mousedown", rect.left + 5, rect.top + 5, 0, 1, 0);
> >   wu.sendMouseEvent("mouseup", rect.left + 5, rect.top + 5, 0, 1, 0);
> >   SimpleTest.executeSoon(checkFirst);
> > }
> 
> Should that be 
> dummy.addEventListener... and var rect = dummy.getBoundingClineRect()?
> Otherwise, I think you are simply clicking the input.

Yes, sorry, too much copy'n'paste.

Comment 133

[David Zbarsky (:dzbarsky)]

> That really shouldn't be happening as the code should already be holding an
> instance of nsIFilePicker and be making all subsequent calls after show() on
> it. The code doesn't grab a new filepicker after every call.
> 
> But I'm fine with executeSoon too.

Ah, I see you are right.

> Yes, sorry, too much copy'n'paste.

No worries, I'm just glad I understand what you tried to do =)

Comment 134

[David Zbarsky (:dzbarsky)]

(In reply to comment #130)

> It'd be better to test that things work even with the popup blocker enabled.
> You can do this by changing runTests to do something like this:

I don't think we should do this.  I already changed test_bug377624.html to enable popup blocking, and that test focuses the input and presses space, thus simulating a click (which this would be doing), while this test is currently testing a different thing, so it is more useful.

Comment 135

[David Zbarsky (:dzbarsky)]

Attachment: To checkin part 3

Comment 136

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

(In reply to comment #134)
> (In reply to comment #130)
> 
> > It'd be better to test that things work even with the popup blocker enabled.
> > You can do this by changing runTests to do something like this:
> 
> I don't think we should do this.  I already changed test_bug377624.html to
> enable popup blocking, and that test focuses the input and presses space, thus
> simulating a click (which this would be doing), while this test is currently
> testing a different thing, so it is more useful.

Actually, no, the code I was suggesting in comment 130 doesn't click the <input type=file>.

What it does is that it clicks a totally *separate* element, and tests that an event handler for the "click" event that is dispatched as a result of that click is allowed to call inputElement.click().

I.e. it tests that calling inputElement.click() is allowed when popups are allowed.

The intent is to test that the popup blocker doesn't completely disable the ability to call inputElement.click().

This also simulates what is probably the most common use case for inputElement.click(). Someone wanting to create a nicer UI for file attachment by creating a separate set of elements, and when the user clicks those elements call inputElement.click() to bring up a file picker.

Hope this makes sense?

Comment 137

[David Zbarsky (:dzbarsky)]

OK I see what you tried to do.
Unfortunately, calling click() on a div doesn't currently work (will be fixed in bug 583514).  Maybe we should just change this when that lands?

Comment 138

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

That's why I wasn't calling .click() on the <div>, but instead used wu.sendMouseEvent to get the same effect :)

(actually, calling .click() on the div wouldn't work as that wouldn't unblock the popup blocker. The whole point of the popup blocker is to only allow popups when the user is *really* clicking on an element, rather than the page faking a click through various APIs).

Comment 139

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

Btw, here is a the fixed code which should better illustrate the intended idea.

function runTests() {
  var dummy = $("dummy");
  dummp.addEventListener("click", function() {
    document.getElementById("a").click();
  }, false);
  netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
  var wu = window.QueryInterface(Ci.nsIInterfaceRequestor)
                 .getInterface(Ci.nsIDOMWindowUtils);
  var rect = dummy.getBoundingClientRect();
  wu.sendMouseEvent("mousedown", rect.left + 5, rect.top + 5, 0, 1, 0);
  wu.sendMouseEvent("mouseup", rect.left + 5, rect.top + 5, 0, 1, 0);
  SimpleTest.executeSoon(checkFirst);
}

Comment 140

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

Comment on attachment 469866 [details] [diff] [review]
To checkin part 3

Minusing based on above comments. (In general, be careful about marking r=someone if you don't make all the requested changes)

Comment 141

[David Zbarsky (:dzbarsky)]

Attachment: Changes to the test

Comment 142

[Mounir Lamouri (:mounir)]

Attachment: Fix Tests

Comment 143

[Mounir Lamouri (:mounir)]

(In reply to comment #142)
> Created attachment 470948 [details] [diff] [review]
> Fix Tests

This patch is also cleaning a bit the file like removing the ugly ^M at eol.

Comment 144

[Mounir Lamouri (:mounir)]

Last test pushed:
http://hg.mozilla.org/mozilla-central/rev/126e91feeb53

Previous pushes were:
http://hg.mozilla.org/mozilla-central/rev/a5a97bafc953
http://hg.mozilla.org/mozilla-central/rev/af7d35e5d336
http://hg.mozilla.org/mozilla-central/rev/a8328013a773

Given that these three pushes have been done last week, the feature will be available in beta5.

Bug completely fixed now...

Comment 145

[Eric Shepherd [:sheppy]]

removing doc needed flag, tracking doc requirement off bug 592802 instead.

Comment 146

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

So why do we fire the click event async?

Comment 147

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

Ah, it is just FireAsyncClickHandler(), not really the event dispatch.

Though, I still don't understand why we need it.
The default handler for click event should take care of file picker.

Comment 148

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

We wanted the ability to have the filepicker be asynchronous in the future. So we don't want people to rely on code like:

...
myinputelement.click();
if (myinputelement.value) {
  xhr.send(myinputelement.files[0]);
}
...


or even worse:


while (myinputelement.value == "") {
  myinputelement.click();
}