Closed Bug 508479 Opened 10 years ago Closed 10 years ago

HTML5 Drag and Drop: Drop event on elements that are not drop targets

Categories

(Core :: DOM: Drag & Drop, defect, P2, major)

1.9.1 Branch
defect

Tracking

()

RESOLVED FIXED
Tracking Status
status1.9.2 --- beta2-fixed

People

(Reporter: noel.gordon, Assigned: smaug)

References

Details

Attachments

(8 files, 2 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US) AppleWebKit/531.3 (KHTML, like Gecko) Chrome/3.0.193.2 Safari/531.3
Build Identifier: Build identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1.2) Gecko/20090729 Firefox/3.5.2 GTBDFff GTB6 (.NET CLR 3.5.30729)

Drag and drop external data (text,url,image,files) on a page element
that is not a drop target.  A drop event is sent to that element.


Reproducible: Always

Steps to Reproduce:
1. Select a file from the desktop.
2. Drag drop the file on a web page element that is not a drop target.

Actual Results:  
A drop event is sent to that web page element.

Expected Results:  
https://developer.mozilla.org/En/DragDrop/Drag_Operations#Performing_a_Drop

A drop event should only be sent to a drop target.  Per the above url,
a page element becomes a drop target if it cancels the default action
of the dragenter/dragover events.
 

FF3.5.2 XP SP3, FF3.5.2 Vista.
Attached file testcase
Component: General → Drag and Drop
Product: Firefox → Core
QA Contact: general → drag-drop
Version: unspecified → 1.9.1 Branch
Neil: Can you shed some light on this?

Noel: Do you have a pointer to the part of the HTML5 spec that describes what should happen here?
This happens because the browser content area accepts drops on it such as dropped links, files, etc.
Neil: do you mean that things work as per spec and that this bug is INVALID? Or that there's a bug but it's understood, i.e. this bug should be confirmed?
The spec doesn't mention how it interacts with the browser/platforms existing drag and drop functionality. That said, it does say that drags over a non-dom element are platform-specific which can imply that dragging over a browser can do whatever is necessary.

On the other hand, having the drop event fire anyway could be inconvenient, but I wouldn't know how this could be changed. Possibly some flag during event handling.
I don't think the fact that we implement firefox using a DOM allows us to behave any differently. What the spec describes is what the webpage should see in order for browsers to behave uniformly. Marking confirmed based on that.

I suspect we'll want to fix this pretty quickly in order to prevent people from coming to depend on not having to call preventDefault on dragenter/dragover events.

