1705877 - Incorrect cursor size when using image-set()

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[ashley]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:89.0) Gecko/20100101 Firefox/89.0

Steps to reproduce:

When using CSS image-set() for a cursor property on a high-dpi display, Firefox appears to show the wrong size cursor.

Minimal repro: https://downloads.scirra.com/labs/bugs/cursor-image-set/

Using a high-dpi display (my system has devicePixelRatio 2), hover the yellow box in Nightly and see the cursor size. Compare with Chrome.

Actual results:

In Firefox only, the cursor is unexpectedly large.

Expected results:

The cursor should be smaller and use the larger image to increase the detail for high-dpi displays, which is what it does in Chrome (albeit with the -webkit-image-set() prefix).

Comment 1

[ashley]

BTW this affects our browser-based game editor Construct 3 (www.construct.net). Since we use image-set for cursors in our PWA, in Nightly these cursors appear too large.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Ah, so I guess this is the one place in case images where we care about the intrinsic size of the image...

For regular image elements density is accounted for here: https://searchfox.org/mozilla-central/rev/00fe1b9131e16daeb71aa90ad3d752454761fb1f/layout/generic/nsImageFrame.cpp#498

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1705877 - image-set() should influence intrinsic size of the image. r=dholbert,#layout-reviewers

Astounding that there was literally no WPT for this at all. I added three, one
for backgrounds, one for list-style-image, and one for content. Cursor is not
handled on this patch because that one requires a fair amount of extra work.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1705877 - image-set() should influence intrinsic size of the image. r=dholbert,#layout-reviewers

https://drafts.csswg.org/css-images-4/#image-set-notation has:

[...] it also specifies the image’s natural resolution, overriding any other
source of data that might supply a natural resolution.

Astounding that there was literally no WPT for this at all. I added three, one
for backgrounds, one for list-style-image, and one for content. Cursor is not
handled on this patch because that one requires a fair amount of extra work.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1705877 - Introduce nsIWidget::Cursor. r=mstange,dholbert

This required more refactoring so it seemed sensible to split it out. GTK
doesn't seem to provide an API for scaled cursors so we get pixelated cursors
instead.

Depends on D112474

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1705877 - Add support for scaled cursors on GTK. r=stransky

I had overlooked the way to achieve this. This is possible using cairo
surfaces, but they only support integer scale factors, so we ceil and
potentially rasterize the image to a bigger size if needed.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/652bf5d7540d
image-set() should influence intrinsic size of the image. r=dholbert,layout-reviewers

Comment 8

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/28588 for changes under testing/web-platform/tests

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1705877 - Apply image-set resolution for cursors on Windows and Mac. r=mstange,dholbert

This required more refactoring so it seemed sensible to split it out. GTK
doesn't seem to provide an API for scaled cursors so we get pixelated cursors
instead.

Depends on D112475

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9141b021cbed
Introduce nsIWidget::Cursor. r=dholbert

Comment 11

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 12

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/652bf5d7540d

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/07f095c42f60
Remove a bunch of useless virtual keywords in nsBaseWidget.h.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/16194d15c078
Add back some virtual keywords that I shouldn't have removed.

Comment 15

[Emilio Cobos Álvarez (:emilio)]

