Bug 99493 – CSS cursor property should support webkit-image-set

• Component: CSS
• Status: RESOLVED FIXED

[Rick Byers]

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.

[Rick Byers]

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

[Rick Byers]

Created attachment 169690 [details]
Rough initial working patch - still needs cleanup, ENABLE flag, and testing

[Beth Dakin]

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.

[Rick Byers]

(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?

[Beth Dakin]

(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.

[Rick Byers]

(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).

[Rick Byers]

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.

[Rick Byers]

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.

[Rick Byers]

I filed bug 100550 for adding basic infrastructure for testing mouse cursor changes.

[Rick Byers]

Created attachment 174805 [details]
Patch

[Rick Byers]

Created attachment 174806 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Early Warning System Bot]

Comment on attachment 174806 [details]
Patch

Attachment 174806 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14868447

[Early Warning System Bot]

Comment on attachment 174806 [details]
Patch

Attachment 174806 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14868448

[EFL EWS Bot]

Comment on attachment 174806 [details]
Patch

Attachment 174806 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14878167

[Rick Byers]

Created attachment 174810 [details]
Fix non-chromium build

[Rick Byers]

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!

[WebKit Review Bot]

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

[Adam Barth]

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.

[Rick Byers]

(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.

[Rick Byers]

Created attachment 174848 [details]
Patch

[Rick Byers]

Created attachment 174872 [details]
Merge with ToT

[Rick Byers]

Created attachment 174876 [details]
Patch

[Beth Dakin]

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.

[Rick Byers]

Created attachment 177002 [details]
Tweaks based on suggestions from bethdakin

[Rick Byers]

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.

[Beth Dakin]

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.

[Rick Byers]

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.

[Rick Byers]

Created attachment 178052 [details]
Patch for landing

[Early Warning System Bot]

Comment on attachment 178052 [details]
Patch for landing

Attachment 178052 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15186035

[Rick Byers]

Created attachment 178059 [details]
Fix non-chromium compile

[Build Bot]

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

[Rick Byers]

Created attachment 178075 [details]
Fix windows build

[Build Bot]

Comment on attachment 178075 [details]
Fix windows build

Attachment 178075 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15181248

[Rick Byers]

(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)
...

[WebKit Review Bot]

Comment on attachment 178075 [details]
Fix windows build

Clearing flags on attachment: 178075

Committed r136919: <http://trac.webkit.org/changeset/136919>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.