Closed
Bug 1200469
Opened 9 years ago
Closed 8 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)
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/
Reporter | ||
Updated•9 years ago
|
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
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•8 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
Assignee | ||
Comment 9•8 years ago
|
||
Assignee | ||
Comment 10•8 years ago
|
||
Incorrectly saved the source page...
Attachment #8780070 -
Attachment is obsolete: true
Assignee | ||
Comment 11•8 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 13•8 years ago
|
||
Probably changing only if all loading completes if from the same frame works...
Comment 14•8 years ago
|
||
That's probably more a question for :seth (if he's still around) or :tn.
Flags: needinfo?(dbaron) → needinfo?(seth.bugzilla)
Assignee | ||
Comment 15•8 years ago
|
||
Probably :seth is not quite around. Let me try to ni? :tn.
Flags: needinfo?(tnikkel)
Comment 16•8 years ago
|
||
(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•8 years ago
|
Flags: needinfo?(vladimir)
Flags: needinfo?(seth.bugzilla)
Assignee | ||
Comment 19•8 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•8 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)
Comment 21•8 years ago
|
||
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•8 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•8 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+
Comment 24•8 years ago
|
||
(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•8 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•8 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•8 years ago
|
||
Why couldn't I autoland the patches...
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → xidorn+moz
Comment 28•8 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•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5f3c1d227089
https://hg.mozilla.org/mozilla-central/rev/7367f3041715
Status: NEW → RESOLVED
Closed: 8 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.
Description
•