1304098 - [e10s] SVG cursors are no longer HiDPI

Status: VERIFIED FIXED

(Core :: Widget, defect, P3)

Description

[Markus Stange [:mstange]]

Steps to reproduce:
 1. Be on macOS with a retina display.
 2. Go to http://jsbin.com/puneripu
 3. Move your mouse over the gray rectangle.

Expected results:
The cursor should look sharp.

Actual results:
The cursor is blurry.

This was working before (bug 888689) but it seems it has broken.

Comment 1

[Jim Mathies [:jimm]]

Evan, can you take a look here?

Comment 2

[Markus Stange [:mstange]]

It would be nice to get a regression range first. I guess I'll do that now.

Comment 3

[Markus Stange [:mstange]]

Oh no, this was already broken in the 2016-07-12 nightly, which is the first one that starts on 10.12 (bug 1284677). Getting Mac regression ranges on 10.12 is going to be a major problem going forward...

Comment 4

[Ryan VanderMeulen [:RyanVM]]

I assume that this is going to be wontfix for 50 at this point. Not sure if it's bad enough to care about for 51, though.

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

I believe this is a regression from e10s. Disabling e10s should fix it. And I have some analysis in bug 1312973 comment 0. And this issue is not Mac-specific, it happens across all platforms with e10s.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

FWIW, the SVG testcase from bug 888781 comment 13 looks equally bad to me whether e10s is enabled or not.

Comment 8

[Evan Wallace [:evanw]]

I would love to get this fixed. It still looks really bad, especially in Figma (the app my company is developing) where the cursor is really blurry in Firefox. It's especially noticeable for our app because it's a design tool and the cursor is almost always near the user's center of attention.

I'm not too familiar with the Mozilla codebase but I had some time to look into it today and I was able to fix this by pre-scaling the cursor in "PuppetWidget::SetCursor" and then scaling it back down in "TabParent::RecvSetCustomCursor". I had to add an additional scale parameter to "PBrowser::SetCustomCursor" to do that though. Is this something that Mozilla would be willing to accept a patch for? If so, who should I be working with to help with the patch?

Comment 9

[Markus Stange [:mstange]]

That sounds like the right approach! I can help out.

Comment 10

[Evan Wallace [:evanw]]

Attachment: Bug 1304098 - SVG cursors are no longer HiDPI.

This change fixes custom CSS cursors on high-DPI displays in Windows. The problem was that custom cursor images were being rasterized at 1x before being passed across the IPC boundary. That results in a blurry cursor if the image being rasterized was a vector image (e.g. SVG). The fix is to rasterize the vector image at the correct high-DPI scale and also pass the scale factor along so the other end of the IPC message can scale the image back down appropriately.

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

Comment 11

[Evan Wallace [:evanw]]

Thanks so much! I just posted my patch. Here's what I did to manually test it:

* Check that the cursor on http://output.jsbin.com/puneripu is now the correct density. This is an SVG cursor, which should be sharp on a high-DPI display. It still has the incorrect colors because of bug 1096826 but I'll work on a fix for that that separately.

* Check that the cursor on http://jsfiddle.net/hvr2918h/ is still the same scale. This is a Windows .cur format cursor. My first attempt to fix this bug accidentally made the cursor shrink on that page because I had caused the cursor image never to be scaled. Now the image is still scaled up if the backing image doesn't support being rasterized at different resolutions.

It would be really great to have this covered by an automated test so it doesn't regress again in the future, but I have no idea how to do that. Is there an existing test harness that covers cursor images?

Comment 12

[Markus Stange [:mstange]]

Thank you!

