Bug 100059

Summary:

custom CSS cursors ignore hotspot values embedded in CUR files

Product:

WebKit

Reporter:

Rick Byers <rbyers>

Component:

CSS

Assignee:

Rick Byers <rbyers>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, cc-bugs, dglazkov, jamesr, kbr, pkasting, webkit-ews, webkit.review.bot

Priority:

Keywords:

WebExposed

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

100550, 101501

Bug Blocks:

102398, 101811

Attachments:

Description	Flags
Patch	none
Combined with fixes for 100550 and 101501 for EWS	none
Combined with fixes for 100550 and 101501 for EWS	none
Patch	none
Combined with fixes for 100550 and 101501 for EWS	none
Patch	none
Combined with fixes for 100550 and 101501 for EWS	none
Tweaks based on pkastings review	none
Combined with fix for 101501	none
merge with ToT	none
Patch for landing	none

Rick Byers

Reported 2012-10-22 18:27:05 PDT

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.

Attachments
Patch (13.62 KB, patch) 2012-11-09 07:22 PST, Rick Byers	no flags	Details Formatted Diff Diff
Combined with fixes for 100550 and 101501 for EWS (60.91 KB, patch) 2012-11-09 07:23 PST, Rick Byers	no flags	Details Formatted Diff Diff
Combined with fixes for 100550 and 101501 for EWS (60.90 KB, patch) 2012-11-09 07:44 PST, Rick Byers	no flags	Details Formatted Diff Diff
Patch (19.44 KB, patch) 2012-11-09 16:42 PST, Rick Byers	no flags	Details Formatted Diff Diff
Combined with fixes for 100550 and 101501 for EWS (64.69 KB, patch) 2012-11-09 16:44 PST, Rick Byers	no flags	Details Formatted Diff Diff
Patch (19.44 KB, patch) 2012-11-09 17:03 PST, Rick Byers	no flags	Details Formatted Diff Diff
Combined with fixes for 100550 and 101501 for EWS (64.69 KB, patch) 2012-11-09 17:04 PST, Rick Byers	no flags	Details Formatted Diff Diff
Tweaks based on pkastings review (19.53 KB, patch) 2012-11-09 20:44 PST, Rick Byers	no flags	Details Formatted Diff Diff
Combined with fix for 101501 (47.33 KB, patch) 2012-11-09 20:45 PST, Rick Byers	no flags	Details Formatted Diff Diff
merge with ToT (19.52 KB, patch) 2012-11-10 04:33 PST, Rick Byers	no flags	Details Formatted Diff Diff
Patch for landing (19.33 KB, patch) 2012-11-15 13:07 PST, Rick Byers	no flags	Details Formatted Diff Diff
Show Obsolete (10) View All Add attachment proposed patch, testcase, etc.

Rick Byers

Comment 1 2012-10-23 06:40:32 PDT

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

Comment 2 2012-11-08 14:27:06 PST

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

Comment 3 2012-11-08 14:30:42 PST

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

Comment 4 2012-11-09 07:19:36 PST

(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

Comment 5 2012-11-09 07:22:51 PST

Created attachment 173302 [details] Patch

Rick Byers

Comment 6 2012-11-09 07:23:35 PST

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

Early Warning System Bot

Comment 7 2012-11-09 07:32:10 PST

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 8 2012-11-09 07:33:34 PST

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

Comment 9 2012-11-09 07:44:35 PST

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

Build Bot

Comment 10 2012-11-09 10:47:40 PST

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

Comment 11 2012-11-09 16:42:17 PST

Created attachment 173400 [details] Patch

Rick Byers

Comment 12 2012-11-09 16:44:31 PST

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

Rick Byers

Comment 13 2012-11-09 16:50:46 PST

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

Rick Byers

Comment 14 2012-11-09 17:03:39 PST

Created attachment 173405 [details] Patch

Rick Byers

Comment 15 2012-11-09 17:04:07 PST

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

Peter Kasting

Comment 16 2012-11-09 17:33:51 PST

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 17 2012-11-09 20:26:29 PST

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

Comment 18 2012-11-09 20:29:24 PST

(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

Comment 19 2012-11-09 20:44:27 PST

Created attachment 173431 [details] Tweaks based on pkastings review

Rick Byers

Comment 20 2012-11-09 20:45:56 PST

Created attachment 173432 [details] Combined with fix for 101501

Rick Byers

Comment 21 2012-11-09 20:48:18 PST

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 22 2012-11-10 00:42:14 PST

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

Comment 23 2012-11-10 04:33:55 PST

Created attachment 173443 [details] merge with ToT

Peter Kasting

Comment 24 2012-11-10 10:48:20 PST

Rick Byers

Comment 25 2012-11-10 12:20:17 PST

(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 26 2012-11-10 15:02:52 PST

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 27 2012-11-12 12:01:50 PST

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

Comment 28 2012-11-12 13:06:08 PST

> 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 29 2012-11-12 13:18:01 PST

Comment on attachment 173443 [details] merge with ToT Adam, thanks for your input. r=me

Rick Byers

Comment 30 2012-11-15 09:15:11 PST

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

Comment 31 2012-11-15 10:40:33 PST

(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

Comment 32 2012-11-15 12:19:20 PST

(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

Comment 33 2012-11-15 13:07:24 PST

Created attachment 174504 [details] Patch for landing

WebKit Review Bot

Comment 34 2012-11-15 15:46:38 PST

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 35 2012-11-15 18:28:34 PST

Comment on attachment 174504 [details] Patch for landing Clearing flags on attachment: 174504 Committed r134874: <http://trac.webkit.org/changeset/134874>

WebKit Review Bot

Comment 36 2012-11-15 18:28:41 PST

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.