Closed Bug 1200469 Opened 5 years ago Closed 4 years ago

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

Categories

(Core :: Layout, defect)

20 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: benjamin.grant, Assigned: xidorn)

References

()

Details

(Keywords: regression, reproducible, Whiteboard: [gfx-noted])

Attachments

(3 files, 1 obsolete file)

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/
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
Whiteboard: [gfx-noted]
Version: 41 Branch → 20 Branch
Alice, any idea about the regressing bug?
Flags: needinfo?(alice0775)
no str, no idea
Flags: needinfo?(alice0775)
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
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)
Duplicate of this bug: 1293673
Attached file testcase from bug 1293673 (obsolete) —
Incorrectly saved the source page...
Attachment #8780070 - Attachment is obsolete: true
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...
dbaron, what do you think?
Flags: needinfo?(dbaron)
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)
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)
Flags: needinfo?(vladimir)
Flags: needinfo?(seth.bugzilla)
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.
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 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 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 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 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+
Why couldn't I autoland the patches...
Assignee: nobody → xidorn+moz
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
https://hg.mozilla.org/mozilla-central/rev/5f3c1d227089
https://hg.mozilla.org/mozilla-central/rev/7367f3041715
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.