(In reply to Pulsebot from comment #13)

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/07f095c42f60
Remove a bunch of useless virtual keywords in nsBaseWidget.h.

Sylvestre, this is a bit silly, my patch almost gets backed out for introducing one more of these using 'final' rather than 'override', while we had a gazillion of them. Do you know where should I file an issue? If we want to enforce this we should fix all the codebase first (for both 'final' and 'override', not just 'final').

Comment 16

[Sylvestre Ledru [:Sylvestre]]

I think cpeterson wrote this checker a while back.

In term of component, it should probably be
https://bugzilla.mozilla.org/enter_bug.cgi?product=Firefox+Build+System&component=Lint+and+Formatting

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/aecf24e96348
Apply image-set resolution for cursors on Windows and Mac. r=dholbert

Comment 18

[Chris Peterson [:cpeterson]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #15)

Sylvestre, this is a bit silly, my patch almost gets backed out for introducing one more of these using 'final' rather than 'override', while we had a gazillion of them. Do you know where should I file an issue? If we want to enforce this we should fix all the codebase first (for both 'final' and 'override', not just 'final').

This lint is a simple regular expression that doesn't know how to check function declarations that span multiple lines. That's why some the results are inconsistent even within the same file. At the time, I thought reporting some problems was better than none. But this lint is mostly a style issue, so I can remove it if it's causing problems.

Comment 19

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/9141b021cbed
https://hg.mozilla.org/mozilla-central/rev/07f095c42f60
https://hg.mozilla.org/mozilla-central/rev/16194d15c078
https://hg.mozilla.org/mozilla-central/rev/aecf24e96348

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ee030bf89f28
Add support for scaled cursors on GTK. r=stransky

Comment 21

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/ee030bf89f28

Comment 22

[ashley]

Thanks for the fix! I can confirm cursors appear at the right size in our PWA in Nightly now.

Comment 23

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9216600 [details]
Bug 1705877 - image-set() should influence intrinsic size of the image. r=dholbert,#layout-reviewers

Beta/Release Uplift Approval Request

• User impact if declined: Potentially broken (as in, too big) cursors or backgrounds or other CSS images if they use image-set, a feature that we shipped in 88.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: comment 0
• List of other uplifts needed: none
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): Even though the patches look really really big, it's really mostly plumbing and refactoring of the cursor code, in a way that they only change behavior for cursors using image-set(). It'd be nice to not ship this bug in 89.
• String changes made/needed: none

Comment 24

[Pascal Chevrel:pascalc (PTO until April 26)]

Emilio, I will let these patches on nightly bake a few days before taking a decision on the uplift.

Comment 25

[Emilio Cobos Álvarez (:emilio)]

Sure, that's totally fair, thanks Pascal!

Comment 26

[Raluca Popovici, Desktop QA]

I confirm the issue with the larger cursor on Fx 88.0 and Fx 89.0b2 compared with chrome.
Verified - Fixed in latest Nightly 90.0a1, the cursor has the same size with chrome, using Windows 10x64 with 4k monitor, Mac 11 and Ubuntu 18.04.

Comment 27

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9216600 [details]
Bug 1705877 - image-set() should influence intrinsic size of the image. r=dholbert,#layout-reviewers

Visible bug in a new CSS feature we shipped in 88, has tests, verified on nightly and we are early in the beta cycle, uplift approved for beta 4, thanks Emilio.

Comment 28

[Pascal Chevrel:pascalc (PTO until April 26)]

https://hg.mozilla.org/releases/mozilla-beta/rev/427042d0eb86
https://hg.mozilla.org/releases/mozilla-beta/rev/ded67aa96d59
https://hg.mozilla.org/releases/mozilla-beta/rev/b820f8ab1798
https://hg.mozilla.org/releases/mozilla-beta/rev/cbc94105c4c3
https://hg.mozilla.org/releases/mozilla-beta/rev/204748c192c5
https://hg.mozilla.org/releases/mozilla-beta/rev/7c76ed0e6eac

Comment 29

[Pascal Chevrel:pascalc (PTO until April 26)]

Backout revision 7c76ed0e6eac532c59f6de20cfd059acd460a11e (bug 1705877) for build bustage
https://hg.mozilla.org/releases/mozilla-beta/rev/950b132716ae46f186bd0f541eb7fa1c400cc1ea

Comment 30

[Emilio Cobos Álvarez (:emilio)]

Attachment: GTK patch rebased to beta.

Only difference is the mozgtk.c changes, which were needed before bug 1377445.

Comment 31

[Pascal Chevrel:pascalc (PTO until April 26)]

https://hg.mozilla.org/releases/mozilla-beta/rev/5ead04bf61af

Comment 33

[Raluca Popovici, Desktop QA]

Verified - Fixed in latest Firefox Beta 89.0b8, the cursor has the same size with chrome, using Windows 10x64 Ubuntu 20.04. with 4k monitor, Mac 11.
Verified- Fixed the duplicated bug 1709284 in the latest Fx Beta 89.0b8 using Windows 10x64 Ubuntu 20.04. with 4k monitor, Mac 11.