Bug 99493

Summary: CSS cursor property should support webkit-image-set
Product: WebKit Reporter: Rick Byers <rbyers>
Component: CSSAssignee: Rick Byers <rbyers>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, abecsi, allan.jensen, bdakin, cmarcelo, dbates, dglazkov, eric, fishd, gyuyoung.kim, jamesr, kling, macpherson, menard, ojan.autocc, rakuco, rjkroege, sadrul, syoichi, tkent+wkapi, vestbo, webkit-ews, webkit.review.bot
Priority: P2 Keywords: WebExposed
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 99530, 99725, 100550, 102612    
Bug Blocks: 102579, 106242, 106455    
Attachments:
Description Flags
Rough initial working patch - still needs cleanup, ENABLE flag, and testing
none
Patch
none
Patch
none
Fix non-chromium build
none
Patch
none
Merge with ToT
none
Patch
none
Tweaks based on suggestions from bethdakin
none
Patch for landing
none
Fix non-chromium compile
none
Fix windows build none

Description Rick Byers 2012-10-16 13:05:06 PDT
Per discussion on www-style (http://lists.w3.org/Archives/Public/www-style/2012Jul/0140.html), the css 'cursor' property should support all types of <image>, not just <url>.  In particular, I want to add support for -webkit-image-set to enable high-dpi custom mouse cursors.

I've started to look into this, but I don't have a lot of context and it seems non-trivial.  I'm thinking I'll need to refactor CSSCursorImageValue so it doesn't inherit from CSSImageValue but instead contains a CSSValue for the image (which can be a CSSImageValue, CSSImageSetValue, and eventually also CSSImageGeneratorValue.
Comment 1 Rick Byers 2012-10-19 13:34:39 PDT
Note the CSS4 images spec has been updated to explicitly state that 'cursor' should support any <image> type (including an image-set): http://dev.w3.org/csswg/css4-images/#image-values
Comment 2 Rick Byers 2012-10-19 14:08:35 PDT
Created attachment 169690 [details]
Rough initial working patch - still needs cleanup, ENABLE flag, and testing
Comment 3 Beth Dakin 2012-10-19 14:41:31 PDT
I don't think we should add a new value here. I think instead of adding CSSCursorImageSetValue, that we should refactor CSSCursorImageValue to have either a regular image or an image set.
Comment 4 Rick Byers 2012-10-19 17:28:57 PDT
(In reply to comment #3)
> I don't think we should add a new value here. I think instead of adding CSSCursorImageSetValue, that we should refactor CSSCursorImageValue to have either a regular image or an image set.

Thanks Beth, I had come that conclusion too (see the TODO in the code).  I started that way, but it was feeling messy dealing with the SVG support stuff (which I'd rather not worry about now - much lower priority IMHO).  I'll look for a way to clean it up.

Seem like a reasonable approach to you otherwise?
Comment 5 Beth Dakin 2012-10-19 17:43:12 PDT
(In reply to comment #4)
> (In reply to comment #3)
> > I don't think we should add a new value here. I think instead of adding CSSCursorImageSetValue, that we should refactor CSSCursorImageValue to have either a regular image or an image set.
> 
> Thanks Beth, I had come that conclusion too (see the TODO in the code).  I started that way, but it was feeling messy dealing with the SVG support stuff (which I'd rather not worry about now - much lower priority IMHO).  I'll look for a way to clean it up.
> 
> Seem like a reasonable approach to you otherwise?

It does! This will be a great enhancement. I do wonder if it is necessary for PlatformCursor to cache the scale factor, but we can figure that out in time as the patch develops.
Comment 6 Rick Byers 2012-10-19 18:57:24 PDT
(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > I don't think we should add a new value here. I think instead of adding CSSCursorImageSetValue, that we should refactor CSSCursorImageValue to have either a regular image or an image set.
> > 
> > Thanks Beth, I had come that conclusion too (see the TODO in the code).  I started that way, but it was feeling messy dealing with the SVG support stuff (which I'd rather not worry about now - much lower priority IMHO).  I'll look for a way to clean it up.
> > 
> > Seem like a reasonable approach to you otherwise?
> 
> It does! This will be a great enhancement.

Thank you!  As we've been working with Google sites to get them to look great on the retina mac, this has been the one thing missing (it's noticeable in gmail for example).

> I do wonder if it is necessary for PlatformCursor to cache the scale factor, but we can figure that out in time as the patch develops.

Getting the scale into PlatformCursor is, I think, the essential piece.  The embedder reads it from there and then knows whether to, for example, show a 40x40 image as a small high-dpi cursor, or a large pixel-doubled cursor.  But yes, we can discuss the details when I've polished the patch (hopefully early next week).
Comment 7 Rick Byers 2012-10-22 08:03:57 PDT
I'm just looking at how best to test this.  It looks like changes to cursor support in the past have gone untested, eg. in r96566 darin@ wrote 'No tests because we currently don't have any test machinery for cursors.'.

I'd like to get at least some minimal testing of this in place.  Two main approaches I see:

1) Expose EventHandler::selectCursor through internals.idl
This is currently a private function, so I'll need to either use friend classes or come up with some cleaner abstraction to make this functionality available to tests.

2) Test at a port-specific event level via EventSender
Send fake mouse events, then use port-specific DRT code to watch the calls to Widget::setCursor.  For chromium this means exposing DRT's WebViewHost::m_currentCursor out to JavaScript somehow.

It's a shame to do something port-specific here, but I suspect that'll be more practical than trying to get the necessary public api surface area changes done for #1.  So I'm planning on taking approach #2.  Please let me know if you have any input.
Comment 8 Rick Byers 2012-10-25 12:29:50 PDT
Another reason to take approach #2 for testing: it's necessary to test port specific behavior like bug 100059.  So since I'll need to do it this way for bug 100059 anyway, I'll try to avoid having to also implement another approach to testing cursors.
Comment 9 Rick Byers 2012-10-26 12:11:44 PDT
I filed bug 100550 for adding basic infrastructure for testing mouse cursor changes.
Comment 10 Rick Byers 2012-11-16 22:05:10 PST
Created attachment 174805 [details]
Patch
Comment 11 Rick Byers 2012-11-16 22:19:05 PST
Created attachment 174806 [details]
Patch
Comment 12 WebKit Review Bot 2012-11-16 22:21:11 PST
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 13 Early Warning System Bot 2012-11-16 22:26:55 PST
Comment on attachment 174806 [details]
Patch

Attachment 174806 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14868447
Comment 14 Early Warning System Bot 2012-11-16 22:28:06 PST
Comment on attachment 174806 [details]
Patch

Attachment 174806 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14868448
Comment 15 EFL EWS Bot 2012-11-16 22:44:23 PST
Comment on attachment 174806 [details]
Patch

Attachment 174806 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14878167
Comment 16 Rick Byers 2012-11-16 22:47:16 PST
Created attachment 174810 [details]
Fix non-chromium build
Comment 17 Rick Byers 2012-11-16 22:50:15 PST
Beth, I think this is pretty solid now.  Still waiting for EWS results, and I'll probably do a build on Mac to do some additional manual testing to double-check that I didn't break any of the SVG cursor stuff.  But all the code is polished and tests are in place.  

Can you please take a look (I'm assuming you're the right person to review it).  Thanks!
Comment 18 WebKit Review Bot 2012-11-17 00:36:16 PST
Comment on attachment 174810 [details]
Fix non-chromium build

Attachment 174810 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14859662

New failing tests:
fast/css/cursor-parsing-image-set.html
Comment 19 Adam Barth 2012-11-17 10:08:24 PST
Comment on attachment 174810 [details]
Fix non-chromium build

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

API change LGTM with one nit.

> Source/WebKit/chromium/public/WebCursorInfo.h:98
> +    float imageScaleFactor;

Please initialize this scalar in the constructor.
Comment 20 Rick Byers 2012-11-18 08:29:58 PST
(In reply to comment #19)
> (From update of attachment 174810 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174810&action=review
> 
> API change LGTM with one nit.

Thanks Adam.  I guess it's a good idea to submit the API change separately (to decouple the chromium and webkit sides).  I've move it to bug 102612.

> 
> > Source/WebKit/chromium/public/WebCursorInfo.h:98
> > +    float imageScaleFactor;
> 
> Please initialize this scalar in the constructor.

Done.
Comment 21 Rick Byers 2012-11-18 09:09:38 PST
Created attachment 174848 [details]
Patch
Comment 22 Rick Byers 2012-11-18 18:16:28 PST
Created attachment 174872 [details]
Merge with ToT
Comment 23 Rick Byers 2012-11-18 19:25:06 PST
Created attachment 174876 [details]
Patch
Comment 24 Beth Dakin 2012-11-29 16:40:59 PST
Comment on attachment 174876 [details]
Patch

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

This is very close to r+! I just have a few minor comments I would like you to address first.

> Source/WebCore/css/CSSCursorImageValue.cpp:166
> +        if (m_imageValue->isImageValue())

Doesn't this need to be an 'else if' to avoid overriding m_image if it is SVG?

> Source/WebCore/css/CSSParser.cpp:1875
> +        // [<image> [<x> <y>]?,]*  [ auto | crosshair | default | pointer | progress | move | e-resize | ne-resize |

I'm not convinced we should make this change. We don't use <image> anywhere else in this class, so it's a little confusing.

> Source/WebCore/page/EventHandler.cpp:157
> +const double minimumCursorScale = 0.001;

Why is this the minimum scale?

> Source/WebCore/page/EventHandler.cpp:1450
> +                    cimage = static_cast<StyleCachedImage*>(image)->cachedImage();

I know this variable name was already here and is not your fault…but cimage is such a terrible name! Maybe change it to cursorImage while your changing this code anyway?

> Source/WebCore/page/EventHandler.cpp:1453
> +                    StyleCachedImageSet* imageSet = static_cast<StyleCachedImageSet*>(image);

This cast shouldn't be necessary. imageScaleFactor() is implemented as a virtual function on StyleImage in order to avoid this. I'm guessing that you need it though because of the call to cachedImage() which is not implemented on StyleImage. I think you should make cachedImage() virtual and add an implementation to StyleImage that returns null.

> Source/WebCore/page/EventHandler.cpp:1469
> +            // Limit the size of cursors (in ui pixels) so that they cannot be

Capitalize UI.

> Source/WebCore/page/EventHandler.cpp:1475
> +            Image* rimage = cimage->imageForRenderer(renderer);

I would just call this image.
Comment 25 Rick Byers 2012-11-30 11:45:30 PST
Created attachment 177002 [details]
Tweaks based on suggestions from bethdakin
Comment 26 Rick Byers 2012-11-30 11:46:27 PST
Comment on attachment 174876 [details]
Patch

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

>> Source/WebCore/css/CSSCursorImageValue.cpp:166
>> +        if (m_imageValue->isImageValue())
> 
> Doesn't this need to be an 'else if' to avoid overriding m_image if it is SVG?

I don't think so.  In the normal SVG case where resourceReferencedByCursorElement succeeds, I store and _return_ the SVG image above.  An else if would affect the case where we have a SVG URL, but resourceReferencedByCursorElement fails.  In that case I guess it's debatable whether we should just flat out return 0, or attempt to process the SVG URL as a normal URL (although I think the end result should be the same either way).  I don't fee like I understand 100% of the existing SVG cursor stuff (and having only a couple manual tests for it doesn't help), so I tried to keep the behavior as close to the previous code structure as possible.

>> Source/WebCore/css/CSSParser.cpp:1875
>> +        // [<image> [<x> <y>]?,]*  [ auto | crosshair | default | pointer | progress | move | e-resize | ne-resize |
> 
> I'm not convinced we should make this change. We don't use <image> anywhere else in this class, so it's a little confusing.

Sure, I was basically just copying this from the CSS3 specification.  Is this better now do you think?  I don't want to describe the entire image-set grammer here, and it looks like we don't have any other similar comments that I can see that use some token for image sets...

>> Source/WebCore/page/EventHandler.cpp:157
>> +const double minimumCursorScale = 0.001;
> 
> Why is this the minimum scale?

It's fairly arbitrary.  I've added a comment to explain the reasoning.  I'm happy to change it to something else if you prefer (eg. could probably be safely be as small as 0.00000005 to prevent overflow, but anything up to 1.0 is probably fine in terms of functionality).  

I didn't come across any scale factor clamps elsewhere in the webkit-image-set handling code - do you know that there's no unprotected divisions that could result in overflow?

>> Source/WebCore/page/EventHandler.cpp:1450
>> +                    cimage = static_cast<StyleCachedImage*>(image)->cachedImage();
> 
> I know this variable name was already here and is not your fault…but cimage is such a terrible name! Maybe change it to cursorImage while your changing this code anyway?

Yes, good idea.  I suspect the 'c' is for 'cached' not 'cursor' though.

>> Source/WebCore/page/EventHandler.cpp:1453
>> +                    StyleCachedImageSet* imageSet = static_cast<StyleCachedImageSet*>(image);
> 
> This cast shouldn't be necessary. imageScaleFactor() is implemented as a virtual function on StyleImage in order to avoid this. I'm guessing that you need it though because of the call to cachedImage() which is not implemented on StyleImage. I think you should make cachedImage() virtual and add an implementation to StyleImage that returns null.

Ah, that's much cleaner.  I've been able to simplify a bunch of this code as a result.  Thanks!

>> Source/WebCore/page/EventHandler.cpp:1469
>> +            // Limit the size of cursors (in ui pixels) so that they cannot be
> 
> Capitalize UI.

Done

>> Source/WebCore/page/EventHandler.cpp:1475
>> +            Image* rimage = cimage->imageForRenderer(renderer);
> 
> I would just call this image.

image was already taken for the StyleImage, but I've renamed it to be styleImage to be absolutely clear.
Comment 27 Beth Dakin 2012-12-04 11:28:15 PST
Comment on attachment 177002 [details]
Tweaks based on suggestions from bethdakin

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

> Source/WebCore/ChangeLog:6747
> +        (StyleCachedImageSet::cachedImage): Override new virtual.

This Changelog merge seems like it might be messed up?

> Source/WebCore/css/CSSParser.cpp:1908
> +        // [ [ <uri> | -webkit-image-set(...) ] [<x> <y>]?,]*

If this matches the spec, then I take back my previous objection.
Comment 28 Rick Byers 2012-12-06 11:38:07 PST
Comment on attachment 177002 [details]
Tweaks based on suggestions from bethdakin

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

>> Source/WebCore/ChangeLog:6747
>> +        (StyleCachedImageSet::cachedImage): Override new virtual.
> 
> This Changelog merge seems like it might be messed up?

Whoops - thanks for catching that!

>> Source/WebCore/css/CSSParser.cpp:1908
>> +        // [ [ <uri> | -webkit-image-set(...) ] [<x> <y>]?,]*
> 
> If this matches the spec, then I take back my previous objection.

Thanks.  It's a little ugly.  http://dev.w3.org/csswg/css3-ui/#cursor uses <uri>, but then http://dev.w3.org/csswg/css4-images/#image-type modifies it to replace <uri> by <image>.  I'll add a comment making that clear.  Note that we don't yet support all forms of image here (eg. gradients), but it's probably best to reflect the intent of the grammar.
Comment 29 Rick Byers 2012-12-06 12:02:44 PST
Created attachment 178052 [details]
Patch for landing
Comment 30 Early Warning System Bot 2012-12-06 12:12:45 PST
Comment on attachment 178052 [details]
Patch for landing

Attachment 178052 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15186035
Comment 31 Rick Byers 2012-12-06 12:21:56 PST
Created attachment 178059 [details]
Fix non-chromium compile
Comment 32 Build Bot 2012-12-06 13:48:32 PST
Comment on attachment 178059 [details]
Fix non-chromium compile

Attachment 178059 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15173494
Comment 33 Rick Byers 2012-12-06 14:01:32 PST
Created attachment 178075 [details]
Fix windows build
Comment 34 Build Bot 2012-12-06 15:11:23 PST
Comment on attachment 178075 [details]
Fix windows build

Attachment 178075 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15181248
Comment 35 Rick Byers 2012-12-06 17:31:34 PST
(In reply to comment #34)
> (From update of attachment 178075 [details])
> Attachment 178075 [details] did not pass mac-ews (mac):
> Output: http://queues.webkit.org/results/15181248

Looks like an infrastructure issue unrelated to this patch:

Touch /Volumes/Data/EWS/WebKit/WebKitBuild/Release/WebCore.framework
    cd /Volumes/Data/EWS/WebKit/Source/WebCore
    /usr/bin/touch -c /Volumes/Data/EWS/WebKit/WebKitBuild/Release/WebCore.framework

** INTERNAL ERROR: Uncaught exception **
Exception: *** -[NSFileManager fileSystemRepresentationWithPath:]: conversion failed for /Volumes/Data/EWS/WebKit/We/Volumes/Data/EWS/WebKit/Source/JavaScriptCore
Stack:
  0  0x00007fff8db5d08e __exceptionPreprocess (in CoreFoundation)
  1  0x00007fff8c8a03f0 objc_exception_throw (in libobjc.A.dylib)
  2  0x00007fff8db5ce7c +[NSException raise:format:] (in CoreFoundation)
  3  0x00007fff8eb0650e -[NSFileManager fileSystemRepresentationWithPath:] (in Foundation)
  4  0x000000010dbc0e87 -[XCBuildNodeState writeToFileHandle:] (in DevToolsCore)
  5  0x000000010dbbea8a -[XCBuildStateStore writeToFileHandle:] (in DevToolsCore)
  6  0x000000010dabf7ce -[PBXTargetBuildContext writeDependencyInfoCacheToBuildDirectory:createIfNecessary:] (in DevToolsCore)
  7  0x000000010da7d49a -[PBXTarget(XCBuildables) postprocessingAfterBuildingWithBuildParameters:] (in DevToolsCore)
...
Comment 36 WebKit Review Bot 2012-12-06 19:08:22 PST
Comment on attachment 178075 [details]
Fix windows build

Clearing flags on attachment: 178075

Committed r136919: <http://trac.webkit.org/changeset/136919>
Comment 37 WebKit Review Bot 2012-12-06 19:08:30 PST
All reviewed patches have been landed.  Closing bug.