(In reply to Evan Wallace [:evanw] from comment #11)
> It would be really great to have this covered by an automated test so it
> doesn't regress again in the future, but I have no idea how to do that. Is
> there an existing test harness that covers cursor images?

I'm not aware of any tests for cursors. And cursors aren't really "observable" at the moment, so you'd need to add such an observation mechanism. For example:
 - Add an API to nsIWidget like that returns a SourceSurface for the currently in-use cursor, maybe wrapped up in a struct with a displaySize field, and implement (or stub out) this API on the various widget implementations
 - Expose the nsIWidget API to JavaScript through the nsIDOMWindowUtils interface. The SourceSurface could be exposed as an ImageBitmap, maybe... not sure how easy to work with ImageBitmaps are these days.
Then you could write a test which consists of a content page with the right cursor CSS + SVG and a piece of JavaScript that runs in the chrome which moves the mouse in the right place (using sendNativeMouseMove), waits until content has processed that mouse move, and then checks the cursor using the nsIDOMWindowUtils API in the parent process. It can check both the pixel size and the displaySize of the cursor, and draw the ImageBitmap to a canvas and use getImageData() to check the actual pixels.

This would be a fair amount of work. I think the trickiest piece would be the nsIDOMWindowUtils API: deciding how to expose the information and creating new IDL interfaces if necessary (e.g. to convert the struct as a JS object).

Any test is better than no test, though, so if you can come up with a way to test just a subset of all this, that's fine, too. Whatever you're comfortable with.

Comment 13

[Xidorn Quan [:xidorn] UTC+10]

FWIW, even if we have test checking cursor, I don't think we currently run any test on HiDPI machine at all, so that doesn't help in this case anyway. (I may be wrong here, but I'm not aware of any such effort to have HiDPI covered, and I would expect there are reftests going wrong with HiDPI...)

Comment 14

[Markus Stange [:mstange]]

Oh, you're right.

Comment 15

[Evan Wallace [:evanw]]

Comment on attachment 8936105 [details]
Bug 1304098 - SVG cursors are no longer HiDPI.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/206876/diff/1-2/

Comment 16

[Evan Wallace [:evanw]]

I just posted an updated patch with a test. You're right that writing a test for this was a fair amount of work. Thanks so much for the detailed description of how to write the test! Luckily testing different DPIs was actually pretty easy. All I had to do was make sure my current Windows session was in 100% DPI mode and then temporarily change the "layout.css.devPixelsPerPx" preference from the test.

I think it's ready for review now. I ended up returning the RGBA data as a string instead of an ImageBitmap since it seemed simpler on both ends to avoid extra encoding and decoding. Let me know what you think about that approach. I tried to write code like the code that's already there but not sure if I'm following the right conventions or not (naming, memory management, security checks, etc.) so please let me know if anything needs changing. Thanks for your help!

Comment 17

[Markus Stange [:mstange]]

Comment on attachment 8936105 [details]
Bug 1304098 - SVG cursors are no longer HiDPI.

https://reviewboard.mozilla.org/r/206876/#review215848

This is really great work. Sorry for the long delay in reviewing.

The actual functionality change looks good. It'll need review from somebody else in addition to myself though, ideally from Neil Deakin. Could you split out the functionality change and the test change into two different commits? Then Neil can start reviewing the functionality change while you finish the test part.

As for the test:
 - Using the DPI pref in order to test 2x cursor behavior on a 1x system is a great idea!
 - I don't think the test actually exercises the e10s codepath. I could be wrong, though.
 - The setTimeout will need to be replaced with something else, unfortunately. Tests need to run reliably even when the system is under load.

::: dom/ipc/TabParent.cpp:1813
(Diff revision 2)
> +      RefPtr<gfxDrawable> drawable = new gfxPatternDrawable(
> +        new gfxPattern(customCursor, matrix), IntSize(aUnscaledWidth, aUnscaledHeight));

This is a neat trick and not something I would have thought of.

::: widget/tests/window_custom_cursor_win.html:3
(Diff revision 2)
> +<html lang="en-US">
> +<head>
> +  <title>Test for custom CSS cursors on Windows</title>

Does this test fail without your changes? I think the code in this file runs in the parent process, so the PuppetWidget / TabChild paths that you're touching are probably not used.

::: widget/tests/window_custom_cursor_win.html:121
(Diff revision 2)
> +    for (let i = 0; i < 3; i++) {
> +      const scale = utils.screenPixelsPerCSSPixel;
> +      const x = (window.mozInnerScreenX + 50 + i) * scale;
> +      const y = (window.mozInnerScreenY + 50 + i) * scale;
> +      await utils.sendNativeMouseMove(x, y, null);
> +      await sleep(100);

This sleep is a problem. In general, every sleep / setTimeout we've put in a test has caused that test to fail intermittently. As a consequence, adding new tests with setTimeout is extremely discouraged.

Instead of waiting for some time to elapse, you'll need to listen for something that has a causal relationship to the change you're waiting for. You may have to add new, possibly test-only, events or observer notifications for this.

::: widget/tests/window_custom_cursor_win.html:139
(Diff revision 2)
> +    // Change to our custom cursor
> +    document.body.style.cursor = `url(${cursorURL}) ${hotspotX} ${hotspotY}, auto`;
> +    await jiggleMouse();
> +
> +    // Check that the cursor is correct
> +    const cursor = await utils.getCursorForTests();

Is this "await" correct? utils.getCursorForTests does not return a promise, as far as I can tell.

::: widget/windows/nsWindow.cpp:3066
(Diff revision 2)
> +
> +  int width = bmInfo.bmiHeader.biWidth;
> +  int height = abs(bmInfo.bmiHeader.biHeight);
> +  int stride = width * 4;
> +
> +  RefPtr<DataSourceSurface> surface = Factory::CreateDataSourceSurface(

You could use CreateDataSourceSurfaceWithStride here and avoid the need for the unpacking loop.

Comment 18

[Hansol Kim (zvuc)]

I just wanted to implement a custom cursor with SVG in url and came across this issue. How is it going with applying the fix?

Comment 19

[Evan Wallace [:evanw]]

I had some time over winter break to take an initial stab at this, but my life is really busy at the moment and I haven't found the time to follow up on the review. If someone else has the time to carry this patch forward, please feel free to jump in and take over!

Comment 20

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1304098 - Scale vector images appropriately in PuppetWidget before sending them to the parent process. r=mstange

This builds on the image-set work to trivially implement this.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/61c4dd7693e4
Scale vector images appropriately in PuppetWidget before sending them to the parent process. r=mstange

Comment 22

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/61c4dd7693e4

Comment 23

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:emilio, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 24

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9216946 [details]
Bug 1304098 - Scale vector images appropriately in PuppetWidget before sending them to the parent process. r=mstange

Beta/Release Uplift Approval Request

• User impact if declined: SVG cursors on hiDPI screens are too small.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: See test-case in comment 0
• List of other uplifts needed: none
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This is a relatively small patch that builds on top of other uplifted fixes (bug 1705877), and might be nice to get in beta. However it's a long-standing regression, so your call :)
• String changes made/needed: none

Comment 25

[Catalin Sasca, Desktop Test Engineering [:csasca]]

Reproduced the issue on Firefox 90.0a1 (2021-04-20) under macOS 10.15.7 on a 2015 macbook pro retina. The issue is no longer reproducible on Firefox 90.0a1 (2021-05-05).

Comment 26

[Pascal Chevrel:pascalc]

Comment on attachment 9216946 [details]
Bug 1304098 - Scale vector images appropriately in PuppetWidget before sending them to the parent process. r=mstange

It regressed with Firefox 28, I think it can ride the trains :D

Comment 27

[Catalin Sasca, Desktop Test Engineering [:csasca]]

Based on info from Comment 26, will update the flags accordingly to wontfix for Firefox 89 and mark this as verified for Firefox 90.