Last Comment Bug 698936 - dragleave is fired on previous drop target after successful drop
: dragleave is fired on previous drop target after successful drop
Product: Core
Classification: Components
Component: Drag and Drop (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla11
Assigned To: Tim Taubert [:ttaubert]
: Neil Deakin
: 623214 (view as bug list)
Depends on:
  Show dependency treegraph
Reported: 2011-11-01 16:24 PDT by Tim Taubert [:ttaubert]
Modified: 2011-11-09 02:04 PST (History)
4 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

test case (968 bytes, text/html)
2011-11-01 16:24 PDT, Tim Taubert [:ttaubert]
no flags Details
patch v1 (924 bytes, patch)
2011-11-01 16:37 PDT, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review
patch v2 (1.60 KB, patch)
2011-11-03 04:48 PDT, Tim Taubert [:ttaubert]
enndeakin: review+
Details | Diff | Splinter Review

Description Tim Taubert [:ttaubert] 2011-11-01 16:24:54 PDT
Created attachment 571179 [details]
test case

Use the provided test case as follows:

1) start dragging the green rectangle
2) drag it over the red rectangle
3) drop it
4) "status" should be "drop" now
5) start dragging the green rectangle again


"status" is "drop dragleave" because this was fired again on sLastDragOverFrame (the red rectangle) from the previous dnd operation.


"status" should still be "drop".
Comment 1 Tim Taubert [:ttaubert] 2011-11-01 16:37:53 PDT
Created attachment 571183 [details] [diff] [review]
patch v1

I don't know if that's the right approach but it works for my test case. This patch sets sLastDragOverFrame to null when starting a new drag operation and thus cleaning out the value from previous drag operations.
Comment 2 Karl Tomlinson (:karlt) 2011-11-01 16:56:17 PDT
Enn or smaug would be better reviewers here.
Comment 3 Karl Tomlinson (:karlt) 2011-11-01 16:56:49 PDT
Comment on attachment 571183 [details] [diff] [review]
patch v1

Guessing smaug looked at this most recently.
Comment 4 Karl Tomlinson (:karlt) 2011-11-01 16:58:21 PDT
FWIW ISTR this behaviour even before bug 591249 landed.
Comment 5 Neil Deakin 2011-11-01 17:01:14 PDT
Would it be better to clear this during the dragend event (in PostHandleEvent)?
Comment 6 Olli Pettay [:smaug] 2011-11-01 17:07:08 PDT
Is this a regression? If it is, is it perhaps from bug 591249
Comment 7 Olli Pettay [:smaug] 2011-11-01 17:09:02 PDT
Comment on attachment 571183 [details] [diff] [review]
patch v1

Neil, could you review this.
Comment 8 Olli Pettay [:smaug] 2011-11-01 17:09:30 PDT
(If you don't have time to review, I can review)
Comment 9 Karl Tomlinson (:karlt) 2011-11-01 18:50:07 PDT
(In reply to Neil Deakin from comment #5)
> Would it be better to clear this during the dragend event (in
> PostHandleEvent)?

I assume that wouldn't be suitable when the source node is external to the browser.
Comment 10 Neil Deakin 2011-11-02 05:08:58 PDT
Does the bug occur for an external drag? Shouldn't the node be cleared in dragexit?

Regardless, there's no harm is clearing sLastDragOverFrame both in dragend and during dragstart. That would make it better in case someone adds some code to check sLastDragOverFrame outside a drag for whatever reason.
Comment 11 Tim Taubert [:ttaubert] 2011-11-03 04:48:56 PDT
Created attachment 571604 [details] [diff] [review]
patch v2

I guess it would be sufficient to only clean sLastDragOverFrame in PostHandleEvent[NS_DRAGDROP_DROP] (it's already set to null on NS_DRAGDROP_EXIT) but I kept the change from the first patch to also clean out on dragstart like you said.
Comment 12 Tim Taubert [:ttaubert] 2011-11-03 07:16:11 PDT
*** Bug 623214 has been marked as a duplicate of this bug. ***
Comment 13 Neil Deakin 2011-11-07 05:22:02 PST
Comment on attachment 571604 [details] [diff] [review]
patch v2

I'd actually meant to put it in 'dragend' rather than 'drop' but this is probably ok.
Comment 14 Tim Taubert [:ttaubert] 2011-11-08 03:15:13 PST
Comment 15 Tim Taubert [:ttaubert] 2011-11-09 02:04:33 PST

Note You need to log in before you can comment on or make changes to this bug.