| Summary: | CSS cursor property should support webkit-image-set | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Rick Byers <rbyers> | ||||||||||||||||||||||||
| Component: | CSS | Assignee: | 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
Rick Byers
2012-10-16 13:05:06 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 Created attachment 169690 [details]
Rough initial working patch - still needs cleanup, ENABLE flag, and testing
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. (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? (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. (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). 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. 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. I filed bug 100550 for adding basic infrastructure for testing mouse cursor changes. Created attachment 174805 [details]
Patch
Created attachment 174806 [details]
Patch
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 on attachment 174806 [details] Patch Attachment 174806 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14868447 Comment on attachment 174806 [details] Patch Attachment 174806 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14868448 Comment on attachment 174806 [details] Patch Attachment 174806 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14878167 Created attachment 174810 [details]
Fix non-chromium build
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 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 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. (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. Created attachment 174848 [details]
Patch
Created attachment 174872 [details]
Merge with ToT
Created attachment 174876 [details]
Patch
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. Created attachment 177002 [details]
Tweaks based on suggestions from bethdakin
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 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 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. Created attachment 178052 [details]
Patch for landing
Comment on attachment 178052 [details] Patch for landing Attachment 178052 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/15186035 Created attachment 178059 [details]
Fix non-chromium compile
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 Created attachment 178075 [details]
Fix windows build
Comment on attachment 178075 [details] Fix windows build Attachment 178075 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15181248 (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 on attachment 178075 [details] Fix windows build Clearing flags on attachment: 178075 Committed r136919: <http://trac.webkit.org/changeset/136919> All reviewed patches have been landed. Closing bug. |