Using javascript and a canvas element to quickly change the data-url of a css cursor property causes the cursor to flicker.

RESOLVED FIXED in Firefox 51

Status

()

Core
Layout
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: Benjamin Grant, Assigned: xidorn)

Tracking

({regression, reproducible})

20 Branch
mozilla51
regression, reproducible
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

(Whiteboard: [gfx-noted], URL)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/44.0.2403.155 Safari/537.36

Steps to reproduce:

Used javascript to change the url of the css cursor attribute at a high speed. See http://gra0007.github.io/Color-Picker-Cursor/


Actual results:

The cursor flickers from the data url to the fallback cursor.


Expected results:

The cursor should have updated smoothly with new data urls with no flickering. Try viewing this in chrome: http://gra0007.github.io/Color-Picker-Cursor/

Comment 1

3 years ago
What are the STR?
The bug is reproducible on the latest release(42.0) and latest Nightly(45.0a1).

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:42.0) Gecko/20100101 Firefox/42.0
Build ID: 20151029151421

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:45.0) Gecko/20100101 Firefox/45.0
Build ID: 20151203030226

Tested this issue also on Chrome and correctly works, but in IE the color picker doesn't work at all.
Considering this, I will mark this bug as New and assign the appropriate component.
Status: UNCONFIRMED → NEW
Component: Untriaged → Graphics
Ever confirmed: true
Product: Firefox → Core
This is a regression dating back to Firefox 20.

> Last good revision: 4dfe323a663d (2012-12-11)
> First bad revision: 634180132e68 (2012-12-12)
> Pushlog:
> http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=4dfe323a663d&tochange=634180132e68
Component: Graphics → Canvas: 2D
Keywords: regression, reproducible
Whiteboard: [gfx-noted]
Version: 41 Branch → 20 Branch

Comment 4

2 years ago
Alice, any idea about the regressing bug?
Flags: needinfo?(alice0775)

Comment 5

2 years ago
no str, no idea
Flags: needinfo?(alice0775)

Comment 6

2 years ago
STR:
1) Open http://gra0007.github.io/Color-Picker-Cursor/
2) Paste the image link https://secure.gravatar.com/avatar/1dcce558e7dd742de09268d7681568e8?d=mm&size=64 then press 'get image'
3) Hover your mouse over the image loaded as canvas

Result: cursor flickers

Comment 7

2 years ago
Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=89ad76a08a74&tochange=9602f98a6a70

Via local build,
Last Good : 9dffd2d825c9
First Bad : 513ec84b5c88

Triggered by: 513ec84b5c88	Vladimir Vukicevic — b=731974, requestAnimationFrame generates too short/long frames (incl. bug 799242); r=bz,smaug,roc,ehsan
Blocks: 731974
Component: Canvas: 2D → Layout
Flags: needinfo?(vladimir)
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1293673
(Assignee)

Comment 9

2 years ago
Created attachment 8780070 [details]
testcase from bug 1293673
(Assignee)

Comment 10

2 years ago
Created attachment 8780075 [details]
testcase from bug 1293673

Incorrectly saved the source page...
Attachment #8780070 - Attachment is obsolete: true
(Assignee)

Comment 11

2 years ago
So this is because we load the cursor image asynchronously, and the image may not have been loaded at the time we display the cursor. When that happens, we change the cursor to a fallback cursor. You can find code in nsFrame::FillCursorInformationFromStyle for this logic.

I'm not completely sure how should we fix this. Probably we could add a flag to nsIFrame::Cursor to indicate that there is an image loading, and make EventStateManager::UpdateCursor avoid changing the cursor if the cursor is from the same frame but there is some image is loading now. But given that the cursor could have several fallbacks, I'm not sure what should happen if there are multiple images...

Alternatively, we could probably make "data:" image load synchronously if they are small enough?

Not sure what can we do here...
(Assignee)

Comment 12

2 years ago
dbaron, what do you think?
Flags: needinfo?(dbaron)
(Assignee)

Comment 13

2 years ago
Probably changing only if all loading completes if from the same frame works...
That's probably more a question for :seth (if he's still around) or :tn.
Flags: needinfo?(dbaron) → needinfo?(seth.bugzilla)
(Assignee)

