http://www.w3.org/TR/css3-ui/#cursor says this about specified hotspot values: "If the values are unspecified, then the intrinsic hotspot defined inside the image resource itself is used. If both the values are unspecific and the referenced cursor has no defined hotspot, the effect is as if a value of "0 0" were specified." WebKit has code for this in platform/Cursor.cpp in determineHotSpot, but the chromium port (ChromiumCursor.cpp) never calls this code. Instead, when a hotspot isn't explicitly specified, we just get a value of (-1,-1) which in WebCursor::ClampHotspot gets clamped to (0,0). On other ports (eg. those using LAZY_NATIVE_CURSOR) we fall back to determineHotSpot whenever the hotspot value is out-of-range, including the -1,-1 we get when there is none provided. We should either do the same on chromium, or we should plumb a 'has hotspot' flag through. See bug 99530 for some related discussion.
We currently have no infrastructure to allow us to test any fix for this, but I plan on adding some in bug 99493 so I'll block on that.
It looks like the existing code in WebKit for this is actually incomplete - ImageSource::getHotSpot always returns false, and ICOImageDecoder assumes that CUR and ICO files are exactly the same format (when in fact their directory structure is slightly different - see http://en.wikipedia.org/wiki/ICO_(file_format) ). Regardless this seems easy to fix. I've got it working for chromium at least, but I'll wait for bug 100550 and bug 101501 to land first.
I verified that Safari on Mac does handle CUR file hotspots correctly. My guess is that on some platforms, the CUR file is being handed directly to the OS. On chromium we decode the file in webkit and send a raw bitmap to the browser process (due to chromium's standboxing model). Anyway technically this fix isn't specific to chromium, but I'm not sure exactly which ports will benefit from it other than chromium.
(In reply to comment #3) > I verified that Safari on Mac does handle CUR file hotspots correctly. My guess is that on some platforms, the CUR file is being handed directly to the OS. Oh, I see now - mac has it's own implementation of ImageSource in platform/graphics/cg/ImageSourceCG.cpp, and in that implementation getHotSpot is implemented. In all other ports we use the base platform/graphics/ImageSource.cpp where it is stubbed out.
Created attachment 173302 [details] Patch
Created attachment 173303 [details] Combined with fixes for 100550 and 101501 for EWS
Comment on attachment 173303 [details] Combined with fixes for 100550 and 101501 for EWS Attachment 173303 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14766982
Comment on attachment 173303 [details] Combined with fixes for 100550 and 101501 for EWS Attachment 173303 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14788169
Created attachment 173309 [details] Combined with fixes for 100550 and 101501 for EWS
Comment on attachment 173309 [details] Combined with fixes for 100550 and 101501 for EWS Attachment 173309 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14786252 New failing tests: fast/events/mouse-cursor.html
Created attachment 173400 [details] Patch
Created attachment 173402 [details] Combined with fixes for 100550 and 101501 for EWS
Peter, It looks like you've done a lot of work on the image decoders, can you please review this for me?
Created attachment 173405 [details] Patch
Created attachment 173406 [details] Combined with fixes for 100550 and 101501 for EWS
Comment on attachment 173406 [details] Combined with fixes for 100550 and 101501 for EWS View in context: https://bugs.webkit.org/attachment.cgi?id=173406&action=review I'm not a WebKit reviewer, so I can't r+ this. In general the image decoder bits seem reasonable. > Source/WebCore/platform/image-decoders/ImageDecoder.h:382 > + virtual bool hotSpotAtIndex(size_t, IntPoint&) const { return false; } It doesn't seem like anyone external ever calls hotSpotAtIndex(). Why add it? > Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.cpp:139 > + // in processDirectoryEntries. More critical is being consistent with the frame that gets chosen when someone says "use this .cur file". Can we instead refer to whatever code does that (which presumably takes the first frame of the image)? > Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.cpp:309 > + entry.m_hotSpotY = readUint16(6); What about a hotspot coordinate outside the image bounds? Should we clamp to the image bounds (as Mozilla's docs claim to do, see https://developer.mozilla.org/en-US/docs/CSS/Using_URL_values_for_the_cursor_property ; I haven't checked their source code)? > Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.h:78 > + uint16_t m_hotSpotY; Can we just make a single IntPoint m_hotSpot member?
Comment on attachment 173406 [details] Combined with fixes for 100550 and 101501 for EWS View in context: https://bugs.webkit.org/attachment.cgi?id=173406&action=review >> Source/WebCore/platform/image-decoders/ImageDecoder.h:382 >> + virtual bool hotSpotAtIndex(size_t, IntPoint&) const { return false; } > > It doesn't seem like anyone external ever calls hotSpotAtIndex(). Why add it? I was just trying to be consistent with all the other functions which provide both frame-by-frame access and a default frame-unspecified access. But I guess I shouldn't bother until it's actually needed. >> Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.cpp:139 >> + // in processDirectoryEntries. > > More critical is being consistent with the frame that gets chosen when someone says "use this .cur file". Can we instead refer to whatever code does that (which presumably takes the first frame of the image)? Yep, I was just being lazy in not tracking that down. Done. >> Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.cpp:309 >> + entry.m_hotSpotY = readUint16(6); > > What about a hotspot coordinate outside the image bounds? Should we clamp to the image bounds (as Mozilla's docs claim to do, see https://developer.mozilla.org/en-US/docs/CSS/Using_URL_values_for_the_cursor_property ; I haven't checked their source code)? The only caller of this code ultimately accepts only values inside the image bounds (and falls back to 0,0 otherwise). See determineHotSpot in WebCore/platform/Cursor.cpp. I guess I should add a test case for that though - thanks. >> Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.h:78 >> + uint16_t m_hotSpotY; > > Can we just make a single IntPoint m_hotSpot member? Done, thanks.
(In reply to comment #17) > > What about a hotspot coordinate outside the image bounds? Should we clamp to the image bounds (as Mozilla's docs claim to do, see https://developer.mozilla.org/en-US/docs/CSS/Using_URL_values_for_the_cursor_property ; I haven't checked their source code)? > > The only caller of this code ultimately accepts only values inside the image bounds (and falls back to 0,0 otherwise). See determineHotSpot in WebCore/platform/Cursor.cpp. I guess I should add a test case for that though - thanks. Nevermind, I do already have this test case.
Created attachment 173431 [details] Tweaks based on pkastings review
Created attachment 173432 [details] Combined with fix for 101501
Kenneth, it looks like you've reviewed a bunch of the image decoder changes in the past. Can you please review this?
Comment on attachment 173431 [details] Tweaks based on pkastings review Attachment 173431 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14770957 New failing tests: inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html
Created attachment 173443 [details] merge with ToT
Comment on attachment 173406 [details] Combined with fixes for 100550 and 101501 for EWS View in context: https://bugs.webkit.org/attachment.cgi?id=173406&action=review >>> Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.cpp:309 >>> + entry.m_hotSpotY = readUint16(6); >> >> What about a hotspot coordinate outside the image bounds? Should we clamp to the image bounds (as Mozilla's docs claim to do, see https://developer.mozilla.org/en-US/docs/CSS/Using_URL_values_for_the_cursor_property ; I haven't checked their source code)? > > The only caller of this code ultimately accepts only values inside the image bounds (and falls back to 0,0 otherwise). See determineHotSpot in WebCore/platform/Cursor.cpp. I guess I should add a test case for that though - thanks. That sounds as if Mozilla will clamp to the nearest point on the boundary, whereas we'll clamp to the origin. I don't think we should differ. It would be nice to check their behavior as well as the Windows native behavior and try and be maximally consistent.
(In reply to comment #24) > (From update of attachment 173406 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=173406&action=review > > >>> Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.cpp:309 > >>> + entry.m_hotSpotY = readUint16(6); > >> > >> What about a hotspot coordinate outside the image bounds? Should we clamp to the image bounds (as Mozilla's docs claim to do, see https://developer.mozilla.org/en-US/docs/CSS/Using_URL_values_for_the_cursor_property ; I haven't checked their source code)? > > > > The only caller of this code ultimately accepts only values inside the image bounds (and falls back to 0,0 otherwise). See determineHotSpot in WebCore/platform/Cursor.cpp. I guess I should add a test case for that though - thanks. > > That sounds as if Mozilla will clamp to the nearest point on the boundary, whereas we'll clamp to the origin. I don't think we should differ. It would be nice to check their behavior as well as the Windows native behavior and try and be maximally consistent. Sure, I'll look into that. What about consistency with user-specified (via CSS) hotspots? WebKit has ignored (rather than clamped) out-of-range hot-spots for awhile - not sure I should be changing WebKit behavior in order to be consistent with Firefox. There's also code in chrome that clamps hotspots to the image boundary (WebCursor::ClampHotspot), but now that we're using this codepath (as of bug 101501) we may be ignoring out-of-range hotspots first (so in an effort to unify webkit code, I may have regressed chrome here). Anyway, I'll collect some more details on this so we can make a better informed decision. Perhaps it should be a separate bug though since it's potentially changing existing (as opposed to this new) behavior?
Comment on attachment 173406 [details] Combined with fixes for 100550 and 101501 for EWS View in context: https://bugs.webkit.org/attachment.cgi?id=173406&action=review >>>>> Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.cpp:309 >>>>> + entry.m_hotSpotY = readUint16(6); >>>> >>>> What about a hotspot coordinate outside the image bounds? Should we clamp to the image bounds (as Mozilla's docs claim to do, see https://developer.mozilla.org/en-US/docs/CSS/Using_URL_values_for_the_cursor_property ; I haven't checked their source code)? >>> >>> The only caller of this code ultimately accepts only values inside the image bounds (and falls back to 0,0 otherwise). See determineHotSpot in WebCore/platform/Cursor.cpp. I guess I should add a test case for that though - thanks. >> >> That sounds as if Mozilla will clamp to the nearest point on the boundary, whereas we'll clamp to the origin. I don't think we should differ. It would be nice to check their behavior as well as the Windows native behavior and try and be maximally consistent. > > Sure, I'll look into that. What about consistency with user-specified (via CSS) hotspots? WebKit has ignored (rather than clamped) out-of-range hot-spots for awhile - not sure I should be changing WebKit behavior in order to be consistent with Firefox. > > There's also code in chrome that clamps hotspots to the image boundary (WebCursor::ClampHotspot), but now that we're using this codepath (as of bug 101501) we may be ignoring out-of-range hotspots first (so in an effort to unify webkit code, I may have regressed chrome here). > > Anyway, I'll collect some more details on this so we can make a better informed decision. Perhaps it should be a separate bug though since it's potentially changing existing (as opposed to this new) behavior? Let's figure out what other people do first and that will tell us whether we need to also change our existing behavior. Then we can file new bugs as needed. If it came down to "consistency with everyone else versus consistency with past WebKit behavior" I'd definitely pick the first one, but I don't know that it will.
Comment on attachment 173443 [details] merge with ToT This looks good overall thanks to Peter's careful review, except for the issue of clamping the hotspot within the icon image. Despite the fact that the spec http://www.w3.org/TR/css3-ui/#cursor0 doesn't state what happens if the hotspot is outside the cursor's image, I think clamping it is the safer behavior to make the behavior of badly formed cursors less surprising. CC'ing abarth for an opinion of whether my concern is unfounded; if so, I'm willing to r+ this as is.
> I think clamping it is the safer behavior to make the behavior of badly formed cursors less surprising. That makes sense, but it's not that big an issue. The web page can always just hide the cursor and draw a fake cursor using an image element or whatever. Another thing to consider if transparent regions of the cursor image.
Comment on attachment 173443 [details] merge with ToT Adam, thanks for your input. r=me
I've filed bug 102398 to track changing the hotspot clamping behavior, and included a bunch of data there on current browser behavior (and how I've changed it). I propose we land this patch as is (once bug 100550 lands again), and deal with this question in the other bug (this bug doesn't really make things much/any worse, if anything bug 101501 did and should be the main point of contention).
(In reply to comment #30) > I've filed bug 102398 to track changing the hotspot clamping behavior, and included a bunch of data there on current browser behavior (and how I've changed it). I propose we land this patch as is (once bug 100550 lands again), and deal with this question in the other bug (this bug doesn't really make things much/any worse, if anything bug 101501 did and should be the main point of contention). Please mark the patch cq? (Details link) if you would like it submitted to the commit queue.
(In reply to comment #31) > (In reply to comment #30) > > I've filed bug 102398 to track changing the hotspot clamping behavior, and included a bunch of data there on current browser behavior (and how I've changed it). I propose we land this patch as is (once bug 100550 lands again), and deal with this question in the other bug (this bug doesn't really make things much/any worse, if anything bug 101501 did and should be the main point of contention). > > Please mark the patch cq? (Details link) if you would like it submitted to the commit queue. Yep, I was just waiting to verify that the change for bug 100550 (on which this depends) has passed all the builders. Thanks.
Created attachment 174504 [details] Patch for landing
Comment on attachment 174504 [details] Patch for landing Rejecting attachment 174504 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: ICOImageDecoder.cpp M Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.h Merge conflict during commit: Conflict at '/trunk/Source/WebCore/ChangeLog' at /usr/lib/git-core/git-svn line 570 Failed to run "[u'git', u'svn', u'dcommit']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue eue/Source/WebKit/chromium' ________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' Updating webkit projects from gyp files... Total errors found: 0 in 1 files Full output: http://queues.webkit.org/results/14834920
Comment on attachment 174504 [details] Patch for landing Clearing flags on attachment: 174504 Committed r134874: <http://trac.webkit.org/changeset/134874>
All reviewed patches have been landed. Closing bug.