591249 - event dragleave not properly dispatched if drag-and-drop aborted with ESC within iframe

Status: RESOLVED FIXED

(Core :: DOM: Copy & Paste and Drag & Drop, defect)

Description

[446620]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.8) Gecko/20100722 Firefox/3.6.8 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.8) Gecko/20100722 Firefox/3.6.8 (.NET CLR 3.5.30729)

If a drag-and-drop session is aborted with the ESC key while the ghost being over a possible drop target in an iframe, the according dragleave event is not dispatched to the iframe at all.

Reproducible: Always

Steps to Reproduce:
1. Decompress attachment example.zip
2. Start file main.html with FF3.6
3. Drag something over the dotted square in the iframe - the background color of the square turns yellow.
4. Press ESC - the background color of the square keeps yellow because Javascript function onDragLeave() has not been called.
5. This works quite well if you start iframe.html instead and proceed as before.
Actual Results:  
The dragleave event is not dispatched to the appropriate handler in the iframe.

Expected Results:  
The dragleave event should be dispatched to the appropriate handler within the iframe. Instead, only the handler gets called that has been registered for the dragleave event of the top window.

Comment 1

[446620]

Attachment: Sample to reproduce this issue.

Comment 2

[Nickolay_Ponomarev]

I can't reproduce in Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2.8) Gecko/20100722 Firefox/3.6.8 or Mozilla/5.0 (Macintosh; Intel Mac OS X 10.5; rv:2.0b6pre) Gecko/20100904 Firefox/4.0b6pre when dragging the text from the page. Maybe windows-specific? What should be "something" in step 3?

Comment 3

[446620]

> Maybe windows-specific?
Same on Linux (Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.7) Gecko/20100723 Fedora/3.6.7-1.fc13 Firefox/3.6.7).

> What should be "something" in step 3?
Take e.g. part of the text on the sample html page (say: "Iframe"): drag it over the square -> the square's background turns yellow; hit Escape key -> background remains yellow (should change to white again).

Comment 4

[446620]

Checked with FF3.6.9 under Mac OS X 10.6: Nickolay is right, this issue is not reproducible with FF3.6 under Mac OS X (v10.6).

Comment 5

[Nickolay_Ponomarev]

Yeah, and I can reproduce on Windows: Mozilla/5.0 (Windows NT 6.0; rv:2.0b6pre) Gecko/20100908 Firefox/4.0b6pre

Comment 6

[Adam]

Alternate steps to reproduce:

1. Open bookmarks sidebar (Ctrl+B).
2. Drag a bookmark such that the drop marker shows in the sidebar.
3. Press Escape to cancel the drag.

The drop marker will remain in place on Windows and Linux, as the sidebar never received the final drag leave event to clear the marker.

Comment 7

[Adam]

Attachment: Make the LastDragOverFrame static for EventStateManager

The presContext used by GenerateDragDropEnterExit may differ for drag over and drag exit.  The latter is the root presContext on Linux, while the presContext for drag over is the one it's over.  This patch fixes that by making the LastDragOverFrame static.

The problem is likely the same on Windows, but haven't tested.  OS X seems to use the correct presContext for drag cancellations, but haven't tested that either.

Patch works on Linux except that the CSS pseudoselector :-moz-drag-over doesn't respond to the dragleave received from the drag cancellation.  That's probably a separate bug.

The test fails, though the drag starts successfully.  Is it possible to synthesize a drag cancellation?  I believe gtk needs to receive the keypress first and then passes along the drag event off that, so the synthesizeKey probably doesn't fit the bill.

Comment 8

[Timothy Nikkel (:tnikkel)]

smaug, review ping? Is this the right approach?

This patch also fixes bug 684431.

Comment 9

[Timothy Nikkel (:tnikkel)]

