Closed Bug 701353 Opened 13 years ago Closed 11 years ago

When clicking on a label attached to a File Upload field, it no longer works to upload a file.

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: bigdaddykraven, Assigned: mounir)

References

Details

(Keywords: testcase, Whiteboard: [parity-chrome][parity-ie][fixed by bug 838695])

Attachments

(3 files, 13 obsolete files)

404 bytes, text/html
Details
519 bytes, text/html
Details
10.26 KB, patch
Details | Diff | Splinter Review
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_7_2) AppleWebKit/535.2 (KHTML, like Gecko) Chrome/15.0.874.106 Safari/535.2

Steps to reproduce:

When designing/developing a web page I wanted to create a custom button for uploading a file, which is done by hiding the ugly normal file upload field (done by moving it off the screen hiding it in an overflow). Normally, like most form fields, you create a style a label which is attached to the field and makes it clickable to activate that field.


Actual results:

When clicking the label...the browser does nothing.


Expected results:

When clicking the label, the file browser should pop up and allow me to upload a file in the same manner as if I had clicked a standard file upload form field.
I know this has been an issue since Firefox 7 and is still evident in 8. It works in Many versions of IE, Chrome and Safari.
OS: Mac OS X → Windows Vista
Can you attach a HTML testcase or URL?
Can't show you what I'm working on but the code is very simple.

<label>
You should be able to click here to launch file browser
<input type="file">
</label>

You could do it this way too, either of which should work but neither do:

<label for="test">
You should be able to click here to launch file browser
</label>
<input type="file" id="test">
Attached file testcase
It does not work on Linux either. Also, does not work in Opera. Doesn't work in Firefox 4 either, so it seems not to be a regression. Can you say if it worked in any FF version?


Does work in Chrome.
Component: General → Layout: Form Controls
OS: Windows Vista → All
Product: Firefox → Core
QA Contact: general → layout.form-controls
Hardware: x86 → All
Version: unspecified → Trunk
I feel like I've done it before in FF3 but it was a while ago and they could have been using JS on those instances. I must admit that I don't use Opera much so I didn't test it but have had it working in IE, Chrome and Safari. I haven't done a file upload for a while and when I have, I used the default box instead of making my own button like I'm doing now.
Forgot to confirm. However I can't confirm the spec requires this behavior.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [parity-chrome][parity-ie]
Whiteboard: [parity-chrome][parity-ie] → DUPEME[parity-chrome][parity-ie]
FYI, When click label text,
Firefox 1.5.0.x : Input box is focused and caret is in the input box. No file picker pops up automatically.
Firefox 2.0.0.x : "Browse..." button is focused. No file picker pops up automatically.
Firefox 3.0.0.x and later : Nothing happens
Given that we know allow .click() to trigger the file picker, I don't see any reason to disallow clicking the label to do the same.
I had a quick look at the code and it seems that we handle .click() as a special case. Why not opening the file picker in ::PostHandleEvent instead of special casing .click in ::Click? If I'm not missing anything, I think we could easily solve this bug if we were calling the file picker when a trusted click event was received on an <input type='file'>.
Component: Layout: Form Controls → DOM: Core & HTML
QA Contact: layout.form-controls → general
Whiteboard: DUPEME[parity-chrome][parity-ie] → DUPEME[parity-chrome][parity-ie][mentor=volkmar]
Whiteboard: DUPEME[parity-chrome][parity-ie][mentor=volkmar] → DUPEME[parity-chrome][parity-ie][mentor=mounir
Patch works as expected.
Attachment #583315 - Flags: review?(mounir)
Comment on attachment 583315 [details] [diff] [review]
Popup file picker when click on label.

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

Olli, could you check if the call in ::PostHandleEvent() is correctly placed? This method is probably the most obscure method in nsHTMLInputElement.cpp (for me).

Also, Jignesh, did you check if .click() was still working and sending an untrusted click event doesn't show the file picker? The former is very likely tested in our test suite but I doubt the later is. It might be interesting to write a test doing that if you want to try.

And thank you for the patch :)

