536920 - Cursor flashes and doesn't update during drag and drop operations

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Phil Lacy]

Attachment: initial transferdata cursorState sets event transferdata cursorstate

drag and drop causes cursor flashing and indecisive image from arrow to the proper one. Caused by cursorState of current event setting the initial event setting. Although this is incorrect it still seems strange that the event cursorState setting wasn't initialized to FALSE to start, but that doesn't matter here.

Comment 1

[Phil Lacy]

http://quality.mozilla.org/sites/quality.mozilla.org/files/images/Minefield-icon.png

STR:
go to image above.
drag over tabs and move around soon it changes to arrow and never gets updated. I usually will flicker. 
I notice this a lot more in shredder and I'll try this patch over there. But it is really noticeable in chrome windows.
bug 462172 enables dragging files out of download manager and it really has a problem of flickering. This patch fixes that.

Comment 2

[Phil Lacy]

Comment on attachment 419276 [details] [diff] [review]
initial transferdata cursorState sets event transferdata cursorstate

this can't be right, the function is to update the original datatransfer with the events. 
I think the problem is our constructors for events should initialize the mozCursor to PR_FALSE

It seems the clone constructor also.
http://mxr.mozilla.org/comm-central/source/mozilla/content/events/src/nsDOMDataTransfer.cpp#83
http://mxr.mozilla.org/comm-central/source/mozilla/content/events/src/nsDOMDataTransfer.cpp#83
I can patch that and test the results on this bug. 
Let me know what you think.

Comment 3

[Phil Lacy]

(In reply to comment #2)
> (From update of attachment 419276 [details] [diff] [review])
> this can't be right, the function is to update the original datatransfer with
> the events. 
> I think the problem is our constructors for events should initialize the
> mozCursor to PR_FALSE

or initialize it here:

http://mxr.mozilla.org/cmm-central/source/mozilla/content/base/src/nsContentUtils.cpp#4661

Comment 4

[Nochum Sossonko [:Natch]]

This is a dupe of bug 521966, I think, but this one has a patch...you choose which way to dupe...

Comment 5

[Jim Mathies [:jimm]]

The purpose of the code here was to transfer changes made to the event transfer back to the initiating transfer. (See bug 483776.) This patch would cause changes to be lost. I'm not sure this is what we want to do.

Comment 6

[Phil Lacy]

Yes, I came up with this patch because I noticed the event dt mozCursor was corrupt. 
I'm going to put up what I think is a correct patch.

Comment 7

[Phil Lacy]

Attachment: clone cursorState in ctor

Jim, I initially created the other patch because the event nsDOMDataTransfer.mCursorState was unintialized and had data like 0xcd etc. which figures as TRUE and "default".
The cloning of this object should also clone the initial DataTransfer objects cursorState.
There may be other member variables that we are not cloning over maybe you want all the data that needs cloning to be in the parameters of the ctor instead of the clone function.

Comment 8

[Neil Deakin]

The other constructors should be initializing mCursorState as well. The one called in Clone should just take the cursor state as an argument as with the other fields.

Comment 9

[Jim Mathies [:jimm]]

Phil, sorry I can't review this. You'll need an r+ from someone who is an owner or peer for events -

http://www.mozilla.org/about/owners.html#document-object-model

Comment 10

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

I could review

Comment 11

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

But Neil would be better.

Comment 12

[Phil Lacy]

I got to revise it per Neil's comment and test again.

Comment 13

[Phil Lacy]

Attachment: initialized mCursorState

This smooths out the cursor and updates it properly.
I initialized the mCursorState in the other contructor. It seems proper but I can't claim to know its use. I believe it is called in the contentUtility and it seems to me it should use an auto cursor for the drag from external source which I believe is the intent of this constructor.

Comment 14

[Neil Deakin]

Comment on attachment 419519 [details] [diff] [review]
initialized mCursorState

> I believe it is called in the contentUtility and
it seems to me it should use an auto cursor for the drag from external source
> which I believe is the intent of this constructor.

Yes, it's called with drags from other applications, or if someone started a drag by calling the drag service directly.

Comment 16

[Jim Mathies [:jimm]]

http://hg.mozilla.org/mozilla-central/rev/925d4e9ff21f

Thanks Phil!

Comment 17

[[not reading bugmail]]

Tested with latest trunk.  Looks like you fixed r

Comment 18

[[not reading bugmail]]

(In reply to comment #17)
> Tested with latest trunk.  Looks like you fixed r

Oops, sorry for the spam, Requesting for 3.6 to fix bug 521966 as this looks fixed for bug 524748 with 1.9.3.

Comment 19

[Mike Beltzner [:beltzner, not reading bugmail]]

This isn't a release blocker, but a good ride-along if the patch is safe. Why are there no tests with the patch?

Enn/Jimm: comments on safety?

Comment 20

[Phil Lacy]

(In reply to comment #19)
> ... Why
> are there no tests with the patch?
> 

The patch adds initializers to the constructors so I'd say it is its own test. Any code using the constructors has to initialize the cursorState whereas that was not required before the patch and caused the bug.

Comment 21

[Jim Mathies [:jimm]]

(In reply to comment #20)
> (In reply to comment #19)
> > ... Why
> > are there no tests with the patch?
> > 
> 
> The patch adds initializers to the constructors so I'd say it is its own test.
> Any code using the constructors has to initialize the cursorState whereas that
> was not required before the patch and caused the bug.

..and that patch never landed on 1.9.2, so unless this bug has other risks for branch, I think an m-c landing is all that's needed.