Bug 120783

Summary: Custom CSS cursors do not use -webkit-image-set on retina displays
Product: WebKit Reporter: evan.exe
Component: CSSAssignee: Tim Horton <thorton>
Severity: Normal CC: ap, bdakin, commit-queue, devongovett+webkit, dino, jond, simon.fraser, thorton
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Macintosh   
OS: OS X 10.8   
URL: http://jsbin.com/ewilah
Description Flags
Patch to use -webkit-image-set for custom CSS cursors
Updated patch with fixes and manual test html page
rebased none

Description evan.exe 2013-09-05 11:16:54 PDT
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.
Comment 1 Alexey Proskuryakov 2013-09-05 12:45:55 PDT
This was fixed under an ifdef in bug 99493, but Mac port doesn't turn it on (ENABLE_MOUSE_CURSOR_SCALE).
Comment 2 Alexey Proskuryakov 2013-09-05 12:46:10 PDT
Comment 3 evan.exe 2013-09-09 23:17:23 PDT
Created attachment 211167 [details]
Patch to use -webkit-image-set for custom CSS cursors
Comment 4 Darin Adler 2013-09-10 12:56:39 PDT
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:


Too bad the script got it wrong, but it would be great if you fixed it.
Comment 5 evan.exe 2013-09-11 10:01:00 PDT
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.
Comment 6 Darin Adler 2013-09-11 15:25:51 PDT
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.
Comment 7 evan.exe 2013-09-11 22:00:33 PDT
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.
Comment 8 evan.exe 2013-10-04 13:51:14 PDT
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?
Comment 9 Csaba Osztrogon√°c 2014-02-13 04:11:58 PST
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.
Comment 10 Devon Govett 2014-10-23 01:04:42 PDT
Any updates on this?  Seems it works in Chrome but not Safari.
Comment 11 Jon Davis 2015-04-06 12:08:09 PDT
Can we get any more traction on this issue?
Comment 12 Tim Horton 2015-04-15 14:54:55 PDT
Created attachment 250865 [details]
Comment 13 Simon Fraser (smfr) 2015-04-15 15:44:33 PDT
Comment on attachment 250865 [details]

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

> Source/WebCore/platform/mac/CursorMac.mm:48
> +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?
Comment 14 WebKit Commit Bot 2015-04-15 16:22:45 PDT
Comment on attachment 250865 [details]

Clearing flags on attachment: 250865

Committed r182869: <http://trac.webkit.org/changeset/182869>
Comment 15 WebKit Commit Bot 2015-04-15 16:22:52 PDT
All reviewed patches have been landed.  Closing bug.