Neil: would you have the cycles to fix this for 1.9.2?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking1.9.2+
OS: Windows Vista → All
Hardware: x86 → All
-> me
Assignee: nobody → honzab.moz
Status: NEW → ASSIGNED
Attached patch wip1 (obsolete) — Splinter Review
This is proposed fix. When we do not prevent default handling of ondragenter or ondragover, we do not reset canDrop attribute of the drag service explicitly.
I forgot to say, I need some help here to provide an automated test. using syntesizeMouse doesn't do the right thing for for me... Any ideas?
Thanks, I know this function. I was trying to do almost the same. I'll try to do EXACTLY the same.
Sorry, I spoke too soon, that function doesn't work, at least until bug 512347 is sorted out.
(In reply to comment #10)
> EventUtils.synthesizeDrop ?
> 
> http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/EventUtils.js#468

Unfortunately I think that function is totally broken. Feel free to try to use it and see if you have better luck, but be prepared for it not working :(

Fixing it would be great of course :)
(In reply to comment #13)
> Unfortunately I think that function is totally broken. Feel free to try to use
> it and see if you have better luck, but be prepared for it not working :(

What's wrong with it? Apart from the minor api change incompatibility, I just tested it and it worked fine.
If it works for you that's great. It could definitely have been something minor that was broken before.
The patch is wrong anyway, so testing it isn't really necessary ;)

Dragging a link onto the content area fails with this patch. This is because it changes to unconditionally call SetCanDrop overriding any other callers -- such as the content area drop handler which still uses the old api.
Neil: synthesizeDrop didn't work for me when I tried it, I always got "move" returned, which is what is set (??) in the dragstart handler (trapDrag). I think the drag-drop api want a:

synthesizeDrop(in nsIContent aTo, in nsIDOMTransferable aTransfer, in void *aDropCallback)

type of function, so that drag'n'drop can be reasonably tested. The idea is that the callback gets called (or maybe just return the event) last, after all drop handlers have been called.
(In reply to comment #16)
> The patch is wrong anyway, so testing it isn't really necessary ;)
> 
It will be when the patch gets ok ;)
> Dragging a link onto the content area fails with this patch. This is because it
> changes to unconditionally call SetCanDrop overriding any other callers -- such
> as the content area drop handler which still uses the old api.

Ok, I was thinking about it. If that is a requirement I'll have a new patch for this.
As I understand the content area drop handler is here: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#5009. It calls prevent default on the event, so it should 'eat' it. 

I'll need a bit guidance about 'the old API'.
The content drag/drop handler is actually in nsContentAreaDragDrop.cpp.

The one you linked is a different handler, that for whatever reason, the browser implemented separately for links, and it only handles links.
Priority: -- → P2
Ok, I have spent some time on this w/o a luck. 

First, what to do when more then a single element contained each in other handles ondragenter/ondrop but only one of them calls preventDefault? We have no way to recognize which element canceled the event, we just know the first that did it. Do we have to send ondrop then to this and all following elements? To all elements that define it but actually may have not called preventDefault on the event? It's unclear.

Second, the default browser handler was really where I originally said: browser/base/content/browser.js: contentAreaDNDObserver.onDrop. This defines a JS listener that is handled absolutely the same way as any other listener defined by content. It is invoked from nsEventListenerManager::HandleEventSubType with all other event listeners like this: some html element, html body, the browser xul window. The code in nsContentAreaDragDrop.cpp is called but never executed because it is always missing mNavigator (see the method).

As discussed with bz, he suggested to have a hash set in the drag session that will be filled with all nsIContent pointers after event got preventDefault call. When invoking ondrop all elements will be checked against this hash set, if not contained, get no ondrop. However, when there is no element in the hash set (means none has canceled the event) we must call the browser default handler. How to recognize it?
And actually, contentAreaDNDObserver is violating the spec as it is not canceling the event in ondragenter. It should not be called at all as it is implemented now.

Some kind of wrapping the content handlers could be the solution:
- wrap ondragenter/over and alter the event with a new implementation giving us information that the event got called preventDefault and preserving the current value of that flag to let the handler correctly read it in case some other handler has already called preventDefault itself. remember if the element cancel the event in a hash set.

- wrap ondrop and just check the element is in the hash set. if not, do not call the real handler.

- then we must wipe the hash set before every ondragenter/over iteration because the spec says to take into account only the last call to ondragenter/over.
Attachment #395082 - Attachment is obsolete: true
If we get a preventDefault call, I think it's fine to fire the drop event to the node we fired the dragenter/dragover event. That event will eventually reach the element that called preventDefault.
(In reply to comment #21)
> First, what to do when more then a single element contained each in other
> handles ondragenter/ondrop but only one of them calls preventDefault? We have
> no way to recognize which element canceled the event, we just know the first
> that did it. Do we have to send ondrop then to this and all following elements?

Not sure what you're getting at here. The drop event fires at the element where the drop occured (where the mouse was released).

> Second, the default browser handler was really where I originally said:
> browser/base/content/browser.js: contentAreaDNDObserver.onDrop.

I was referring to the dragover handler.

> As discussed with bz, he suggested to have a hash set in the drag session that
> will be filled with all nsIContent pointers after event got preventDefault
> call. When invoking ondrop all elements will be checked against this hash set,
> if not contained, get no ondrop. However, when there is no element in the hash
> set (means none has canceled the event) we must call the browser default
> handler. How to recognize it?

Huh? All we need to know is whether content or chrome cancelled the event, and not fire the event at content listeners if content cancelled it.
(In reply to comment #24)
> Not sure what you're getting at here. The drop event fires at the element where
> the drop occured (where the mouse was released).

It may be more then one element. Imagine: <html><body ondrop="somecode1();"><div ondrop="somecode2();><span ondrop="somecode3();>Hallo world!</span></div></body></html>

Each browser xul window, body, div and span will get ondrop (ondragenter) when we release (move) the mouse over the span. It means 4 invocations.

> I was referring to the dragover handler.

Yes, sorry, you are right.

> Huh? All we need to know is whether content or chrome cancelled the event, and
> not fire the event at content listeners if content cancelled it.

Yes, we need to know if CHROME or CONTENT respectively canceled the event and then we need to know if we are calling the listener handler of CHROME or CONTENT. That is something I don't know how to recognize.

Ones again: contentAreaDNDObserver is handled (called from the event manager) the same way as content defined listeners. There is no way to figure out this is the default handler.

There should be a completely different way of handling default ondrop by chrome, probably a kind of a new method onDefaultDrop? That one would be called synthetically from dnd code when it has to be.

In a nut shell: let the dnd code remember the last ondragenter/over event has been canceled. When the dnd code is then invoking ondrop, check that flag. If it is true (means 'content want to get ondrop') invoke ondrop event, all content defined listeners will get its ondrop called. If the flag is false (none has called preventDefault) call our new event onDefaultDrop.

Makes sense?
(In reply to comment #25)
> It may be more then one element. Imagine: <html><body
> ondrop="somecode1();"><div ondrop="somecode2();><span
> ondrop="somecode3();>Hallo world!</span></div></body></html>
> 
> Each browser xul window, body, div and span will get ondrop (ondragenter) when
> we release (move) the mouse over the span. It means 4 invocations.
> 

But that's just normal event bubbling. If you drop on the <span>, the drop fires only once with a target of the <span>, and the listeners are called in turn.

> In a nut shell: let the dnd code remember the last ondragenter/over event has
> been canceled. When the dnd code is then invoking ondrop, check that flag. If
> it is true (means 'content want to get ondrop') invoke ondrop event, all
> content defined listeners will get its ondrop called. If the flag is false
> (none has called preventDefault) call our new event onDefaultDrop.

I think when dragoverEvent.preventDefault() is called, we cache whether the caller was content or chrome.

When it's time to fire drop, we check the flag. If it's 'content', then we fire the drop as normal. If 'chrome', we do some special event initialization which fires the event at the <browser> but sets the target to what it would have been.

We have some code that does something like this already I think.
(In reply to comment #26)
> We have some code that does something like this already I think.

Do you know where? Or at least the bug where this has been added? I cannot find it.
Just a note that the patch passed the try server run on all platforms w/o any failure. Could the failure after I have landed be some random failure?
I don't think we have a whole lot of tests for drag-n-drop unfortunately :(
(In reply to comment #28)
> Just a note that the patch passed the try server run on all platforms w/o any
> failure. Could the failure after I have landed be some random failure?

Sorry! Wrong bug! Ignore please...
As there is no feedback I have two options to fix this:

1. add the code that recognize chrome from content
2. add a new event onDefaultDrop handled by contentAreaDNDObserver

I'll try the former as the first way, if I fail, I'll do the letter one.
Attached patch wip 2 (obsolete) — Splinter Review
much better patch, allows chrome default drop handling. the test doesn't work on linux (centos 5). it ends with dataTransfer undefined on EventUtils.js:513. I did't try the test on mac yet.

I change msIDragService interface, we'll need _1_9_2 interface for this.
Attached patch v1Splinter Review
Regular version, test passes and works on all 3 platforms. I had to enlarge the syntesizeDrop's drag threshold from 8 pixels to 18. Linux needed that.
Attachment #406010 - Attachment is obsolete: true
Attachment #406237 - Flags: review?(enndeakin)
(In reply to comment #33)
> Created an attachment (id=406237) [details]
> v1
> 
> Regular version, test passes and works on all 3 platforms. I had to enlarge the
> syntesizeDrop's drag threshold from 8 pixels to 18. Linux needed that.

I forgot to change the IID of nsIDragService, please review w/o it, I'll add that change before commit.
Could the test case be expanded to also test a link (URL) drag drop?
Comment on attachment 406237 [details] [diff] [review]
v1

I'm missing something here. This patch doesn't even fix this bug, and the included test passes even without the changes applied.

Nor do I see how this patch would work. Can you explain what you expect this to be doing?
Attachment #406237 - Flags: review?(enndeakin) → review-
(In reply to comment #36)
> (From update of attachment 406237 [details] [diff] [review])
> I'm missing something here. This patch doesn't even fix this bug,

Using the test cases to verify for this patches fixes the bug.

> and the
> included test passes even without the changes applied.
> 

That's true, just verified that. It is strange, have to recheck the test.

> Nor do I see how this patch would work. Can you explain what you expect this to
> be doing?

The problem is that ondrop of any content handler must not be called when none of content handlers has not canceled its ondragenter/over events (didn't call preventDefault() on the event).

What the patch is doing (should be doing) is to set a flag saying if the last ondragover/enter got preventDefault() call or not, and if NOT, then prevent invocation of any ondrop defined by content. NS_EVENT_FLAG_NO_CONTENT_DISPATCH flag on the event is ensuring that.

If you see a problem with this or any missing piece in it, please let me know.

I'll recheck the test now, seems it is not sufficient to check this bug.
OK. The test is wrong. After I have a closer looked at synthesizeDrop I realized it is completely useless for me. It synthetically (not by mouse primitive events emulation) fires events like ondragover/ondrop and it's programmed to do not call ondrop when there is no drop effect allowed, that is not when ondragover doesn't cancel the event. Existing code in nsEventStateManager is already doing this, therefor the test is passing even w/o the patch. This existing code is not behaving sufficiently for external links (like dragging larry from the URL bar); setting drop effect to NONE is not enough to prevent ondrop invocation of content handlers in that case.

I have absolutely no idea how to automate such a test. Also answering noel's question...

However, I stand my opinion the patch is fixing this bug.
I tried it with a simple testcase:

<html><body ondrop="alert('a')">This is fun</body></html>

Dragging text from the url bar onto the page showed the alert.
Neil, and the test case (the first attachment) works for you?
(In reply to comment #39)
> I tried it with a simple testcase:
> 
> <html><body ondrop="alert('a')">This is fun</body></html>
> 
> Dragging text from the url bar onto the page showed the alert.

NS_EVENT_FLAG_NO_CONTENT_DISPATCH doesn't prevent event fire in this case. Is it a bug or is it intentional?
You can ask Olli about why my test doesn't work.

The main thing I don't understand about this patch is that it checks that preventDefault was called, but doesn't seem to check who did so.

If content calls preventDefault, the drop event should fire at content and bubble up to chrome.
If chrome calls preventDefault, the drop event should *not* fire at content, but *should* fire at chrome.
If noone calls preventDefault, the drop event should not fire at all.
About NS_EVENT_FLAG_NO_CONTENT_DISPATCH, see Bug 329119.

I don't quite understand the patch. As Neil said, if content calls
preventDefault, drop should fire at content.
> If content calls preventDefault, the drop event should fire at content and
> bubble up to chrome.
> If chrome calls preventDefault, the drop event should *not* fire at content,
> but *should* fire at chrome.
> If noone calls preventDefault, the drop event should not fire at all.

Also, this should include the old-style way of calling SetCanDrop which should be equivalent to chrome calling preventDefault.
I base my patch on fact that chrome (browser.xul) doesn't do ondragover = function(e) {e.preventDefault()} at all, at least in most cases. So, it should not almost ever get ondrop by the spec. It is another bug.

(In reply to comment #43)
> About NS_EVENT_FLAG_NO_CONTENT_DISPATCH, see Bug 329119.
> 

I has already been looking at that. I'm not sure it's related to this bug, it's old and w/o any activity, should we block on it?

> I don't quite understand the patch. As Neil said, if content calls
> preventDefault, 

That is indicated by (nsEventStatus_eConsumeNoDefault == *aStatus) in nsEventStateManager, I set on the drag session its flag mOnlyDefault to (nsEventStatus_eConsumeNoDefault _!=_ *aStatus). It means the flag is false when preventDefault has not been called.

> drop should fire at content.

..and if the flag is set, I set the NS_EVENT_FLAG_NO_CONTENT_DISPATCH that, as I would expect, and what in case of AddEventListener usage works, should prevent invocation of ondrop in content.

It is a hack, yes, and obviously I don't know the API that well to solve this bug.

What I have to know is:
- to recognize who called preventDefault. It should probably happen at nsEventListenerManager::HandleEvent?
- target only chrome handlers (event group), when only and only chrome called preventDefault

All this should be established by setting flags (probably new) on an event, the only way to carry such info through the generic APIs of nsEventXXXManager.
(In reply to comment #45)
> It means the flag is false when preventDefault has not been called.
> 

Sorry: It means the flag is false when preventDefault HAS BEEN called.
(In reply to comment #45)
> What I have to know is:
> - to recognize who called preventDefault. It should probably happen at
> nsEventListenerManager::HandleEvent?

Maybe in nsDOMEvent::PreventDefault. Check the current target and see if that
is in chrome?

> - target only chrome handlers (event group)
what you mean with (event group)? Usually chrome uses default event group.

Perhaps we need to have NS_EVENT_FLAG_ONLY_CHROME_DISPATCH
(and eventually remove NS_EVENT_FLAG_NO_CONTENT_DISPATCH).
Assignee: honzab.moz → Olli.Pettay
Attached file testcase 2
Form controls make this even more trickier.
IE does fire drop on containing div, so does gecko, but webkit doesn't.

I posted an email to whatwg list. Need to get this resolved in the spec before
committing any patch.
Even trickier, consider <textarea>, contentEditable=true, <iframe> ...
Why would iframe be trickier? events don't bubble from iframe to content window..

<textarea> is a form control.

contentEditable should behave probably like form controls.
I'm not convinced we should fix this for 1.9.2, since this may
cause regressions in web content, FF UI and extensions.
And HTML5 draft doesn't yet specify this all properly.
I'm worried that if we don't fix this now, we'll never be able to. It seems very very easy for pages to come to depend on the way things work now.

This is especially bad since we'll be pushing file drag'n'drop which I suspect some high-profile sites might start using even though it doesn't work cross browser yet.
The one problem is that not everything is properly defined in HTML5.

But ok, I'll try to come up with some solution for 1.9.2.
(In reply to comment #37)
> > and the
> > included test passes even without the changes applied.
> 
> That's true, just verified that. It is strange, have to recheck the test.

The problem is that synthesizeDrop uses just normal dom event dispatch.
Something new is needed in nsIDOMWindowUtils.
Attached patch patchSplinter Review
Needs automated tests
See you've been busy, let me catch up.

> Comment #54
> Why would iframe be trickier? events don't bubble from iframe to
> content window..

That's the theory and it's my belief too -- events should not bubble
from an <iframe> to the surrounding content.  What's tricky is all the
element re-targeting code used for drag & drop event dispatch.  Get
that code wrong near an <iframe>, and you can very easily break this
no-event-bubbling <iframe> rule.  http://crbug.com/22472 refers.

An automated drag drop test for <iframe> is all that's needed to test
that this important rule is never broken when someone modifies the drag
drop event re-targeting/dispatch code.  Perhaps there's test for that
in gecko already?  webkit will have one shortly.


> <textarea> is a form control.
> contentEditable should behave probably like form controls.

And there's designMode too :).  And if by proper behavior you mean let
the drag drop events bubble out of these elements when they are dragged
over, I'm not sure I agree.

These elements have default drag drop behaviors that automatically set
the drop cursor effect.  Page javascript has no control over that and it
usually doesn't matter.  However, when these elements are direct children
of parent elements that are also drop targets, bubbling is not such a
great idea if the parent can't accept a drag data format that the child
element does.

Simple example: wrap an <input type="text"> inside a <div> that has drag
drop handlers that only accept links.  Drag "text" over the <input>.  The
cursor is automatically set to + (copy effect) by the browser.  The event bubbles into the parent <div>, which cancels the event and sets the drag
cursor to the none effect (it only wants links).  What does the user see?
Drag cursor flash, since the cursor oscillates between the copy and none
effects while the user drags over the <input> element.  Not so good.

Webkit makes things easy here, it doesn't allow the event bubble since the
<input> can handle the drop (copy effect) by default; no cursor flash.  On gecko/IE, the parent must take remedial action: check if the event target
is an <input>, <textarea>, contentEditable ... any element that has an in-
built drag drop behavior.  If so, stopPropagation() and exit the event
handler.  Otherwise cancel the event's default action, set the drop effect
(copy or none depending on the drag data) and stopPropagation().

To me this is a lot of extra work, given I must stopPropagation() in any
case.  Perhaps there's some other way to prevent drag cursor flash on IE
or gecko, I'm open to suggestions, but the webkit way seems right to me
and it handles the drop-zones-inside-drop-zones case well.  The app I'm
working on (wave) has this use-case.


> Comment #54
> The problem is that synthesizeDrop uses just normal dom event dispatch.
> Something new is needed in nsIDOMWindowUtils.

"Something new" yeap, I know that pain.  A lot of work was done in webkit
June-July to update their test system to support automatic testing of
external drag drop of Text/URL/Files onto the content area.  HTML5 File
drag drop shipped in Safari 4.0.3 in August.


> Comment #55
> patch, Needs automated tests

Cool to see this, and I think HTML5 is a good guide here.  When there are
no drop targets in the content area, the drop event is not allowed.  What
event should the content area see?  Answ: dragleave, according to HTML5,
also IE/Safari/Chrome.  A test for that would be good-to-have.
Attached patch patchSplinter Review
Tested on linux. Posted the patch to tryserver.

Calling preventDefault in nsContentAreaDragDrop only when target is content so
that we change the behavior only for content (for now). Similar checks in ESM for
the same reason.
I'll file a followup to make this all work properly on chrome, but that isn't
something for 1.9.2.

nsDOMEvent::PreventDefault() is a bit hackish, but I can't really see anything
better.

NS_EVENT_FLAG_ONLY_CHROME_DISPATCH and changes to nsEventDispatcher are quite
nice and could be used also in other cases.

nsDOMWindowUtils::DispatchDOMEventViaPresShell is just for testing.
Attachment #408088 - Flags: superreview?(jonas)
Attachment #408088 - Flags: review?(enndeakin)
And BTW, HTML5 was updated and what webkit does with form controls is wrong.
Let's not rush things, I found this testcase:

<html>
 <body ondrop="alert('this is fun');">
  <textarea cols="80" rows="4">Drop on me</textarea>
 </body>
</html>
I didn't say gecko does it right ;)
That is a different bug, related to bubbling.
I'll fix that for 1.9.3.
(In reply to comment #60)
> That is a different bug, related to bubbling.
related to drop bubbling from form elements.
Comment on attachment 408088 [details] [diff] [review]
patch

>+  PRBool isChrome =
>+    node ? nsContentUtils::IsChromeDoc(node->GetOwnerDoc()) : PR_FALSE;
>+  if (isChrome) {
>+    session->SetCanDrop(dropAllowed);
>+  } else if (dropAllowed) {
>+    inEvent->PreventDefault();

Why does this need to be different? This is a dragover listener attached to <browser>.


> NS_IMETHODIMP
> nsDOMEvent::PreventDefault()
> {
>   if (!(mEvent->flags & NS_EVENT_FLAG_CANT_CANCEL)) {
>     mEvent->flags |= NS_EVENT_FLAG_NO_DEFAULT;
>+
>+    // Need to set an extra flag for drag events.
>+    if (mEvent->eventStructType == NS_DRAG_EVENT &&

Would it be easier to just override this with nsDOMDragEvent::PreventDefault?

You might want to instead check if this is NS_DRAGDROP_ENTER or NS_DRAGDROP_OVER
as this isn't needed for other drag events.


>diff --git a/content/events/src/nsEventStateManager.cpp b/content/events/src/nsEventStateManager.cpp
...
>+        // For now, do this only for dragover.
>+        //XXXsmaug dragenter needs some more work.
>+        if (aEvent->message == NS_DRAGDROP_OVER && !isChromeDoc) {
>+          // Someone has called preventDefault(), check whether is was content.
>+          dragSession->SetOnlyChromeDrop(
>+            !(aEvent->flags & NS_EVENT_FLAG_CONTENT_NO_DEFAULT));
>+        }

What is harder about dragenter events?

Also in the comment:

check whether *it* was content


>diff --git a/dom/interfaces/base/nsIDOMWindowUtils.idl b/dom/interfaces/base/nsIDOMWindowUtils.idl
...
> 
> [scriptable, uuid(4171ea1a-3752-4bc3-8c66-1b2936ecde7a)]

update?


>+// Use this flag if the event should be dispatched only to chrome.
>+#define NS_EVENT_FLAG_ONLY_CHROME_DISPATCH 0x2000
>+
>+// A flag for drop event handling.
>+#define NS_EVENT_FLAG_CONTENT_NO_DEFAULT   0x4000
>+

This is an odd name for the flag. If it's just for drop event handling maybe
the name should say that. Otherwise, the comment should be clearer as to what it
actually does.
Attachment #408088 - Flags: review?(enndeakin) → review+
(In reply to comment #62)
> Why does this need to be different? This is a dragover listener attached to
> <browser>.
A <browser> can load chrome documents, right? So I really want to change only
content handling.

> Would it be easier to just override this with nsDOMDragEvent::PreventDefault?
I don't really mind here. Will do that.
 
> What is harder about dragenter events?
See the spec. It has special handling for example for body.
For that I'll file a new bug.

> >+// A flag for drop event handling.
> >+#define NS_EVENT_FLAG_CONTENT_NO_DEFAULT   0x4000
> >+
> 
> This is an odd name for the flag. If it's just for drop event handling maybe
> the name should say that. Otherwise, the comment should be clearer as to what
> it
> actually does.
I could improve the comment. And/Or change the flag name to
NS_EVENT_FLAG_NO_DEFAULT_WAS_CALLED_IN_CONTENT or something.
Jonas, review?
(In reply to comment #63)
> > Would it be easier to just override this with nsDOMDragEvent::PreventDefault?
> I don't really mind here. Will do that.
> 
Actually, because of the way we forward method calls to nsDOMEvent,
changing nsDOMEvent::PreventDefault is much easier.
Attachment #408088 - Flags: superreview?(jonas) → superreview+
http://hg.mozilla.org/mozilla-central/rev/674972c0556a
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
(In reply to comment #63)
> A <browser> can load chrome documents, right? So I really want to change only
> content handling.

What happens when a chrome document loaded in a content window does or doesn't call preventDefault?
 
> > >+// A flag for drop event handling.
> > >+#define NS_EVENT_FLAG_CONTENT_NO_DEFAULT   0x4000
> > >+

What I meant here is that comment says 'for drop event handling', but the flag is named 'NS_EVENT_FLAG_CONTENT_NO_DEFAULT' which doesn't imply anything to do with drop events.
(In reply to comment #67)
> What happens when a chrome document loaded in a content window does or doesn't
> call preventDefault?
That is not the right question.
Question is what happens when a chrome document is loaded to a chrome <browser>.
And that is something I didn't want to change for 1.9.2.

> What I meant here is that comment says 'for drop event handling', but the flag
> is named 'NS_EVENT_FLAG_CONTENT_NO_DEFAULT' which doesn't imply anything to do
> with drop events.
Well, the comment says what it is used currently for.
The flag name would be terribly long it if it would be something like
NS_EVENT_FLAG_NO_DEFAULT_CALLED_ON_A_DROP_EVENT_ON_CONTENT.
If you have a good suggestion for that flag name, I could change it.
(In reply to comment #68)
> (In reply to comment #67)
> > What happens when a chrome document loaded in a content window does or doesn't
> > call preventDefault?
> That is not the right question.

It is the question I asked though, and would like to know the answer to that.
Well, if you load something which has chrome privileges to the content window,
it has the chrome behavior.
And note, it has chrome behavior everywhere, in ESM, in nsDOMEvent etc. The patch shouldn't change it at all.
Attached patch for 1.9.2Splinter Review
This adds _1_9_2 interfaces.
Hmm side-effects (526286).  Thanks for getting this done Olli, but I need to
ask.  Did this change affect the behavior of URL and File drag drops?  I saw
no tests for that, but y'know, side-effects ;)
This certainly has "side effects" because this changes our behavior ;)
Bug 526286 is 'just' a regression. 

I'll change the drag&drop handling in chrome, but only in 1.9.3.
I just tested URL and File drag&drop, and no change there.
But that is something where automated tests would be great.
Yup, understood the regression, you get that sometimes, and drag & drop is
trickey so automated test sure help us sleep at night.

My point was when I write cross-browser HTML5 drag drop js, the drop event
handler is always written as follows:

 function handleDrop(event) {
   var evt = event || window.event;

   // Process the drop data ...

   // FF chrome overrides the drop target and navigates URL/File drops
   // even though our target said it would handle this drop! Stop that
   // via an evt.stopPropagation().

   if (navigator.userAgent.match(/Firefox\//))
     stopPropagation(evt);
 }

I was hoping your change meant that I wouldn't need to do this anymore ie.,
chrome no longer overrides content area drop targets.
Oh, that is very a different bug. Please file a new one.
Though, we do check if someone has called preventDefault on drop event and
don't process those drop events.
Do you call preventDefault in your drop handler?
And indeed HTML5 draft has still bugs related to drop event handling.
I'll write to what wg mailing list.
>    // FF chrome overrides the drop target and navigates URL/File drops
>    // even though our target said it would handle this drop! Stop that
>    // via an evt.stopPropagation().
> 
>    if (navigator.userAgent.match(/Firefox\//))
>      stopPropagation(evt);
>  }

Calling preventDefault instead should prevent this. And I see no reason for either call to be Firefox specific.
> Though, we do check if someone has called preventDefault on drop event and
> don't process those drop events.

Yes, and that'd make sense to me for elements whose default action is to drop (<textarea>, form <input>, and editable elements).  It is important that application designers have this level of control over such elements, especially editable elements, since their default drop behavior is sub-optimal at times.

But for all other elements, it's somewhat contradictory.  The drop event is
fired at a DOM target with drop data present in event.dataTransfer. That drop data can be stored anywhere, right?  So I don't think you can really prevent that, and not with preventDefault since it has a different role to play ...


> Do you call preventDefault in your drop handler?

The meaning ascribed to preventDefault in a drop event is mentioned in three separate places in HTML5 Section 7.9 Drag and Drop.  That meaning is not to prevent default browser navigation -- it is instead used to allow a drop target to signal the final dropEffect back to the drag source.  The dropEffect is allowed to the change during a drop event, for good reason, and preventDefault is reserved to indicate that it was changed.

So to answer your question: yes I call preventDefault if I want to communicate with the drag source. Since that's an advanced use of HTML5 Drag and Drop, then for the typical case, no, it's not needed, so I don't call it in the drop event.
> And indeed HTML5 draft has still bugs related to drop event handling.
> I'll write to what wg mailing list.

Maybe so, but it’s clear about to whom and when you fire the drop event. You fire the drop event to the content area drop target, or to the chrome, but
not both:

 “2. Otherwise, the drag operation was as success. If the current target
 element is a DOM element, the user agent must fire a drop event at it;
 otherwise, it must use platform-specific conventions for indicating a drop.”

Perhaps "indicating a drop" is not the best choice of words, maybe I’d prefer "completing the drop", but it sure says “otherwise, …”, which is the precise reason for the current bug.
>>    // FF chrome overrides the drop target and navigates URL/File drops
>>    // even though our target said it would handle this drop! Stop that
>>    // via an evt.stopPropagation().
>> 
>>    if (navigator.userAgent.match(/Firefox\//))
>>      stopPropagation(evt);
>>  }

> Calling preventDefault instead should prevent this. And I see no reason for
> either call to be Firefox specific.

A drop event preventDefault has a different meaning in the HTML5 as mentioned
above.  Typically, it's not called, and I'd prefer not to call stopPropagation
in some cases.

Perhaps the simple thing to do is remove the stopPropagation also, and test cross-browser without any Firefox specific code ...
Tested File and URL drops on IE6 XP3, IE7/8 Vista, Chrome 3 XP3/Vista, Safari 4.0.3 XP3, Firefox 3.5.4 XP3, Firefox 3.6b1 XP3/Vista.  Only Firefox navigates
on a drop event.
It would be great if someone could make it clear what the remaining problems are. Especially since this bug is now marked FIXED.

Are there more things we should fix in order to avoid people coming to depend on faulty behavior?
(In reply to comment #86)
> It would be great if someone could make it clear what the remaining problems
> are. Especially since this bug is now marked FIXED.
For the remaining bugs (which are in implementations and in HTML5 draft) new bugs
should be filed.
You need to log in before you can comment on or make changes to this bug.