Closed
Bug 992691
Opened 10 years ago
Closed 10 years ago
Dragging multiple files to a file input field works even if it has no "multiple" attribute
Categories
(Core :: Layout: Form Controls, defect)
Core
Layout: Form Controls
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: j_campbell7, Assigned: Gijs)
References
(Blocks 1 open bug, )
Details
(Keywords: reproducible, testcase)
Attachments
(1 file)
4.65 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0 (Beta/Release) Build ID: 20140314220517 Steps to reproduce: Open a page using a form file input field (without the multiple attribute). Drag multiple files onto the browse target. Browse works as expected, multiple files cannot be selected. Actual results: File name field changes to xx files. Expected results: Drag should be denied in some way, either by an invalid cursor or error message.
Blocks: 770305
Status: UNCONFIRMED → NEW
Component: Untriaged → Drag and Drop
Ever confirmed: true
Product: Firefox → Core
Assignee | ||
Updated•10 years ago
|
Keywords: reproducible,
testcase
Assignee | ||
Comment 2•10 years ago
|
||
Also broken on OS X, so twiddling some more stuff. I expect this might need to be in Form control land...
Component: Drag and Drop → Layout: Form Controls
OS: Windows 8.1 → All
Hardware: x86_64 → All
Version: 28 Branch → Trunk
Comment 3•10 years ago
|
||
So in nsFileControlFrame::DnDListener::HandleEvent the "drop" case we get the list of files and call SetFiles on the input. HTMLInputElement::SetFiles (both overloads) just set the state to those files, whether "multiple" is set or not. Presumably we should be checking @multiple somewhere in here, right? The drop handler would make some sense...
Flags: needinfo?(bugs)
Summary: Dragging multiple files to a file input field works → Dragging multiple files to a file input field works even if it has no "multiple" attribute
Comment 4•10 years ago
|
||
yeah, and then what? Dispatch a chrome only event which FF UI can catch and show an error message perhaps. Do we have some similar error handling in FF UI already which could be reused here (with some other error message)? What is the e10s-compatible way to show errors?
Flags: needinfo?(bugs) → needinfo?(jaws)
Updated•10 years ago
|
Flags: needinfo?(jaws) → needinfo?(gijskruitbosch+bugs)
Assignee | ||
Comment 5•10 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #4) > yeah, and then what? Dispatch a chrome only event which FF UI can catch and > show an error message perhaps. > > Do we have some similar error handling in FF UI already which could be > reused here (with some other error message)? What is the e10s-compatible way > to show errors? Why do we need to show an error message? FWIW, I am halfway implementing this, hence the recent question in #developers :-) It seems that IE doesn't support dropping files on input elements at all. Blink does, and just shows a "drag forbidden" cursor when dragging over the file input, and a "drag cancelled" animation if you drop it there anyway. I'm actually struggling getting exactly the same behaviour; setting dropEffect and effectAllowed to "none" doesn't seem to be enough to achieve it (instead, the browser ends up navigating to one of the files in the set). I think as a first step, it's fine to make the element not accept the drag/drop. Note that there are further issues here (and AFAICT, this is all not specced because the whatwg spec only mentions dnd behaviour as something of an aside), like what happens if I drag 5 files and the input has an accept attribute that only accepts 1 of them? (Blink doesn't support things like accept="image/png", AFAICT, while IIRC we do, in principle? Maybe I'm misremembering)
Flags: needinfo?(gijskruitbosch+bugs)
Comment 6•10 years ago
|
||
It might be a bit better UX to tell the user why dnd multiple files didn't succeed. But I'm fine just not accepting drop. (Don't call preventDefault() here http://mxr.mozilla.org/mozilla-central/source/layout/forms/nsFileControlFrame.cpp#196 if dragging multiple files on top of non-multiple type="file" ?) (Let's not deal with accept="foo/bar" in this bug. accept is anyway just a hint)
Assignee | ||
Comment 7•10 years ago
|
||
I refactored things a little... I hope the current setup makes sense. And yes, I was thinking the accept stuff should be a followup bug. It wouldn't hurt to have more clarity from the spec on what would be expected, either...
Attachment #8524651 -
Flags: review?(bugs)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Comment 8•10 years ago
|
||
Comment on attachment 8524651 [details] [diff] [review] don't allow dropping multiple files on non-multiple file input, >+ if (!CanDropTheseFiles(dataTransfer, supportsMultiple)) { >+ dataTransfer->SetDropEffect(NS_LITERAL_STRING("none")); This should be enough >+ dataTransfer->SetEffectAllowed(NS_LITERAL_STRING("none")); So don't call this one.
Attachment #8524651 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 9•10 years ago
|
||
remote: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c8e35a08ebb9
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(gijskruitbosch+bugs)
Assignee | ||
Comment 10•10 years ago
|
||
Pushed with the right forwarded class declaration at the top of the header file so as not to break non-unified linux debug builds. Better try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=109ddb3014de (reftest failure is a bad parent cset from m-i, got backed out there) push: remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/b8919fcbf6b2
Flags: needinfo?(gijskruitbosch+bugs)
https://hg.mozilla.org/mozilla-central/rev/b8919fcbf6b2
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in
before you can comment on or make changes to this bug.
Description
•