Custom CSS cursors do not use -webkit-image-set on retina displays

Visit http://jsbin.com/ewilah and move the mouse over the first red square on a retina display. The expected result is a high-resolution checkerboard where one checkerboard square fills one physical pixel. This demo uses the -webkit-image-set(url(lowres.png) 1x, url(highres.png) 2x) to specify a specific cursor image to use on retina displays. WebKit correctly uses the high-resolution image but incorrectly shows it twice as big as it should be (it shows up huge and blurry). This was recently fixed in Chrome and now works correctly in Chrome Canary.

[Alexey Proskuryakov]

This was fixed under an ifdef in bug 99493, but Mac port doesn't turn it on (ENABLE_MOUSE_CURSOR_SCALE).

[Alexey Proskuryakov]

<rdar://problem/14921432>

Created attachment 211167 [details]
Patch to use -webkit-image-set for custom CSS cursors

[Darin Adler]

Comment on attachment 211167 [details]
Patch to use -webkit-image-set for custom CSS cursors

View in context: https://bugs.webkit.org/attachment.cgi?id=211167&action=review

> Source/WebCore/platform/mac/CursorMac.mm:56
> +    NSSize size = NSMakeSize(image->width() / scale, image->height() / scale);

Did you test with a cursor that has a non-integral pixel height? Like a 31x31 pixel cursor with a 2X scale factor?

> Source/WebKit2/ChangeLog:13
> +        (CoreIPC::::encode):
> +        (CoreIPC::::decode):

Should fix the incorrect function names here. It should be:

    (CoreIPC::ArgumentCoder<Cursor>::encode):

Too bad the script got it wrong, but it would be great if you fixed it.

Created attachment 211322 [details]
Updated patch with fixes and manual test html page

The previous patch doesn't work as expected for odd sizes. I hadn't tested that because -webkit-image-set doesn't work as expected for regular CSS background images at odd sizes either, but you're right that it should work. Added a manual test html page for different image sizes and hotspot locations and fixed all related bugs. The test page is also live at http://jsbin.com/IkuCATe.

[Darin Adler]

Comment on attachment 211322 [details]
Updated patch with fixes and manual test html page

View in context: https://bugs.webkit.org/attachment.cgi?id=211322&action=review

> Source/WebCore/platform/mac/CursorMac.mm:62
> +        RetainPtr<NSImage> intImage = adoptNS([[NSImage alloc] initWithSize:intSize]);

Sure would have been nice to use imageWithSize:flipped:drawingHandler: instead on OS X 10.8 or later.

> Source/WebCore/platform/mac/CursorMac.mm:66
> +        [intImage.get() lockFocus];

Could have used lockFocusFlipped:YES and then the toRect would be slightly less strange.

The documentation for imageWithSize:flipped:drawingHandler: says that the provided drawing handler is invoked at a later time and may be invoked on a separate thread. I'm not familiar enough with the WebKit codebase to feel confident evaluating and resolving the potential threading issues, so I'd like to keep the code simple and straightforward if possible.

If I use lockFocusFlipped: then the image would be upside down, which would require an additional translation and scale transform to fix. Just using lockFocus with an offset seemed simpler.

Is there still a problem with this patch? Should I try to use imageWithSize:flipped:drawingHandler: despite potential threading issues, or is the current approach ok?

[Csaba Osztrogonác]

Comment on attachment 211167 [details]
Patch to use -webkit-image-set for custom CSS cursors

Cleared Darin Adler's review+ from obsolete attachment 211167 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Devon Govett]

Any updates on this?  Seems it works in Chrome but not Safari.

[Jon Davis]

Can we get any more traction on this issue?

[Tim Horton]

Created attachment 250865 [details]
rebased

[Simon Fraser (smfr)]

Comment on attachment 250865 [details]
rebased

View in context: https://bugs.webkit.org/attachment.cgi?id=250865&action=review

> Source/WebCore/platform/mac/CursorMac.mm:48
> +#if ENABLE(MOUSE_CURSOR_SCALE)
> +static RetainPtr<NSCursor> createCustomCursor(Image* image, const IntPoint& hotSpot, float scale)
> +#else
>  static RetainPtr<NSCursor> createCustomCursor(Image* image, const IntPoint& hotSpot)
> +#endif

I don't like #ifdefs that changes signatures. Can we just always pass in scale and UNUSED_PARAM() it?

[WebKit Commit Bot]

Comment on attachment 250865 [details]
rebased

Clearing flags on attachment: 250865

Committed r182869: <http://trac.webkit.org/changeset/182869>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.