::: content/html/content/src/nsHTMLInputElement.cpp
@@ -1740,2 @@
>    return nsGenericHTMLElement::Click();
>  }

You should just remove this method. It's a virtual method so it will automatically call the correct implementation.
Attachment #583315 - Flags: review?(mounir)
Attachment #583315 - Flags: review?(bugs)
Attachment #583315 - Flags: feedback+
Assignee: nobody → jigneshhk1992
Status: NEW → ASSIGNED
Changes :

- Removed click from cpp and put it into header
- Removed click events from layouts for inputbox
Attachment #583315 - Attachment is obsolete: true
Attachment #583315 - Flags: review?(bugs)
Attachment #583358 - Flags: review?(mounir)
Comment on attachment 583358 [details] [diff] [review]
Click on label to popup file dialog

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

The patch looks good and tested locally, it makes us behave like Chromium and Opera. Though, I would like to understand why you don't have to remove the click handler from the 'browse' button. Ideally, it might be nice to remove that code and handle that in the content too. Could you try to investigate this?
Attachment #583358 - Flags: review?(mounir)
Attachment #583358 - Flags: review?(bugs)
Attachment #583358 - Flags: feedback+
Changes : 
-Additional check for input button added.
-Clicking now completely moved to Content for input file.
Attachment #583358 - Attachment is obsolete: true
Attachment #583358 - Flags: review?(bugs)
Attachment #583521 - Flags: review?(mounir)
I don't think this approach is correct because it seems that all buttons are going to try to show a file picker... What is happening is that the text file doesn't handle the click event itself but the <input type=file> does. However, the button handles the event (as you can see, it gets pushed) so <input type=file> isn't aware of the button being clicked. Basically, you should imagine an <input type='file'> element like this:
<input type='file'>
  <input>
  <input type='button'>
</input>
This is not a correct syntax but it can help understanding that if the inner <input> doesn't handle the click event, it goes to the root element.

I assume the click event is actually bubbling and the root element of that tree actually gets it but doesn't handle it because of some reasons (not the target maybe?). We could probably fix this.
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #16)
> I assume the click event is actually bubbling and the root element of that
> tree actually gets it but doesn't handle it because of some reasons (not the
> target maybe?). We could probably fix this.

This code makes us ignore all events coming from the Browse button:
>  // ignore the activate event fired by the "Browse..." button
>  // (file input controls fire their own) (bug 500885)
>  if (mType == NS_FORM_INPUT_FILE) {
>    nsCOMPtr<nsIContent> maybeButton =
>      do_QueryInterface(aVisitor.mEvent->originalTarget);
>    if (maybeButton &&
>      maybeButton->IsRootOfNativeAnonymousSubtree() &&
>      maybeButton->AttrValueIs(kNameSpaceID_None,
>                               nsGkAtoms::type,
>                               nsGkAtoms::button,
>                               eCaseMatters)) {
>        return NS_OK;
>    }
>  }

I think we can remove this.
Attachment #583521 - Attachment is obsolete: true
Attachment #583521 - Flags: review?(mounir)
Attachment #583553 - Flags: review?(mounir)
Comment on attachment 583553 [details] [diff] [review]
Click on label to popup file dialog

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

In nsFileControlFrame.cpp, you should remove the block for the "click" event in ::BrowseMouseListener::HandleEvent and update the comment just above to remove the mention of click events.

r=me with that (but I need a DOM peer to review that)

However, to land this, you will need to write some tests. At least checking that clicking on a label shows the file picker but maybe also that clicking on a button and sending a click event (with .dispatchEvent) without privileges show a file picker (I thought it should not but it seems it does and Chrome does the same thing).
An example of test using the filepicker can be found here: layout/forms/test/test_bug36619.html
Though, your test should be located here: content/html/content/test/forms/test_click_input_file.html (actually, the other test should be here too...)

