698936 - dragleave is fired on previous drop target after successful drop

Status: RESOLVED FIXED

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

Description

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 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

Actual:

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

Expected:

"status" should still be "drop".

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 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)]

Enn or smaug would be better reviewers here.

Comment 3

[Karl Tomlinson (:karlt)]

Comment on attachment 571183 [details] [diff] [review]
patch v1

Guessing smaug looked at this most recently.

Comment 4

[Karl Tomlinson (:karlt)]

FWIW ISTR this behaviour even before bug 591249 landed.

Comment 5

[Neil Deakin]

Would it be better to clear this during the dragend event (in PostHandleEvent)?

Comment 6

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

Is this a regression? If it is, is it perhaps from bug 591249

Comment 7

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

Comment on attachment 571183 [details] [diff] [review]
patch v1

Neil, could you review this.

Comment 8

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

(If you don't have time to review, I can review)

Comment 9

[Karl Tomlinson (:karlt)]

(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]

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] (inactive)]

Attachment: 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 13

[Neil Deakin]

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] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/cad93c498146

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

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