Bug 100059 – custom CSS cursors ignore hotspot values embedded in CUR files

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Rick Byers]

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.

[Rick Byers]

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.

[Rick Byers]

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.

[Rick Byers]

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.

[Rick Byers]

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

[Rick Byers]

Created attachment 173302 [details]
Patch

[Rick Byers]

Created attachment 173303 [details]
Combined with fixes for 100550 and 101501 for EWS

[Early Warning System Bot]

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

[Early Warning System Bot]

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

[Rick Byers]

Created attachment 173309 [details]
Combined with fixes for 100550 and 101501 for EWS

[Build Bot]

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

[Rick Byers]

Created attachment 173400 [details]
Patch

[Rick Byers]

Created attachment 173402 [details]
Combined with fixes for 100550 and 101501 for EWS

[Rick Byers]

Peter, It looks like you've done a lot of work on the image decoders, can you please review this for me?

[Rick Byers]

Created attachment 173405 [details]
Patch

[Rick Byers]

Created attachment 173406 [details]
Combined with fixes for 100550 and 101501 for EWS

[Peter Kasting]

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?

[Rick Byers]

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.

[Rick Byers]

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

[Rick Byers]

Created attachment 173431 [details]
Tweaks based on pkastings review

[Rick Byers]

Created attachment 173432 [details]
Combined with fix for 101501

[Rick Byers]

Kenneth, it looks like you've reviewed a bunch of the image decoder changes in the past.  Can you please review this?

[WebKit Review Bot]

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

[Rick Byers]

Created attachment 173443 [details]
merge with ToT

[Peter Kasting]

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.

[Rick Byers]

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

[Peter Kasting]

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.

[Kenneth Russell]

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.

[Adam Barth]

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

[Kenneth Russell]

Comment on attachment 173443 [details]
merge with ToT

Adam, thanks for your input. r=me

[Rick Byers]

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

[Kenneth Russell]

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

[Rick Byers]

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

[Rick Byers]

Created attachment 174504 [details]
Patch for landing

[WebKit Review Bot]

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

[WebKit Review Bot]

Comment on attachment 174504 [details]
Patch for landing

Clearing flags on attachment: 174504

Committed r134874: <http://trac.webkit.org/changeset/134874>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.