Closed
Bug 539476
Opened 14 years ago
Closed 14 years ago
Dragging an image into designMode iframe can't be prevented
Categories
(Firefox :: General, defect)
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
blocking2.0 | --- | beta1+ |
People
(Reporter: mdavids, Assigned: enndeakin)
References
()
Details
(Whiteboard: [evang-wanted])
Attachments
(1 file, 2 obsolete files)
12.13 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US) AppleWebKit/532.8 (KHTML, like Gecko) Chrome/4.0.288.1 Safari/532.8 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2) Gecko/20100105 Firefox/3.6 GTB6 (.NET CLR 3.5.30729) Using the new drag/drop file handling in FF 3.6, if you drag an image file into an iframe in designMode, the HTML is modified to include an <img> with src of the file:/// URL of the image you dragged. This cannot be prevented. Reproducible: Always Steps to Reproduce: 1. Drag an image from the FS into the frame. 2. Notice the alert (which is the custom drag/drop handling) Actual Results: There is an <img> added to the HTML of the frame (verifiable in Firebug). Expected Results: There should be no <img> added since the script calls preventDefault()
Reporter | ||
Updated•14 years ago
|
blocking2.0: --- → ?
Neil, is part of your drag'n'drop code? Or is this a bug somewhere in editor? This is sort of a big deal since it means that you can't use our new-fangled File drag'n'drop support doesn't work with rich text editing.
Assignee | ||
Comment 2•14 years ago
|
||
Two issues here: 1. The editor drop event listener is firing before the one added by the testcase. 2. The editor drop event listener doesn't check if the event has been cancelled. The editor should really be handing everything as part of the default phase of the event handling, or it should add the listener in the system event group. As a workaround, change: target.addEventListener('drop', drop, false); to: target.documentElement.addEventListener('drop', drop, false); such that this listener occurs before the editor one.
Reporter | ||
Comment 3•14 years ago
|
||
In the code I'm working on we have a contentEditable body in an iframe, so the timing might be different there. I'll try the workaround in the meantime.
Neil, could you take this bug on?
Assignee | ||
Updated•14 years ago
|
Assignee: nobody → enndeakin
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
blocking2.0: ? → beta1
Updated•14 years ago
|
Whiteboard: [evang-wanted]
Assignee | ||
Comment 6•14 years ago
|
||
Assignee | ||
Comment 7•14 years ago
|
||
Attachment #427639 -
Attachment is obsolete: true
Assignee | ||
Updated•14 years ago
|
Attachment #434921 -
Flags: review?(Olli.Pettay)
Comment 8•14 years ago
|
||
Comment on attachment 434921 [details] [diff] [review] version 2 >diff --git a/editor/libeditor/base/nsEditor.cpp b/editor/libeditor/base/nsEditor.cpp >--- a/editor/libeditor/base/nsEditor.cpp >+++ b/editor/libeditor/base/nsEditor.cpp >@@ -364,26 +364,36 @@ nsEditor::InstallEventListeners() > NS_GET_IID(nsIDOMTextListener)); > > rv |= piTarget->AddEventListenerByIID(mEventListener, > NS_GET_IID(nsIDOMCompositionListener)); > > nsCOMPtr<nsIDOMEventTarget> target(do_QueryInterface(piTarget)); > if (target) { > // See bug 455215, we cannot use the standard dragstart event yet >- rv |= target->AddEventListener(NS_LITERAL_STRING("draggesture"), >- mEventListener, PR_FALSE); >- rv |= target->AddEventListener(NS_LITERAL_STRING("dragenter"), >- mEventListener, PR_FALSE); >- rv |= target->AddEventListener(NS_LITERAL_STRING("dragover"), >- mEventListener, PR_FALSE); >- rv |= target->AddEventListener(NS_LITERAL_STRING("dragleave"), >- mEventListener, PR_FALSE); >- rv |= target->AddEventListener(NS_LITERAL_STRING("drop"), >- mEventListener, PR_FALSE); >+ rv |= elmP->AddEventListenerByType(mEventListener, NS_LITERAL_STRING("draggesture"), >+ NS_EVENT_FLAG_BUBBLE | >+ NS_PRIV_EVENT_UNTRUSTED_PERMITTED, >+ sysGroup); >+ rv |= elmP->AddEventListenerByType(mEventListener, NS_LITERAL_STRING("dragenter"), >+ NS_EVENT_FLAG_BUBBLE | >+ NS_PRIV_EVENT_UNTRUSTED_PERMITTED, >+ sysGroup); >+ rv |= elmP->AddEventListenerByType(mEventListener, NS_LITERAL_STRING("dragover"), >+ NS_EVENT_FLAG_BUBBLE | >+ NS_PRIV_EVENT_UNTRUSTED_PERMITTED, >+ sysGroup); >+ rv |= elmP->AddEventListenerByType(mEventListener, NS_LITERAL_STRING("dragleave"), >+ NS_EVENT_FLAG_BUBBLE | >+ NS_PRIV_EVENT_UNTRUSTED_PERMITTED, >+ sysGroup); >+ rv |= elmP->AddEventListenerByType(mEventListener, NS_LITERAL_STRING("drop"), >+ NS_EVENT_FLAG_BUBBLE | >+ NS_PRIV_EVENT_UNTRUSTED_PERMITTED, >+ sysGroup); > } > Do we really want to allow untrusted events? I guess that depends on other browser, do they support dispatching drag events to contentEditable or designMode? > nsresult > nsEditorEventListener::DragOver(nsIDOMDragEvent* aDragEvent) > { I wonder if there should be a check somewhere here that whether preventDefault() was called. >+ >+ // If there is no source node, this is an external drag and the drop is allowed. Always? Even if script dispatches the event?
Assignee | ||
Comment 9•14 years ago
|
||
> Do we really want to allow untrusted events? Probably not, but I'll test some more. > > nsresult > > nsEditorEventListener::DragOver(nsIDOMDragEvent* aDragEvent) > > { > I wonder if there should be a check somewhere here that whether > preventDefault() was called. Sounds like a good idea. > >+ > >+ // If there is no source node, this is an external drag and the drop is allowed. > Always? Even if script dispatches the event? Perhaps not, but it wouldn't be a security issue. The remaining checks are only relevant when there is a source node of the drag (that is, they prevent drops on the start of the drag). I'll clarify the comment.
Comment 10•14 years ago
|
||
Comment on attachment 434921 [details] [diff] [review] version 2 I assume there is a new patch coming.
Attachment #434921 -
Flags: review?(Olli.Pettay) → review-
How does the patch in bug 512717 affect this bug?
Assignee | ||
Comment 12•14 years ago
|
||
Attachment #434921 -
Attachment is obsolete: true
Attachment #438078 -
Flags: review?(Olli.Pettay)
Comment 13•14 years ago
|
||
Comment on attachment 438078 [details] [diff] [review] address comments r+, but this really needs tests.
Attachment #438078 -
Flags: review?(Olli.Pettay) → review+
Assignee | ||
Comment 14•14 years ago
|
||
Tests are dependent on editor using the new api (bug 499008).
Comment 15•14 years ago
|
||
OK. Please add tests somewhere.
Assignee | ||
Comment 16•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/701fd8e1377c
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•14 years ago
|
Flags: in-testsuite?
You need to log in
before you can comment on or make changes to this bug.
Description
•