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)

x86
Windows XP
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- beta1+

People

(Reporter: mdavids, Assigned: enndeakin)

References

()

Details

(Whiteboard: [evang-wanted])

Attachments

(1 file, 2 obsolete files)

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()
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.
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.
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: nobody → enndeakin
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Whiteboard: [evang-wanted]
Depends on: 545714
Blocks: 554400
Attached patch version 2 (obsolete) — Splinter Review
Attachment #427639 - Attachment is obsolete: true
Attachment #434921 - Flags: review?(Olli.Pettay)
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?
> 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 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?
Attached patch address commentsSplinter Review
Attachment #434921 - Attachment is obsolete: true
Attachment #438078 - Flags: review?(Olli.Pettay)
Comment on attachment 438078 [details] [diff] [review]
address comments

r+, but this really needs tests.
Attachment #438078 - Flags: review?(Olli.Pettay) → review+
Tests are dependent on editor using the new api (bug 499008).
OK. Please add tests somewhere.
http://hg.mozilla.org/mozilla-central/rev/701fd8e1377c
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: