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

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Benjamin Grant]

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

[Loic]

What are the STR?

Comment 2

[Paul Pasca[Away. Please needinfo? Paul Oiegas [:pauloiegasSV]]]

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.

Comment 3

[u279076]

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

Comment 4

[Loic]

Alice, any idea about the regressing bug?

Comment 5

[Alice0775 White]

no str, no idea

Comment 6

[Loic]

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

[Alice0775 White]

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

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

Attachment: testcase from bug 1293673

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

Attachment: testcase from bug 1293673

Incorrectly saved the source page...

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

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...

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

dbaron, what do you think?

Comment 13

[Xidorn Quan [:xidorn] UTC+10]

Probably changing only if all loading completes if from the same frame works...

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

That's probably more a question for :seth (if he's still around) or :tn.

Comment 15

[Xidorn Quan [:xidorn] UTC+10]

Probably :seth is not quite around. Let me try to ni? :tn.

Comment 16

[Timothy Nikkel (: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).

Comment 17

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1200469 part 1 - Avoid using fallback cursor if from the same frame.

Review commit: https://reviewboard.mozilla.org/r/72922/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/72922/

Comment 18

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1200469 part 2 - Discard cursor image urgently.

Given that script may change cursor image in a high frequency, and it
is usually small, it is probably better discarding it urgently.

Review commit: https://reviewboard.mozilla.org/r/72924/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/72924/

Comment 19

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 20

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 21

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

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

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

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

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

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

Comment 24

[Timothy Nikkel (:tnikkel)]

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

Comment 25

[Timothy Nikkel (:tnikkel)]

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

https://reviewboard.mozilla.org/r/72924/#review70820

Comment 26

[Timothy Nikkel (:tnikkel)]

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

Comment 27

[Xidorn Quan [:xidorn] UTC+10]

Why couldn't I autoland the patches...

Comment 28

[Pulsebot]

Pushed by [e-mail]:
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

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5f3c1d227089
https://hg.mozilla.org/mozilla-central/rev/7367f3041715