Incorrect cursor size when using image-set()
Categories
(Core :: CSS Parsing and Computation, defect)
Tracking
()
People
(Reporter: ashley, Assigned: emilio)
References
(Blocks 1 open bug)
Details
Attachments
(5 files, 1 obsolete file)
Bug 1705877 - image-set() should influence intrinsic size of the image. r=dholbert,#layout-reviewers
(deleted),
text/x-phabricator-request
|
pascalc
:
approval-mozilla-beta+
|
Details |
(deleted),
text/x-phabricator-request
|
pascalc
:
approval-mozilla-beta+
|
Details |
(deleted),
text/x-phabricator-request
|
pascalc
:
approval-mozilla-beta+
|
Details |
(deleted),
text/x-phabricator-request
|
pascalc
:
approval-mozilla-beta+
|
Details |
(deleted),
text/plain
|
Details |
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).
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.
Updated•3 years ago
|
Assignee | ||
Comment 2•3 years ago
|
||
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
Assignee | ||
Comment 3•3 years ago
|
||
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.
Updated•3 years ago
|
Assignee | ||
Comment 4•3 years ago
|
||
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.
Assignee | ||
Comment 5•3 years ago
|
||
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
Updated•3 years ago
|
Assignee | ||
Comment 6•3 years ago
|
||
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.
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Updated•3 years ago
|
Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/652bf5d7540d image-set() should influence intrinsic size of the image. r=dholbert,layout-reviewers
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/28588 for changes under testing/web-platform/tests
Updated•3 years ago
|
Assignee | ||
Comment 9•3 years ago
|
||
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
Updated•3 years ago
|
Comment 10•3 years ago
|
||
Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/9141b021cbed Introduce nsIWidget::Cursor. r=dholbert
Upstream PR merged by moz-wptsync-bot
Comment 12•3 years ago
|
||
bugherder |
Comment 13•3 years ago
|
||
Pushed by emilio@crisal.io: https://hg.mozilla.org/integration/autoland/rev/07f095c42f60 Remove a bunch of useless virtual keywords in nsBaseWidget.h.
Comment 14•3 years ago
|
||
Pushed by emilio@crisal.io: https://hg.mozilla.org/integration/autoland/rev/16194d15c078 Add back some virtual keywords that I shouldn't have removed.
Assignee | ||
Comment 15•3 years ago
|
||
(In reply to Pulsebot from comment #13)
Pushed by emilio@crisal.io:
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•3 years ago
|
||
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•3 years ago
|
||
Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/aecf24e96348 Apply image-set resolution for cursors on Windows and Mac. r=dholbert
Comment 18•3 years ago
|
||
(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•3 years ago
|
||
bugherder |
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•3 years ago
|
||
Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ee030bf89f28 Add support for scaled cursors on GTK. r=stransky
Assignee | ||
Updated•3 years ago
|
Comment 21•3 years ago
|
||
bugherder |
Reporter | ||
Comment 22•3 years ago
|
||
Thanks for the fix! I can confirm cursors appear at the right size in our PWA in Nightly now.
Assignee | ||
Comment 23•3 years ago
|
||
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
Assignee | ||
Updated•3 years ago
|
Comment 24•3 years ago
|
||
Emilio, I will let these patches on nightly bake a few days before taking a decision on the uplift.
Updated•3 years ago
|
Comment 26•3 years ago
|
||
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.
Updated•3 years ago
|
Comment 27•3 years ago
|
||
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.
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Comment 28•3 years ago
|
||
bugherder uplift |
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•3 years ago
|
||
Backout revision 7c76ed0e6eac532c59f6de20cfd059acd460a11e (bug 1705877) for build bustage
https://hg.mozilla.org/releases/mozilla-beta/rev/950b132716ae46f186bd0f541eb7fa1c400cc1ea
Assignee | ||
Comment 30•3 years ago
|
||
Only difference is the mozgtk.c changes, which were needed before bug 1377445.
Comment 31•3 years ago
|
||
bugherder uplift |
Updated•3 years ago
|
Comment 33•3 years ago
|
||
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.
Updated•3 years ago
|
Updated•3 years ago
|
Description
•