(And it's also green on try server.)

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Argh, I've totally missed noticing this. I tend to focus on my review queue, not so much
feedback queue. My bad.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 542596 [details] [diff] [review]
Make the LastDragOverFrame static for EventStateManager


>+        if (sLastDragOverFrame) {
>           //The frame has changed but the content may not have. Check before dispatching to content
>-          mLastDragOverFrame->GetContentForEvent(aPresContext, aEvent, getter_AddRefs(lastContent));
>+          sLastDragOverFrame->GetContentForEvent(aPresContext, aEvent, getter_AddRefs(lastContent));
Need to check if using sLastDragOverFrame and aPresContext makes sense, or should it be
sLastDragOverFrame and sLastDragOverFrame->PresContext();
(In case of nsImageFrame passing aPresContext could be actually wrong.
 I think we could get rid of passing prescontext to GetContentForEvent)


> 
>           FireDragEnterOrExit(aPresContext, aEvent, NS_DRAGDROP_EXIT_SYNTH,
>-                              targetContent, lastContent, mLastDragOverFrame);
>+                              targetContent, lastContent, sLastDragOverFrame);
Same here.


>-        if (mLastDragOverFrame) {
>+        if (sLastDragOverFrame) {
>           FireDragEnterOrExit(aPresContext, aEvent, NS_DRAGDROP_LEAVE_SYNTH,
>-                              targetContent, lastContent, mLastDragOverFrame);
>+                              targetContent, lastContent, sLastDragOverFrame);
And here.


>+      if (sLastDragOverFrame) {
>         nsCOMPtr<nsIContent> lastContent;
>-        mLastDragOverFrame->GetContentForEvent(aPresContext, aEvent, getter_AddRefs(lastContent));
>+        sLastDragOverFrame->GetContentForEvent(aPresContext, aEvent, getter_AddRefs(lastContent));
And here.


> 
>         FireDragEnterOrExit(aPresContext, aEvent, NS_DRAGDROP_EXIT_SYNTH,
>-                            nsnull, lastContent, mLastDragOverFrame);
>+                            nsnull, lastContent, sLastDragOverFrame);
>         FireDragEnterOrExit(aPresContext, aEvent, NS_DRAGDROP_LEAVE_SYNTH,
>-                            nsnull, lastContent, mLastDragOverFrame);
And here.

Comment 12

[Timothy Nikkel (:tnikkel)]

Yeah, looks like we can just get rid of the aPresContext argument.

Adam, did you want to do that or would you like me to?

Comment 13

[Adam]

Attachment: Remove the aPresContext argument for nsIFrame GetContentForEvent, pass correct nsPresContext to FireDragEnterOrExit

Thanks for the feedback and help.

Note that this patch has some extra, unchanged lines listed as changed (in the section marked "@@ -7096,20 +7096,20 @@ PresShell::HandleEventInternal(nsEvent*" for nsPresShell.cpp) for reasons I could not determine.

In removing the nsPresContext being passed to nsIFrame::GetContentForEvent, note that nsImageFrame was the standout that used it, so it now just looks it up when needed.

The nsPresContext passed to FireDragEnterOrExit now corresponds to the frame that it's dispatched to.

I'll take a look at the style not being cleared for the next revision of the patch.

Comment 14

[Timothy Nikkel (:tnikkel)]

Perhaps your text editor is changing line endings from unix to Windows/DOS style is the reason for the PresShell::HandleEventInternal changes?

The call to GetImageMap in nsImageFrame::GetContentForEvent actually doesn't need the prescontext argument either if you follow it all the way through. So if you wanted you could remove that (and related) too.

Comment 15

[Timothy Nikkel (:tnikkel)]

Comment on attachment 559649 [details] [diff] [review]
Remove the aPresContext argument for nsIFrame GetContentForEvent, pass correct nsPresContext to FireDragEnterOrExit

May as well just ask for review so this doesn't fall off the radar again.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 559649 [details] [diff] [review]
Remove the aPresContext argument for nsIFrame GetContentForEvent, pass correct nsPresContext to FireDragEnterOrExit


>-        mLastDragOverFrame = nsnull;
>+        sLastDragOverFrame->GetContentForEvent(aEvent, getter_AddRefs(lastContent));
>+
>+        nsPresContext *lastDragOverFramePresContext = sLastDragOverFrame->PresContext();
>+        FireDragEnterOrExit(lastDragOverFramePresContext,
>+                            aEvent, NS_DRAGDROP_EXIT_SYNTH,
>+                            nsnull, lastContent, sLastDragOverFrame);
>+        FireDragEnterOrExit(lastDragOverFramePresContext,
>+                            aEvent, NS_DRAGDROP_LEAVE_SYNTH,
>+                            nsnull, lastContent, sLastDragOverFrame);
nsRefPtr<nsPresContext> lastDragOverFramePresContext, since nothing makes sure otherwise that it is still
alive when FireDragEnterOrExit is called second time.


>+++ b/content/events/test/Makefile.in
>@@ -88,16 +88,18 @@ _TEST_FILES = \
> 		test_bug545268.html \
> 		test_bug547996-1.html \
> 		test_bug547996-2.xhtml \
> 		test_bug556493.html \
> 		test_bug574663.html \
> 		test_clickevent_on_input.html \
> 		test_bug593959.html \
> 		test_bug591815.html \
>+    test_bug591249.html \
>+         bug591249_iframe.html \
indentation?


>-  map = GetImageMap(aPresContext);
>+  nsPresContext* context = PresContext();
>+  map = GetImageMap(context);
Could you remove the prescontext param passed to GetImageMap and the presshell passed to
nsImageMap::Init. nsImageMap doesn't seem to use mPresShell for anything.

Comment 17

[Adam]

Attachment: Fix -moz-drag-over style removal (both cancel and drop), remove unneeded PresContext/PresShell, clean up some minor issues.

Thanks tn; the extra line changes are my editor changing from DOS to UNIX newlines.  

Thanks for the feedback smaug.  

The patch removes mPresShell from nsImageMap, and removes the PresContext from nsImageFrame::GetImageMap().

It also fixes the issues with the -moz-drag-over style:  

In the case of the drag cancel, the context/Event State Manager would differ.  Same problem that existed with LastDragOverFrame.  By making DragOverContent static, calls to nsEventStateManager::SetContentState() will properly modify/cancel the state.

In the case of drop, the drop calls nsEventStateManager::ClearGlobalActiveContent(), so adding a check for sDragOverContent can clear it there.  That method may need renaming, given that it previously only dealt with the Active state, not DragOver.  

Wondering whether should the test be changed to todo, removed, or left as-is?

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 559892 [details] [diff] [review]
Fix -moz-drag-over style removal (both cancel and drop), remove unneeded PresContext/PresShell, clean up some minor issues.

>-  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mDragOverContent);
This is actually a bit tricky. We need to at least release DragOverContent at right points, otherwise
we leaks document.

 
>-  if (mDragOverContent &&
>-      nsContentUtils::ContentIsDescendantOf(mDragOverContent, aContent)) {
>-    mDragOverContent = nsnull;
>+  if (sDragOverContent &&
>+      nsContentUtils::ContentIsDescendantOf(sDragOverContent, aContent)) {
>+    sDragOverContent = nsnull;

For performance reasons
if (sDragOverContent && sDragOverContent->GetOwnerDoc() == aContent->GetOwnerDoc() &&
    nsContentUtils::ContentIsDescendantOf(sDragOverContent, aContent)) {
  sDragOverContent = nsnull;
}


> nsEventStateManager::ClearGlobalActiveContent(nsEventStateManager* aClearer)
> {
>   if (aClearer) {
>     aClearer->SetContentState(nsnull, NS_EVENT_STATE_ACTIVE);
>+    if (sDragOverContent)
>+      aClearer->SetContentState(nsnull, NS_EVENT_STATE_DRAGOVER);
if (expr) {
  stmt;
}


>+  static nsCOMPtr<nsIContent> sDragOverContent;

Add something like
if (sDragOverContent && sDragOverContent->GetOwnerDoc() == mDocument) {
  sDragOverContent = nsnull;
}
to ~nsEventStateManager();

That should be enough to not leak document, at least I hope so.


> 		test_bug591815.html \
>+		test_bug591249.html \
>+		     bug591249_iframe.html \
indentation

Comment 19

[Adam]

Attachment: Remove leak risk by removing DragOverContent in destructor, and improve performance when removing content

Thanks smaug.

This patch fixes the leak and improves the check prior to clearing sDragOverContent in ContentRemoved().

> > 		test_bug591815.html \
> >+		test_bug591249.html \
> >+		     bug591249_iframe.html \
> indentation

The extra indentation was to match other tests; they indent support files in that way, presumably to indicate they are support files.  I've changed it, but will change it back if needed.  

Still wondering if the test itself is okay as is.

Comment 20

[Timothy Nikkel (:tnikkel)]

Is this ready for the try server and then landing if all goes well?

Comment 21

[Adam]

Thanks Timothy, it's ready for the try server.

Comment 22

[Timothy Nikkel (:tnikkel)]

So far it looks like it is failing the test you added. More results coming.

Comment 23

[Adam]

Attachment: Make the test todo() as it currently fails.

Sorry, forgot to get final review on the test.  

This patch makes the test use todo(), but if there's a way to make it a useful test, that would be preferred.  

As mentioned before, I'm pretty sure that there isn't a way at present to generate a drag cancellation that would allow this test to be useful.  Synthesizing VK_ESCAPE doesn't work because GTK+ normally takes that before it reaches the browser and converts it into a drag event.

Spent some time revisiting the test to look at possible ways to achieve the desired behavior, but I'm not aware of anything currently exposed to the test that would allow it to simulate canceling a drag.

Comment 24

[Mozilla RelEng Bot]

Try run for a8605b748266 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=a8605b748266
Results (out of 171 total builds):
    exception: 2
    success: 148
    warnings: 19
    failure: 2
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-a8605b748266

Comment 25

[Adam]

Attachment: Test cleanup: add ondragstart to drag target to ensure that the drag isn't held after the test ends.

Test still uses todo(), but the drag attempt was persisting beyond the test end, so this revision prevents any side effect on other tests.

Comment 26

[Adam]

Attachment: Rework test: check the -moz-drag-over style responds appropriately.

I wasn't able to come up with a useful test that used the nsIDragService, but it's possible I overlooked some way?  

This test still gets the warning about enablePrivilege.  I believe that's due to using EventsUtils.js in conjunction with an iframe?  Not certain.  I tried to rework it to avoid the warning, but haven't been successful.

Comment 27

[Timothy Nikkel (:tnikkel)]

(In reply to Adam [:hobophobe] from comment #26)
> This test still gets the warning about enablePrivilege.  I believe that's
> due to using EventsUtils.js in conjunction with an iframe?  Not certain.  I
> tried to rework it to avoid the warning, but haven't been successful.

What is the warning that you are getting?

Comment 28

[Adam]

The full warning, visible in the error console (I can keep the test window open by removing/commenting out the call to SimpleTest.finish() (line 28), possibly some other option (opposite of --close-when-done?)) is:  

> Warning: Use of enablePrivilege is deprecated.  Please use code that runs with the system principal (e.g. an extension) instead.
> Source File: chrome://mochitests/content/chrome/content/events/test/test_bug591249.xul
> Line: 0

I know it's related to the move to SpecialPowers, which allows using specific chrome-privileged calls without exposing privileges to the whole test.  I'm not clear what the best way to fix it is/why it's still not being run as a chrome-level test (I changed the test to be XUL and run it with make -C obj-dir mochitest-chrome)?

Comment 29

[Timothy Nikkel (:tnikkel)]

(In reply to Adam [:hobophobe] from comment #28)
> I know it's related to the move to SpecialPowers, which allows using
> specific chrome-privileged calls without exposing privileges to the whole
> test.  I'm not clear what the best way to fix it is/why it's still not being
> run as a chrome-level test (I changed the test to be XUL and run it with
> make -C obj-dir mochitest-chrome)?

I wouldn't worry about it.

What else is needed before this can land?

Comment 30

[Timothy Nikkel (:tnikkel)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ec855ff26e8d

Comment 31

[Michael Wu [:mwu]]

https://hg.mozilla.org/mozilla-central/rev/ec855ff26e8d

Comment 32

[Timothy Nikkel (:tnikkel)]

Thanks Adam!