|Summary:||Custom CSS cursors do not use -webkit-image-set on retina displays|
|Component:||CSS||Assignee:||Tim Horton <thorton>|
|Severity:||Normal||CC:||ap, bdakin, commit-queue, devongovett+webkit, dino, jond, simon.fraser, thorton|
|Version:||528+ (Nightly build)|
|OS:||OS X 10.8|
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 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: (CoreIPC::ArgumentCoder<Cursor>::encode): 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 13 Simon Fraser (smfr) 2015-04-15 15:44:33 PDT
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?
Comment 14 WebKit Commit Bot 2015-04-15 16:22:45 PDT
Comment on attachment 250865 [details] rebased 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.