Do you need any pointer on how to write a mochitest?
Attachment #583553 - Flags: review?(mounir)
Attachment #583553 - Flags: review?(bugs)
Attachment #583553 - Flags: review+
Attached file testcase 2
This is an idea on what to test: click on the label and clicking on the button should show a file picker.
Comment on attachment 583553 [details] [diff] [review]
Click on label to popup file dialog

mounir did comment all the issues I noticed.
Attachment #583553 - Flags: review?(bugs) → review+
Attachment #583553 - Attachment is obsolete: true
Attachment #583762 - Flags: review?(mounir)
Comment on attachment 583762 [details] [diff] [review]
Test+Patch. Click on label to popup file dialog.

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

I would like to see another version of the tests. Also, you could probably remove the other test in layout/forms/test/.

::: content/html/content/test/forms/test_click_input_file.html
@@ +12,5 @@
> +<body>
> +<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=701353">Mozilla Bug 701353</a>
> +<p id="display"></p>
> +<div id="content" style="display:none;">
> +  <label id="label" onclick="document.getElementById('inputbox').click()">

You should remove the click handler here.

@@ +17,5 @@
> +    this is a label
> +    <input id='inputbox' type='file'>
> +  </label>
> +</div>
> +<button id='browse' onclick="document.getElementById('inputbox').click();">Show Filepicker</button>

Could you add a similar button using dispatchEvent instead of .click()?

@@ +38,5 @@
> +
> +    // Tests that a click on 'b' calls the show method.
> +    var brw = document.getElementById('browse');
> +    brw.focus(); // Be sure the element is visible.
> +    synthesizeMouse(brw, 2, 2, {});

Could you use that method instead: synthesizeMouseAtCenter

