Closed Bug 536920 Opened 10 years ago Closed 10 years ago

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

Categories

(Core :: DOM: Events, defect)

x86
Windows XP
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: philbaseless-firefox, Assigned: philbaseless-firefox)

References

Details

(Whiteboard: [3.6.x])

Attachments

(1 file, 2 obsolete files)

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.
Attachment #419276 - Flags: review?(enndeakin)
Assignee: nobody → philbaseless-firefox
Status: NEW → ASSIGNED
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 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.
Attachment #419276 - Flags: review?(enndeakin)
(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
This is a dupe of bug 521966, I think, but this one has a patch...you choose which way to dupe...
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.
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.
Attached patch clone cursorState in ctor (obsolete) — Splinter Review
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.
Attachment #419276 - Attachment is obsolete: true
Attachment #419381 - Flags: review?(jmathies)
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.
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
Assignee: philbaseless-firefox → nobody
Component: Drag and Drop → DOM: Events
QA Contact: drag-drop → events
Assignee: nobody → philbaseless-firefox
Attachment #419381 - Flags: review?(jmathies)
I could review
But Neil would be better.
I got to revise it per Neil's comment and test again.
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.
Attachment #419381 - Attachment is obsolete: true
Attachment #419519 - Flags: review?(enndeakin)
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.
Attachment #419519 - Flags: review?(enndeakin) → review+
Duplicate of this bug: 521966
Summary: cursor flashes and doesn't get updated--drag and drop → Cursor flashes and doesn't update during drag and drop operations
http://hg.mozilla.org/mozilla-central/rev/925d4e9ff21f

Thanks Phil!
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Tested with latest trunk.  Looks like you fixed r
Flags: wanted1.9.2?
Flags: blocking1.9.2?
(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.
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?
Flags: wanted1.9.2?
Flags: wanted1.9.2+
Flags: blocking1.9.2?
Flags: blocking1.9.2-
Whiteboard: [3.6.x]
(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.
(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.
You need to log in before you can comment on or make changes to this bug.