[Bug 131145]
(In reply to comment #62) We have some automated tests for drag-and-drop, why can't we automate a test here? Because of GetFiles(). It blocks us getting the files if the original event of the transferData was not drop, but the way we make a dataTransfer in script is to synthesise a dragstart event and use that. -- You received this bug notification because you are a member of Ubuntu Desktop Bugs, which is subscribed to epiphany-browser in Ubuntu. https://bugs.launchpad.net/bugs/131145 Title: Dragging icon from Nautilus to HTML File Input box does not work -- desktop-bugs mailing list desktop-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/desktop-bugs
[Bug 131145]
That would require exposing something very sensitive to script, I thought. I did look into it while I was trying to write a test, and the only place this field is written to is the constructor and copy- constructor, so I assumed the field was kept private for security reasons. -- You received this bug notification because you are a member of Ubuntu Desktop Bugs, which is subscribed to epiphany-browser in Ubuntu. https://bugs.launchpad.net/bugs/131145 Title: Dragging icon from Nautilus to HTML File Input box does not work -- desktop-bugs mailing list desktop-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/desktop-bugs
[Bug 131145]
http://hg.mozilla.org/mozilla-central/rev/5c6d107ede5a -- You received this bug notification because you are a member of Ubuntu Desktop Bugs, which is subscribed to epiphany-browser in Ubuntu. https://bugs.launchpad.net/bugs/131145 Title: Dragging icon from Nautilus to HTML File Input box does not work -- desktop-bugs mailing list desktop-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/desktop-bugs
[Bug 131145]
This will be in Firefox 7 at the earliest. As there is problems with automatically testing this bug, we'd need a litmus test. -- You received this bug notification because you are a member of Ubuntu Desktop Bugs, which is subscribed to epiphany-browser in Ubuntu. https://bugs.launchpad.net/bugs/131145 Title: Dragging icon from Nautilus to HTML File Input box does not work -- desktop-bugs mailing list desktop-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/desktop-bugs
[Bug 131145]
(In reply to comment #54)
Comment on attachment 534375 [details] [review]
Patch 2
Review of attachment 534375 [details] [review]:
-
f=mounir with the nits fixed.
Moving the review of the dragover/drop events handling to Neil.
::: content/html/content/src/nsHTMLInputElement.cpp
@@ +1390,5 @@
+ mFiles.AppendObject(file);
+}
+ }
+
+ AfterSetFiles(aSetValueChanged);
Couldn't you just create a nsCOMArraynsIDOMFile from the nsIDOMFileList
and call SetFiles() with the array in parameter? That would prevent creating
another method with a name that I personally find confusing.
I don't know about this, wouldn't it be faster to simply use the
existing list rather than allocate a new one? Also, I don't see what's
confusing about the name especially since it's a private method.
::: content/html/content/src/nsHTMLInputElement.h
@@ +148,5 @@
+
+ // Forward nsIDOMHTMLElement
+ NS_FORWARD_NSIDOMHTMLELEMENT_NOFOCUSCLICK(nsGenericHTMLFormElement::)
+ NS_IMETHOD Focus();
+ NS_IMETHOD Click();
This chunk doesn't seem to change something.
I know. I couldn't figure it out either and couldn't get rid of it.
::: layout/forms/nsFileControlFrame.cpp
@@ +274,5 @@
+ NS_ENSURE_STATE(dragTarget);
+ dragTarget-AddEventListener(NS_LITERAL_STRING(drop),
+ mMouseListener, PR_FALSE);
+ dragTarget-AddEventListener(NS_LITERAL_STRING(dragover),
+ mMouseListener, PR_FALSE);
IMO, it's weird to be able to drag-n-drop a file on the Browse button but
I see that's what Safari and Chrome do.
Bigger drop area = better usability :)
@@ +534,5 @@
+return NS_OK;
+ }
+
+ nsCOMPtrnsIDOMDragEvent dragEvent = do_QueryInterface(aEvent);
+ if (dragEvent) {
Here, you should do:
if (!dragEvent || !IsValidDropData(dragEvent)) {
return NS_OK;
}
It will save some indentation.
@@ +540,5 @@
+aEvent-GetType(eventType);
+if (eventType.EqualsLiteral(dragover)
+IsValidDropData(dragEvent)) {
+ // Prevent default if we can accept this drag data
+ aEvent-PreventDefault();
And here, that could be:
if (eventType.EqualsLiteral(dragover)) {
aEvent-PreventDefault();
return NS_OK;
}
Will do.
And BTW, why do you need to prevent default on dragover?
Because that's what other drop targets seem to do (like the browser),
and apparently (looking at synthesizeDrop() in the test harness) if a
dragover event is not consumed, that is interpreted as having nothing
that takes a drop at that point.
@@ +544,5 @@
+ aEvent-PreventDefault();
+} else if (eventType.EqualsLiteral(drop)) {
+ // We probably shouldn't let any drop data propagate from a file
+ // upload control
+ aEvent-StopPropagation();
Why?
I couldn't think of a good reason to allow it, and thought for security
reasons it's best to not let websites find out what you drag in there
immediately upon drop.
@@ +551,5 @@
+aEvent-PreventDefault();
+
+nsIContent* content = mFrame-GetContent();
+if (!content)
+ return NS_ERROR_FAILURE;
You can probably assert here instead of returning NS_ERROR_FAILURE.
@@ +555,5 @@
+ return NS_ERROR_FAILURE;
+
+nsHTMLInputElement* inputElement =
nsHTMLInputElement::FromContent(content);
+if (!inputElement)
+ return NS_ERROR_FAILURE;
For sure, you should assert here.
OK.
@@ +584,5 @@
+
+ // We only support dropping files onto a file upload control
+ PRBool typeSupported;
+ types-Contains(NS_LITERAL_STRING(Files), typeSupported);
+ return typeSupported;
You can also get the file list and check if it's empty.
Wouldn't that be taken care of already? If we get an empty file list, we
set the file upload control blank, and I think that's a good behaviour.
::: layout/forms/nsFileControlFrame.h
@@ +40,5 @@
#include nsBlockFrame.h
#include nsIFormControlFrame.h
#include nsIDOMMouseListener.h
+#include nsIDOMDragEvent.h
Do the include in the cpp file and do a forward declaration in the header
instead.
@@ +168,5 @@
class BrowseMouseListener: public MouseListener {
public:
BrowseMouseListener(nsFileControlFrame* aFrame) :
MouseListener(aFrame) {};
+NS_IMETHOD MouseClick(nsIDOMEvent* aMouseEvent);
You broke the indentation here.
@@ +171,5 @@
BrowseMouseListener(nsFileControlFrame* aFrame) :
MouseListener(aFrame) {};
+NS_IMETHOD MouseClick(nsIDOMEvent* aMouseEvent);
+NS_IMETHOD HandleEvent(nsIDOMEvent* aEvent);
+ private:
+PRBool IsValidDropData(nsIDOMDragEvent* aEvent);
You don't need this method to be private it should actually be a class
method (ie. static method). And you should return a |bool| instead of a
|PRBool|.
OK.
I'd like some more opinion of what I discussed here,
[Bug 131145]
Created attachment 534375 Patch 2 Sorry roc and jst but I won't be able to write a test for this. This is due to this line here under nsDomDataTransfer::GetFiles: if (mEventType != NS_DRAGDROP_DROP mEventType != NS_DRAGDROP_DRAGDROP) return NS_OK; The way we handle a dataTransfer in test drags is to capture it from a dragStart event, then use it in all other events. But, the dataTransfer saves its original event type (which is _START) and there is no way in JS to change this, so it triggers the early return. I assume it's like this for a reason, eg. so a page can't read the files on a dragover event. I did fix a couple bugs in finding this, but that's a whole day down the drain... -- You received this bug notification because you are a member of Ubuntu Desktop Bugs, which is subscribed to epiphany-browser in Ubuntu. https://bugs.launchpad.net/bugs/131145 Title: Dragging icon from Nautilus to HTML File Input box does not work -- desktop-bugs mailing list desktop-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/desktop-bugs
[Bug 131145]
I've decided to work on this, and have spent most of the day implementing a patch. -- You received this bug notification because you are a member of Ubuntu Desktop Bugs, which is subscribed to epiphany-browser in Ubuntu. https://bugs.launchpad.net/bugs/131145 Title: Dragging icon from Nautilus to HTML File Input box does not work -- desktop-bugs mailing list desktop-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/desktop-bugs
[Bug 131145]
Created attachment 533900 Patch Dragged this patch to the file upload control ;) jst, I think you're the most appropriate person for this? -- You received this bug notification because you are a member of Ubuntu Desktop Bugs, which is subscribed to epiphany-browser in Ubuntu. https://bugs.launchpad.net/bugs/131145 Title: Dragging icon from Nautilus to HTML File Input box does not work -- desktop-bugs mailing list desktop-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/desktop-bugs