@@ +49,5 @@
> +      SimpleTest.executeSoon(function() {
> +        ok(!MockFilePicker.shown, "File picker show method should not have been called");
> +        MockFilePicker.reset();
> +    
> +    var lbl = document.getElementById('browse');

Could you fix the identation of that block?
Attachment #583762 - Flags: review?(mounir)
Attachment #583762 - Attachment is obsolete: true
Attachment #583799 - Flags: review?(mounir)
Attachment #583799 - Attachment is obsolete: true
Attachment #583799 - Flags: review?(mounir)
Attachment #583801 - Flags: review?(mounir)
Attachment #583801 - Attachment is obsolete: true
Attachment #583801 - Flags: review?(mounir)
Attachment #583804 - Flags: review?(mounir)
Comment on attachment 583804 [details] [diff] [review]
Test+Patch. Click on label to popup file dialog.

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

I think you forgot to add test_click_input_file.html to the Makefile or you forgot to add the change in the patch...

r=me with the Makefile changed and the test for .click()

::: content/html/content/test/forms/test_click_input_file.html
@@ +22,5 @@
> +
> +    // enable popups the first time
> +    SpecialPowers.pushPrefEnv({'set': [
> +    ["dom.disable_open_during_load", true],
> +    ["privacy.popups.showBrowserMessage", false]

nit: those two lines are mis-indented

@@ +68,5 @@
> +    <label id="label">
> +      this is a label
> +      <input id='inputbox' type='file'>
> +    </label>
> +  <button id='button' onclick="sendClickEvent();">Show Filepicker</button>

I would like a second test with .click(). That was the meaning of my previous comment. Sorry if I wasn't clear enough :(
Attachment #583804 - Flags: review?(mounir) → review+
Attachment #583804 - Attachment is obsolete: true
Attachment #583817 - Flags: review?(mounir)
Attachment #583817 - Attachment is obsolete: true
Attachment #583817 - Flags: review?(mounir)
Attachment #583839 - Flags: review?(mounir)
Comment on attachment 583839 [details] [diff] [review]
Tests+Patch. Click on label to popup file dialog.

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

r=mounir,smaug

Do you access to the try server?
Attachment #583839 - Flags: review?(mounir) → review+
> Do you access to the try server?

No.
I sent this patch to the try server and it unfortunately breaks a test:
61032 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/test_bug500885.html | click on button should fire 1 DOMActivate event - got 2, expected 1

Log:
https://tbpl.mozilla.org/php/getParsedLog.php?id=8119740&tree=Try

Off-hand, I wouldn't be able to say what produces this error.
Jignesh, can you have a look?
> 61032 ERROR TEST-UNEXPECTED-FAIL |
> /tests/content/html/content/test/test_bug500885.html | click on button
> should fire 1 DOMActivate event - got 2, expected 1

The test which fails is here https://bugzilla.mozilla.org/attachment.cgi?id=414414&action=diff . We can probably remove that test too. Because we deleted the block of the code for which it is made for.
As I said, the test is valid. There is some bug in the patch which causes two DOMActivate events
to fire.
Attached patch Fix test (obsolete) — Splinter Review
This should fix the test failure.
Attachment #585201 - Flags: review?(bugs)
Comment on attachment 585201 [details] [diff] [review]
Fix test

Why don't you keep the old code there?
Comment on attachment 585201 [details] [diff] [review]
Fix test

Since this breaks event handling in the native anonymous button itself.
(It is possible that no one is listening DOMActivate on the
button, but I'd rather not add inconsistencies to event handling if
possible.)
Attachment #585201 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] from comment #37)
> Comment on attachment 585201 [details] [diff] [review]
> Fix test
> 
> Why don't you keep the old code there?

The old code does the opposite: when the file element gets an event targeted to the anonymous button, it is ignoring it. This code makes the button ignoring the event instead so the event can be handled by the file element.
It is not opposite. You're prevent DOMActivate to fire for the button at all.
In that case diff attached in Comment 13 + test in Comment 29 will work for what we are looking for.

Should I go for that?
Clicking <input type="file" /> by clicking its label does currently work in Opera (12.00 Build 1467) too ([parity-opera] probably should be added to "Whiteboard" field of this bug).

It turns out that Firefox is now the only browser that does not pass click event from label element to its labeled form-control.
Keeping it open for someone who would like to work on it.:)
Assignee: jigneshhk1992 → nobody
Depends on: 834652
Keywords: testcase
Whiteboard: DUPEME[parity-chrome][parity-ie][mentor=mounir → DUPEME[parity-chrome][parity-ie][mentor=mounir]
The patch in bug 838695 is going to fix this bug. We should probably take the tests though. At least, add a test with a click on label.
Assignee: nobody → mounir
Depends on: 838695
No longer depends on: 834652
Whiteboard: DUPEME[parity-chrome][parity-ie][mentor=mounir] → [parity-chrome][parity-ie]
Attached patch Tests (obsolete) — Splinter Review
Attachment #583839 - Attachment is obsolete: true
Attachment #585201 - Attachment is obsolete: true
Attachment #730628 - Flags: review?(bugs)
Flags: in-testsuite+
Whiteboard: [parity-chrome][parity-ie] → [parity-chrome][parity-ie][fixed by bug 838695]
Comment on attachment 730628 [details] [diff] [review]
Tests

IIRC felipe is changing these tests too, so I don't expect this to work anymore.
Attachment #730628 - Flags: review?(bugs)
Attached patch Tests (obsolete) — Splinter Review
The only changes made by Felipe were on the test file I'm deleting (merging with another). Should be good to go.
Attachment #730628 - Attachment is obsolete: true
Attachment #732766 - Flags: review?(bugs)
Attachment #732766 - Flags: review?(bugs) → review+
Attached patch Patch to checkinSplinter Review
Tree is closed :(
Attachment #732766 - Attachment is obsolete: true
Attachment #732872 - Flags: checkin?
Keywords: checkin-needed
Attachment #732872 - Flags: checkin?
https://hg.mozilla.org/mozilla-central/rev/1c37f9187fa8
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.