Comment 15

2 years ago
Probably :seth is not quite around. Let me try to ni? :tn.
Flags: needinfo?(tnikkel)
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #11)
> I'm not completely sure how should we fix this. Probably we could add a flag
> to nsIFrame::Cursor to indicate that there is an image loading, and make
> EventStateManager::UpdateCursor avoid changing the cursor if the cursor is
> from the same frame but there is some image is loading now. But given that
> the cursor could have several fallbacks, I'm not sure what should happen if
> there are multiple images...

This kind of thing is what we do for <img> elements, and I think also for background images now too.

I don't think we should worry about the fallbacks until the first one has either loaded or come up with an error. And maybe have a timer for the maximum amount of time that we would show the previous cursor image?

> Alternatively, we could probably make "data:" image load synchronously if
> they are small enough?

Yes, this is the other option (that Chrome is probably doing).
Flags: needinfo?(tnikkel)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Flags: needinfo?(vladimir)
Flags: needinfo?(seth.bugzilla)
(Assignee)

Comment 19

2 years ago
The first patch at least stops the cursor from blinking back and forth, but it is still not ideal. If you move the pointer fast enough, you can see a sharp change on the cursor. So it would still be better to make "data:" image load synchronously for cursor, but I have no idea how to do that :/

The second patch isn't really necessary, but without that, the image from the script would be leaking.
(Assignee)

Comment 20

2 years ago
tnikkel, do you think making "data:" image load synchronously for cursor is something easy? If yes, could you provide some advice? Or probably take this and fix it that way? Otherwise, I guess we can use this approach first and have another bug for making that synchronous.
Flags: needinfo?(tnikkel)
Wouldn't loading and decoding data url synchronously be rather bad for the ux, in case cursor is large.
(I admit it rarely is)

Comment 22

2 years ago
mozreview-review
Comment on attachment 8782893 [details]
Bug 1200469 part 1 - Avoid using fallback cursor if from the same frame.

https://reviewboard.mozilla.org/r/72922/#review70732

I tried this on linux and didn't see issues.

But, looking at the code, do we really want to update cursor during PreHandleEvent and not PostHandleEvent? Since I'd assume the latter.
Or is the update somehow visible to the JS so mousemove listener could somehow use that information?

Comment 23

2 years ago
mozreview-review
Comment on attachment 8782893 [details]
Bug 1200469 part 1 - Avoid using fallback cursor if from the same frame.

https://reviewboard.mozilla.org/r/72922/#review70740

But I guess we can take this even if we improved the setup in followup patches or so
Attachment #8782893 - Flags: review?(bugs) → review+
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #20)
> tnikkel, do you think making "data:" image load synchronously for cursor is
> something easy? If yes, could you provide some advice? Or probably take this
> and fix it that way? Otherwise, I guess we can use this approach first and
> have another bug for making that synchronous.

There is bug 1092837. Maybe you want to pick that up?
Flags: needinfo?(tnikkel)

Comment 25

2 years ago
mozreview-review
Comment on attachment 8782894 [details]
Bug 1200469 part 2 - Discard cursor image urgently.

https://reviewboard.mozilla.org/r/72924/#review70820
Attachment #8782894 - Flags: review?(tnikkel) → review+

Comment 26

2 years ago
mozreview-review
Comment on attachment 8782893 [details]
Bug 1200469 part 1 - Avoid using fallback cursor if from the same frame.

https://reviewboard.mozilla.org/r/72922/#review70822
Attachment #8782893 - Flags: review?(tnikkel) → review+
(Assignee)

Comment 27

2 years ago
Why couldn't I autoland the patches...
(Assignee)

Updated

2 years ago
Assignee: nobody → xidorn+moz

Comment 28

2 years ago
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f3c1d227089
part 1 - Avoid using fallback cursor if from the same frame. r=tnikkel,smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/7367f3041715
part 2 - Discard cursor image urgently. r=tnikkel

Comment 29

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5f3c1d227089
https://hg.mozilla.org/mozilla-central/rev/7367f3041715
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.