Bug 58208 - hidden attribute on <input type=file /> suppresses the file selection dialog
: hidden attribute on <input type=file /> suppresses the file selection dialog
Status: RESOLVED FIXED
: WebKit
Forms
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2011-04-10 12:39 PST by
Modified: 2012-02-02 00:09 PST (History)


Attachments
testcase (246 bytes, text/html)
2011-04-10 12:39 PST, Darth
no flags Details
Proposed patch (2.89 KB, patch)
2012-01-31 08:16 PST, Rakesh
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-04-10 12:39:24 PST
Created an attachment (id=88957) [details]
testcase

Downstream bug: http://code.google.com/p/chromium/issues/detail?id=78961
Test case attached. Will show file selection box in firefox, and nothing in webkit.

<input id=inp type=file hidden />
<button onclick="document.getElementById('inp').click();">Click me</button>

The onclick event is fired though, which can be tested with a simple alert in the input.

Looking at the spec, I do read
"Elements that are not hidden should not link to or refer to elements that are hidden."
http://dev.w3.org/html5/spec/Overview.html#the-hidden-attribute
Though unsure if this falls into that category.
------- Comment #1 From 2011-10-13 01:25:21 PST -------
As per my observation, in Firefox and IE, we are able to open file system though the "file" element is hidden. 

In spec, there is written "All HTML elements may have the hidden content attribute set."
------- Comment #2 From 2011-10-13 01:29:39 PST -------
As per my observation, in Firefox and IE, we are able to open file system though the "file" element is hidden. 

In spec, there is written "All HTML elements may have the hidden content attribute set."

So in Chrome and Safari, the behavior should be the same as expected.
------- Comment #3 From 2011-10-13 12:11:30 PST -------
> Looking at the spec, I do read

That's conformance information for authors, not for user agents.

We probably should match Firefox and IE here.  Suppressing the dialog in this case isn't buying us any security.  (The control could just as easily be "hidden" by being positioned off-screen.)

There's some question about whether the click() method should work at all on this control, but it's behavior shouldn't be different depending on the hidden attribute (unless required by compatibility).
------- Comment #4 From 2011-10-13 12:20:28 PST -------
FWIW, we also disable implicit submission when a button is hidden.
------- Comment #5 From 2011-10-13 12:21:28 PST -------
(In reply to comment #3)
> There's some question about whether the click() method should work at all on this control, but it's behavior shouldn't be different depending on the hidden attribute (unless required by compatibility).

Mozilla has enabled the .click() method even when the control is otherwise hidden. Using opacity: 0 and setting width/height dimensions are what I usually do for WebKit, though webkit's pseudo-selectors are powerful enough to handle it with more grace.

I strongly suggest keeping click, to stay in line with Mozilla.
------- Comment #6 From 2012-01-30 23:53:58 PST -------
Duplicate of bug 53246?
------- Comment #7 From 2012-01-31 08:16:47 PST -------
Created an attachment (id=124739) [details]
Proposed patch

Patch v1
------- Comment #8 From 2012-01-31 08:20:37 PST -------
Attachment 124739 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
First, rewinding head to replay your work on top of it...
Applying: Fix compilation errors on build-webkit --debug --no-workers on mac.
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging LayoutTests/platform/qt/Skipped
CONFLICT (content): Merge conflict in LayoutTests/platform/qt/Skipped
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 Fix compilation errors on build-webkit --debug --no-workers on mac.

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #9 From 2012-01-31 12:42:52 PST -------
(From update of attachment 124739 [details])
Is it possible to use a similar technique employed in input-file-not-open-without-gesture.html to automatically test this?
------- Comment #10 From 2012-01-31 21:45:51 PST -------
(In reply to comment #9)
> (From update of attachment 124739 [details] [details])
> Is it possible to use a similar technique employed in input-file-not-open-without-gesture.html to automatically test this?

Thanks for taking time to see this.

I think it is not possible to automate this test as file dialog opens only when there is user gesture. In input-file-not-open-without-gesture.html the test is other way round where we don't want file dialog to open when not an user gesture. Please do suggest if there is an way to automate.
------- Comment #11 From 2012-01-31 21:55:29 PST -------
(In reply to comment #10)
> I think it is not possible to automate this test as file dialog opens only when there is user gesture. In input-file-not-open-without-gesture.html the test is other way round where we don't want file dialog to open when not an user gesture. Please do suggest if there is an way to automate.

But you can use eventSender to generate that "gesture", right?
------- Comment #12 From 2012-02-01 02:49:29 PST -------
(In reply to comment #11)
> (In reply to comment #10)
> > I think it is not possible to automate this test as file dialog opens only when there is user gesture. In input-file-not-open-without-gesture.html the test is other way round where we don't want file dialog to open when not an user gesture. Please do suggest if there is an way to automate.
> 
> But you can use eventSender to generate that "gesture", right?

Yes, I tried using eventSender but the problem is that when file dialog shows up
the current focus is on file dialog and until the file dialog closes the test won't end.

As per my understanding even if we generate user event with eventSender and test that body does not get the click event, we need to close the native file open dialog some how so that test ends(I think sending escape key event through eventSender will not have effect on native dialog).
------- Comment #13 From 2012-02-01 02:53:30 PST -------
(In reply to comment #12)
> Yes, I tried using eventSender but the problem is that when file dialog shows up
> the current focus is on file dialog and until the file dialog closes the test won't end.

input-file-not-open-without-gesture.html uses setTimeout for that. It schedules a call to notifyDone before opening the dialog.

> As per my understanding even if we generate user event with eventSender and test that body does not get the click event, we need to close the native file open dialog some how so that test ends(I think sending escape key event through eventSender will not have effect on native dialog).

So calling notifyDone in setTimeout won't work?
------- Comment #14 From 2012-02-01 03:29:17 PST -------
(In reply to comment #13)

> input-file-not-open-without-gesture.html uses setTimeout for that. It schedules a call to notifyDone before opening the dialog.

Yes, it does and it works fine as file dialog does not open in this test. input-file-not-open-without-gesture.html does timeout if the click happens through eventSender.

> So calling notifyDone in setTimeout won't work?

It does not work as file dialog is still open and the call FileInputType::handleDOMActivateEvent is not yet complete, I mean the stack has not yet unwinded.
------- Comment #15 From 2012-02-01 08:54:31 PST -------
The file dialog should not be appearing in DRT and WTR, similarly to how alert() does not start a modal loop. This might require changes in WebCore and test runners.
------- Comment #16 From 2012-02-01 22:17:22 PST -------
(In reply to comment #15)
> The file dialog should not be appearing in DRT and WTR, similarly to how alert() does not start a modal loop. This might require changes in WebCore and test runners.

What do you suggest, should we go ahead with this patch and for file dialog not be showing up in DRT and WTR we can file a new bug?
------- Comment #17 From 2012-02-01 23:24:53 PST -------
It would certainly be better to make this testable and add a test in the same patch. But I didn't override the r+, meaning that I don't object to landing the patch as is.
------- Comment #18 From 2012-02-01 23:32:08 PST -------
(From update of attachment 124739 [details])
Given that it's hard to automate the test, it seems fine to land the patch as is.
------- Comment #19 From 2012-02-02 00:09:23 PST -------
(From update of attachment 124739 [details])
Clearing flags on attachment: 124739

Committed r106538: <http://trac.webkit.org/changeset/106538>
------- Comment #20 From 2012-02-02 00:09:28 PST -------
All reviewed patches have been landed.  Closing bug.