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)
Core
DOM: Core & HTML
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.
Reporter | ||
Comment 1•13 years ago
|
||
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
Reporter | ||
Comment 3•13 years ago
|
||
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">
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
Reporter | ||
Comment 6•13 years ago
|
||
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]
Updated•13 years ago
|
Whiteboard: [parity-chrome][parity-ie] → DUPEME[parity-chrome][parity-ie]
Comment 8•13 years ago
|
||
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
Assignee | ||
Comment 9•13 years ago
|
||
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.
Assignee | ||
Comment 10•13 years ago
|
||
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
Assignee | ||
Updated•13 years ago
|
Whiteboard: DUPEME[parity-chrome][parity-ie] → DUPEME[parity-chrome][parity-ie][mentor=volkmar]
Assignee | ||
Updated•13 years ago
|
Whiteboard: DUPEME[parity-chrome][parity-ie][mentor=volkmar] → DUPEME[parity-chrome][parity-ie][mentor=mounir
Assignee | ||
Comment 12•12 years ago
|
||
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 | ||
Updated•12 years ago
|
Assignee: nobody → jigneshhk1992
Status: NEW → ASSIGNED
Comment 13•12 years ago
|
||
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)
Assignee | ||
Comment 14•12 years ago
|
||
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+
Comment 15•12 years ago
|
||
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)
Assignee | ||
Comment 16•12 years ago
|
||
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.
Assignee | ||
Comment 17•12 years ago
|
||
(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.
Comment 18•12 years ago
|
||
Attachment #583521 -
Attachment is obsolete: true
Attachment #583521 -
Flags: review?(mounir)
Attachment #583553 -
Flags: review?(mounir)
Assignee | ||
Comment 19•12 years ago
|
||
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+
Assignee | ||
Comment 20•12 years ago
|
||
This is an idea on what to test: click on the label and clicking on the button should show a file picker.
Comment 21•12 years ago
|
||
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+
Comment 22•12 years ago
|
||
Attachment #583553 -
Attachment is obsolete: true
Attachment #583762 -
Flags: review?(mounir)
Assignee | ||
Comment 23•12 years ago
|
||
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)
Comment 24•12 years ago
|
||
Attachment #583762 -
Attachment is obsolete: true
Attachment #583799 -
Flags: review?(mounir)
Comment 25•12 years ago
|
||
Attachment #583799 -
Attachment is obsolete: true
Attachment #583799 -
Flags: review?(mounir)
Attachment #583801 -
Flags: review?(mounir)
Comment 26•12 years ago
|
||
Attachment #583801 -
Attachment is obsolete: true
Attachment #583801 -
Flags: review?(mounir)
Attachment #583804 -
Flags: review?(mounir)
Assignee | ||
Comment 27•12 years ago
|
||
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+
Comment 28•12 years ago
|
||
Attachment #583804 -
Attachment is obsolete: true
Attachment #583817 -
Flags: review?(mounir)
Comment 29•12 years ago
|
||
Attachment #583817 -
Attachment is obsolete: true
Attachment #583817 -
Flags: review?(mounir)
Attachment #583839 -
Flags: review?(mounir)
Assignee | ||
Comment 30•12 years ago
|
||
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+
Comment 31•12 years ago
|
||
> Do you access to the try server?
No.
Assignee | ||
Comment 32•12 years ago
|
||
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?
Comment 33•12 years ago
|
||
> 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.
Comment 34•12 years ago
|
||
As I said, the test is valid. There is some bug in the patch which causes two DOMActivate events to fire.
Assignee | ||
Comment 35•12 years ago
|
||
This should fix the test failure.
Attachment #585201 -
Flags: review?(bugs)
Assignee | ||
Comment 36•12 years ago
|
||
By "should" I mean "is": https://tbpl.mozilla.org/?tree=Try&rev=d4d1bfda04f3
Comment 37•12 years ago
|
||
Comment on attachment 585201 [details] [diff] [review] Fix test Why don't you keep the old code there?
Comment 38•12 years ago
|
||
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-
Assignee | ||
Comment 39•12 years ago
|
||
(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.
Comment 40•12 years ago
|
||
It is not opposite. You're prevent DOMActivate to fire for the button at all.
Comment 41•12 years ago
|
||
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?
Comment 42•12 years ago
|
||
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.
Comment 43•12 years ago
|
||
Keeping it open for someone who would like to work on it.:)
Assignee: jigneshhk1992 → nobody
Updated•11 years ago
|
Assignee | ||
Comment 45•11 years ago
|
||
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 | ||
Comment 46•11 years ago
|
||
Attachment #583839 -
Attachment is obsolete: true
Attachment #585201 -
Attachment is obsolete: true
Attachment #730628 -
Flags: review?(bugs)
Assignee | ||
Updated•11 years ago
|
Flags: in-testsuite+
Whiteboard: [parity-chrome][parity-ie] → [parity-chrome][parity-ie][fixed by bug 838695]
Comment 47•11 years ago
|
||
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)
Assignee | ||
Comment 48•11 years ago
|
||
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)
Updated•11 years ago
|
Attachment #732766 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 49•11 years ago
|
||
Tree is closed :(
Attachment #732766 -
Attachment is obsolete: true
Attachment #732872 -
Flags: checkin?
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Updated•11 years ago
|
Attachment #732872 -
Flags: checkin?
Comment 50•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c37f9187fa8
Keywords: checkin-needed
Comment 51•11 years ago
|
||
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.
Description
•