Bug 102398 – Clamp cursor hotspot values that are out of range to be consistent with Chrome and Firefox (even though no one has complained)

• Status: REOPENED
• Component: CSS

[Rick Byers]

In bug 100059 we had some discussion on how exactly we should handle custom mouse cursors where the supplied hotspot value is outside the bounds of the image.  Note that the specification (http://www.w3.org/TR/css3-ui/#cursor) is silent on this.

I've tested the behavior on several browsers (using www.rbyers.net/cursor-hotspot.html) and here are the results.  Definitions:
- Explicit means a hot-spot listed in CSS using the CSS3 syntax after the URL.  
- Implicit means a CUR file with an embedded hotspot.
- Clamped means that the hot spot is minimally modified to fit within the image bounds
- Ignored means the invalid hotspot is treated as if it was unspecified (i.e. fall back to 0,0)
- N/A means the browser doesn't support that scenario (IE doesn't support hotspots specified in CSS and so skips any such css rules, and Chrome didn't used to support hot spots embedded in CUR files and so just ignores the hotspot value).

Browser	                           Explicit        Implicit
=============================================================
Chrome pre r134149                 Clamped	   N/A
Chrome post r134149                Ignored         N/A
Chrome w/ patch from  bug 100059   Ignored         Ignored
FireFox 16.0.2                     Clamped         Skipped image as bad
IE 10                              N/A             Clamped
Safari (6.0)                       Ignored         Ignored

Personally I think this is enough of an edge case and lacks consensus enough that it doesn't matter too much what we do as long as it's semi-reasonable.  Is it worth, for example, changing Safari's behavior?  Is it worth the extra complexity in WebKit to support different behavior for different ports?

[Kenneth Russell]

I'd suggest CC'ing other WebKit community members who have worked in this area of the code.

[Peter Kasting]

The fact that neither Firefox nor IE ever uses the "ignore" behavior suggests to me that all WebKit ports should change to "Clamped" for both cases.

I do not think we should write port-specific behaviors on this.

[Rick Byers]

Adding a few people who have worked on mouse cursors in the last little while.

In particular, ap@ added the Safari handling of CUR file hotspots here http://trac.webkit.org/changeset/60849.  Alexey, how would you feel about this changing to clamp out-of-range hotspots instead of ignoring them?

It's a bit of a pain to change this since we've been using -1,-1 to mean 'no hotspot specified'.  I'd have to plumb my 'hasHotspot' flag down all the way to Cursor.cpp.  But it shouldn't be a huge deal.

[Alexey Proskuryakov]

I don't know why we'd want to change a long-standing behavior that never caused any trouble, especially if doing that is painful, and thus likely to cause regressions due to mechanical errors.

[Peter Kasting]

(In reply to comment #4)
> I don't know why we'd want to change a long-standing behavior that never caused any trouble, especially if doing that is painful, and thus likely to cause regressions due to mechanical errors.

I don't know why we'd want to preserve a behavior that makes less sense and doesn't match what other browsers do when there's no clear advantage to doing so and the change is not too hard to implement.  "Likely to cause regressions" is simply wrong.  "Extremely unlikely to cause any regressions since it's a bit of very simple plumbing and easy to closely review" would be more accurate.

I'm also curious how you evaluated that differing from other browsers here has never caused any web authors any trouble.

[Alexey Proskuryakov]

Other browsers can match what WebKit does if they care. And I don't buy your "makes more sense" argument at all.

Please avoid causing unnecessary WebKit code churn.

> I'm also curious how you evaluated that differing from other browsers here has never caused any web authors any trouble.

A behavior that has not lead to bug reports from Web developers definitely falls into this bucket.

There are lots of actual bugs filed by people suffering from them here in Bugzilla. Please don't waste any more of anyone's time discussing this non-issue.

[Peter Kasting]

Wontfix is not an acceptable resolution for Chrome.  Please do not summarily close this bug.

For Chrome at least, our classic behavior for CSS-specified cursors was to clamp, consistent with everyone but Safari.  Only recently did this get changed to ignore.  And we didn't have support for .cur-specified hotspots.  Now that we're adding them it makes sense to consider immediately what their behavior should be.

In addition to noting that other browsers don't use the ignore behavior, we should consider that Windows native behavior is also to clamp (for .cur files).  This means there's a platform-native-consistency argument here as well.

Given that platform behavior, other browser behavior, and Chrome historical behavior all argue for clamping, it simply doesn't make sense for Chrome to do anything other than clamp.

My goal was to avoid a port-specific difference here, because most of these arguments also apply to Safari.  I am fine with worrying about excessive code churn to no benefit, or changes likely to cause bugs, but let's be realistic about this change.  It is basically a small bit of mechanical plumbing.  The amount of "code churn" seems extremely small, whereas at least for Chrome, not matching historical behavior, platform conventions, or other browsers seems to deserve a fix.  I'm just not seeing the tradeoff you are.  If I'm mistaken, though, I suppose we could implement this for Chrome only.

[Peter Kasting]

(In reply to comment #3)
> It's a bit of a pain to change this since we've been using -1,-1 to mean 'no hotspot specified'.

Can't we continue to use (-1, -1) to mean "no hotspot specified"?  Just because when a hotspot exists we want to clamp it to valid coords doesn't mean we can't preserve the use of a "signal" value.

If we can preserve this, then hopefully the change here becomes extremely minimal.

[Alexey Proskuryakov]

I don't know why you chose to keep wasting people's time.

The fact that Chrome didn't support cursor hotspots until now doesn't mean that it's somehow advisable to change behavior of browsers that did, or that it's advisable for Chrome to fork long established WebKit behavior.

This is a trivial and unimportant case, but it's a model for things we should not be doing in WebKit code base.

[Peter Kasting]

(In reply to comment #9)
> The fact that Chrome didn't support cursor hotspots until now

But we did, for CSS-specified hotspots.  And we clamped, until 6 days ago.

> doesn't mean that it's somehow advisable to change behavior of browsers that did

No, what makes that advisable are all of the other arguments presented.  You haven't responded to the platform-native behavior argument at all, you've ignored the past Chrome behavior, and your comment about other browsers is that they can change to match Safari.  I don't understand this response.

> or that it's advisable for Chrome to fork long established WebKit behavior.

Which is precisely why I suggested at first that we not fork.

> This is a trivial and unimportant case, but it's a model for things we should not be doing in WebKit code base.

I am just completely puzzled by your reactions on this bug.  What have we done to engender such antipathy?  How is making a small quick fix to align everyone's behavior something we "should not be doing"?  You're very insistent that we're wasting a bunch of time; wouldn't it have cost far less time for you to simply say at the beginning "I don't think this behavior matters much in the wild, but if you want to bring WebKit in line with everyone else, that's OK; the original behavior here was not chosen to intentionally deviate"?

My hope is that, when taking comment 8 into account, the patch to fix this will be very small and obviously not any kind of risk for mechanical error.  Would posting such a patch make you feel less hostile to this change?  If not, do you feel there's ever any rationale for changing WebKit behavior to align with other browsers aside from "web developers have filed bugs against the WebKit behavior"?  Can fellow developers not file bugs aiming for consistency?

I'm sorry if I didn't help things with comment 5.  It wasn't particularly helpful or kind.  But if it caused you to be more hostile to this change than otherwise, please reconsider, ignoring that comment.  Because while I can understand why you don't feel like it's very important which way we behave here, I don't understand why you seem strongly opposed to what to me looks like the obviously-correct choice.  I feel like I must be missing something.  Unless the argument is that it's really important for web compat that we not change the current behavior, in which case that also seems like an argument that this bug is very much not a waste of time and we should find out the actual compat ramifications so other vendors can do the right thing too.  But that doesn't seem like what you're saying.

[Alexey Proskuryakov]

A small patch would alleviate my main concern, caused by Rick's words that the patch involved pain and lots of plumbing.

I still think that making any changes here is unnecessary and poor investment of time, but we can just as well do this if there is emotional involvement. Risk of behavior regressions from the proposed change is extremely low (which was not necessarily the case for regressions from unintended changes introduced during refactoring).

[Peter Kasting]

(In reply to comment #11)
> A small patch would alleviate my main concern, caused by Rick's words that the patch involved pain and lots of plumbing.
> 
> I still think that making any changes here is unnecessary and poor investment of time, but we can just as well do this if there is emotional involvement. Risk of behavior regressions from the proposed change is extremely low (which was not necessarily the case for regressions from unintended changes introduced during refactoring).

Thanks, that's helpful.  I'll reconsider if the patch turns out to be invasive.

I also asked Tab Atkins (+CCed) if he had a perspective on this as a CSS editor.  He asked me to point him to the bug and he'd consider what the behavior should be, and noted that the CSS spec should probably not be silent on the issue the way it currently is.  I don't know if he intends to supply feedback immediately, but it seems like maybe if the spec gains clarity here that's a good outcome regardless.

[Tab Atkins]

(In reply to comment #12)
> I also asked Tab Atkins (+CCed) if he had a perspective on this as a CSS editor.  He asked me to point him to the bug and he'd consider what the behavior should be, and noted that the CSS spec should probably not be silent on the issue the way it currently is.  I don't know if he intends to supply feedback immediately, but it seems like maybe if the spec gains clarity here that's a good outcome regardless.

The spec isn't silent in the sense of "undefined", but rather in the sense of "nothing further needs to be said".

However, the specced behavior doesn't match any browser, and doesn't seem to have any particular use-case over one of the browser behaviors, so I'll bring this up in www-style and see which behavior seems most sensical to converge on.

[Maciej Stachowiak]

It seems likely to make web content authors' lives a tiny bit easier for this to be consistent between browsers.

If we can get a spec to define this and other vendors agree to converge, and if the change to do it is small, then it is probably reasonable. There doesn't seem to be an important reason for browsers to differ. And we shouldn't be overly reluctant to get consistency even if we don't know of specific bugs today.

On the other hand - it's not really clear to me that the Safari / new Chrome behavior is inferior to or less logical than other browsers. It's also unclear to me why the current behavior is unacceptable for Chrome, and why removing a port-specific behavior has to be in the direction of what Chrome used to do plus a behavior neither Safari or Chrome ever had. Is there a compatibility issue with it (perhaps even in just Chrome-specific content)?

I'm particularly curious why "it simply doesn't make sense for Chrome to do anything other than" what it used to do (when its support for cursors was less complete) but it presumptively makes sense for Safari to change to match Chrome. I agree that avoiding gratuitous differences in behavior between ports is good, but it kind of leaves a bad taste when it is presented as a one-way street. It seems like port consistency discussions often end up with the only two choices being "everyone does it Chrome's way" or "add an #ifdef". That is not a great basis for collaboration in general, though the stakes for this particular bug are quite low.

[Rick Byers]

(In reply to comment #8)
> (In reply to comment #3)
> > It's a bit of a pain to change this since we've been using -1,-1 to mean 'no hotspot specified'.
> 
> Can't we continue to use (-1, -1) to mean "no hotspot specified"?  Just because when a hotspot exists we want to clamp it to valid coords doesn't mean we can't preserve the use of a "signal" value.
> 
> If we can preserve this, then hopefully the change here becomes extremely minimal.

It's not a huge deal, but here's why it's probably non-trivial: At the point of style application (ApplyPropertyCursor::applyValue in StyleBuilder.cpp) we record only the hot spot value and ignore the bit (I recently added) for whether or not a hotspot was explicitly specified in CSS.  Then much later in Cursor's 'determineHotSpot' function we check if the CSS-supplied hotspot value is out of range, and if so fall back to looking inside the image for a hotspot value.  If instead we want to clamp out-of-range values then the right thing to do here is to plumb the bit through saying whether or not a hot spot was explicitly specified (I'm assuming we don't want to be in a weird state where explicitly specifying -1,-1 is different from explicitly specifying a 500,500).  Plumbing this bit through would involve at least the following changes:

RenderStyle::addCursor - add param
CursorData - add field / param
Cursor - add ctor param, use it in Cursor's determineHotSpot.  Since a bunch of ports have their own port-specific implementation of the Cursor ctor, this probably involves updating each of them too.

Again, probably not that big of a deal (especially since most ports now seem to have USE(LAZY_NATIVE_CURSOR)), but still a non-trivial change.

I don't personally care much either way about this bug.  If we can come to consensus about what the change should be, I'm happy to implement it (alternately feel free to reassign to yourself if you want).

[Peter Kasting]

(In reply to comment #15)
> It's not a huge deal, but here's why it's probably non-trivial: At the point of style application (ApplyPropertyCursor::applyValue in StyleBuilder.cpp) we record only the hot spot value and ignore the bit (I recently added) for whether or not a hotspot was explicitly specified in CSS.  Then much later in Cursor's 'determineHotSpot' function we check if the CSS-supplied hotspot value is out of range, and if so fall back to looking inside the image for a hotspot value.

Is this because we don't know what the valid range is at the time of applyValue()?

> If instead we want to clamp out-of-range values then the right thing to do here is to plumb the bit through saying whether or not a hot spot was explicitly specified (I'm assuming we don't want to be in a weird state where explicitly specifying -1,-1 is different from explicitly specifying a 500,500).

So basically, the worry is that if a .cur file has a valid hotspot, but the CSS explicitly specifies our invalid signal value, we'll end up using the .cur file value instead of ignoring or clamping the invalid CSS value?

If so there are three solutions I see:
(1) We plumb this as a separate bit as you've described.
(2) We ignore this case -- authors who do this get the .cur file value if any.
(3) Since all negative values are guaranteed to be invalid, we simply choose two different signal values -- one for "not specified" and one for "specified a negative value" -- and then map negative values to those signal values.  (We could even elect to only have one signal value for "not specified", and if authors specify that, simply adjust one of the coordinates slightly so it differs.)

The first case is perhaps clearest to read but involves more verbosity as well as a larger change.  The last would be simple to implement but somewhat magic.  The middle one is the simplest but people might not like this being the behavior.

[Tab Atkins]

(In reply to comment #16)
> (In reply to comment #15)
> > It's not a huge deal, but here's why it's probably non-trivial: At the point of style application (ApplyPropertyCursor::applyValue in StyleBuilder.cpp) we record only the hot spot value and ignore the bit (I recently added) for whether or not a hotspot was explicitly specified in CSS.  Then much later in Cursor's 'determineHotSpot' function we check if the CSS-supplied hotspot value is out of range, and if so fall back to looking inside the image for a hotspot value.
> 
> Is this because we don't know what the valid range is at the time of applyValue()?
> 
> > If instead we want to clamp out-of-range values then the right thing to do here is to plumb the bit through saying whether or not a hot spot was explicitly specified (I'm assuming we don't want to be in a weird state where explicitly specifying -1,-1 is different from explicitly specifying a 500,500).
> 
> So basically, the worry is that if a .cur file has a valid hotspot, but the CSS explicitly specifies our invalid signal value, we'll end up using the .cur file value instead of ignoring or clamping the invalid CSS value?
> 
> If so there are three solutions I see:
> (1) We plumb this as a separate bit as you've described.
> (2) We ignore this case -- authors who do this get the .cur file value if any.
> (3) Since all negative values are guaranteed to be invalid, we simply choose two different signal values -- one for "not specified" and one for "specified a negative value" -- and then map negative values to those signal values.  (We could even elect to only have one signal value for "not specified", and if authors specify that, simply adjust one of the coordinates slightly so it differs.)
> 
> The first case is perhaps clearest to read but involves more verbosity as well as a larger change.  The last would be simple to implement but somewhat magic.  The middle one is the simplest but people might not like this being the behavior.

(2) is dumb, and I'll explicitly add a test to the CSS testsuite for it if we use it.  We shouldn't expose magic behavior to authors.

I don't care between (1) and (3), as they're identical from the outside.

[Peter Kasting]

(In reply to comment #14)
> On the other hand - it's not really clear to me that the Safari / new Chrome behavior is inferior to or less logical than other browsers. It's also unclear to me why the current behavior is unacceptable for Chrome, and why removing a port-specific behavior has to be in the direction of what Chrome used to do plus a behavior neither Safari or Chrome ever had. Is there a compatibility issue with it (perhaps even in just Chrome-specific content)?
> 
> I'm particularly curious why "it simply doesn't make sense for Chrome to do anything other than" what it used to do (when its support for cursors was less complete) but it presumptively makes sense for Safari to change to match Chrome. I agree that avoiding gratuitous differences in behavior between ports is good, but it kind of leaves a bad taste when it is presented as a one-way street. It seems like port consistency discussions often end up with the only two choices being "everyone does it Chrome's way" or "add an #ifdef". That is not a great basis for collaboration in general, though the stakes for this particular bug are quite low.

I have been pushing too hard.  I will be happy to abide by whatever direction the www-style folks think is best.  I am sure they will have the points I consider below in mind, so their decision should be justifiable and rational whatever it is.

My primary aim was for all browsers to match each other, and secondarily to match native behavior.  It seemed unlikely other browsers (especially IE) would change to match WebKit, and even if they did, we would no longer match Windows native handling of cursors.  When combined with the fact that Chrome's behavior here had just changed a few days ago, there didn't seem to be much reason to try to push for the "ignore" behavior.  That doesn't justify rhetoric that amounts to holding a gun to other ports' heads, however.  You are right that this doesn't aid collaboration or make people feel good about agreeing on some direction to go, and I apologize.

[Tab Atkins]

www-style thread on the issue: http://lists.w3.org/Archives/Public/www-style/2012Nov/0331.html

[Rick Byers]

(In reply to comment #16)
> (In reply to comment #15)
> > It's not a huge deal, but here's why it's probably non-trivial: At the point of style application (ApplyPropertyCursor::applyValue in StyleBuilder.cpp) we record only the hot spot value and ignore the bit (I recently added) for whether or not a hotspot was explicitly specified in CSS.  Then much later in Cursor's 'determineHotSpot' function we check if the CSS-supplied hotspot value is out of range, and if so fall back to looking inside the image for a hotspot value.
> 
> Is this because we don't know what the valid range is at the time of applyValue()?

Right, we can't know until the image has been loaded.

> > If instead we want to clamp out-of-range values then the right thing to do here is to plumb the bit through saying whether or not a hot spot was explicitly specified (I'm assuming we don't want to be in a weird state where explicitly specifying -1,-1 is different from explicitly specifying a 500,500).
> 
> So basically, the worry is that if a .cur file has a valid hotspot, but the CSS explicitly specifies our invalid signal value, we'll end up using the .cur file value instead of ignoring or clamping the invalid CSS value?
> 
> If so there are three solutions I see:
> (1) We plumb this as a separate bit as you've described.
> (2) We ignore this case -- authors who do this get the .cur file value if any.
> (3) Since all negative values are guaranteed to be invalid, we simply choose two different signal values -- one for "not specified" and one for "specified a negative value" -- and then map negative values to those signal values.  (We could even elect to only have one signal value for "not specified", and if authors specify that, simply adjust one of the coordinates slightly so it differs.)

Correct.  #3 is a cute trick that didn't occur to me.  It's ugly though, I'd personally just do #1 - it's not THAT bad.  At this point the cost of this issue is probably dominated by the discussion time, not the code churn / regression risk...
 
> The first case is perhaps clearest to read but involves more verbosity as well as a larger change.  The last would be simple to implement but somewhat magic.  The middle one is the simplest but people might not like this being the behavior.

[Peter Kasting]

(In reply to comment #20)
> > (3) Since all negative values are guaranteed to be invalid, we simply choose two different signal values -- one for "not specified" and one for "specified a negative value" -- and then map negative values to those signal values.  (We could even elect to only have one signal value for "not specified", and if authors specify that, simply adjust one of the coordinates slightly so it differs.)
> 
> Correct.  #3 is a cute trick that didn't occur to me.  It's ugly though, I'd personally just do #1 - it's not THAT bad.  At this point the cost of this issue is probably dominated by the discussion time, not the code churn / regression risk...

#3 is akin how the image decoding system handles animation frames, but given that the GIF decoder is ugly already, that may not be good precedent :)

[Tab Atkins]

Based on initial responses from Mozilla <http://lists.w3.org/Archives/Public/www-style/2012Nov/0332.html> and IE <http://lists.w3.org/Archives/Public/www-style/2012Nov/0344.html>, I think we should switch the spec and our implementation over to clamping in all cases.

(The responses were underwhelming, but this isn't a very interesting problem in the first place.)

[Alexey Proskuryakov]

Note that ignoring out of range values seems to be in line with other CSS behaviors per bug 102735.

[Peter Kasting]

(In reply to comment #23)
> Note that ignoring out of range values seems to be in line with other CSS behaviors per bug 102735.

If you think that's precedent we should follow here, that would probably be a good thing to post to the www-style list?

[Tab Atkins]

(In reply to comment #23)
> Note that ignoring out of range values seems to be in line with other CSS behaviors per bug 102735.

You (and Emil, in the bug) are misinterpreting the line in the spec referenced by that bug.  That line is about *language-defined* ranges, which the author can be aware of ahead of time and unambiguously avoid just by being aware of what they're doing.

The bug in question, though, is about running into *implementation-defined* limits on the allowed range, which should *not* cause a rejection at parse time.  Doing so is author-hostile, as the author has no way to know what the range's limits are without testing, and these limits vary across browsers, and across time in a single browser.

This situation is a mix of the two.  It would be valid for us to define that negative values aren't allowed, and reject them at parse time.  However, we can't detect too-large values until the image loads, which is far too late to do a property rejection.  The most author-friendly behavior for late-detected errors is to respect the author's intent as well as we can, which in this case means clamping.

For simplicity, we might as well just clamp the negative numbers too, rather than rejecting them at parse time.  I'm not against early rejection, though.  If you feel strongly about it, please say so in the linked thread.  Either behavior is fine to me if we can agree on it across browsers.

[Alexey Proskuryakov]

> The most author-friendly behavior for late-detected errors is to respect the author's intent as well as we can, which in this case means clamping.

I don't think that a hotspot of (100, 100) on a 32x32 image can be qualified as author intent. It's just a mistake, and mistakes are ignored in CSS. You don't clamp a font-family of "ArialXXXXXXX" to Arial.

Again, this issue is not worth even 1% of attention is receives.

[Tab Atkins]

(In reply to comment #26)
> > The most author-friendly behavior for late-detected errors is to respect the author's intent as well as we can, which in this case means clamping.
> 
> I don't think that a hotspot of (100, 100) on a 32x32 image can be qualified as author intent. It's just a mistake, and mistakes are ignored in CSS. 

The author may not know the size of the image, and just wants it in the lower-right corner.  Or they may just have code that was written at one point, when the cursor was a particular size, and then the cursor image was swapped out later.

> You don't clamp a font-family of "ArialXXXXXXX" to Arial.

Obviously - there's no way to tell that "ArialXXXXXXX" is incorrect.  That's a valid font-name.

> Again, this issue is not worth even 1% of attention is receives.

Your point?  We still need to decide on a behavior.  IE and FF have said they prefer clamping behavior, and Chrome previously clamped before r134149.  Safari was the outlier.  If the behavior is unimportant, we should just change toward compat and have WebKit clamp.

[Rick Byers]

I'm unlikely to submit any patches for this -> Unassigned.  We did ultimately change the behavior in blink to clamp: http://crbug.com/172857.  It's not that important IMHO though.

[Brent Fulgham]

Is there actually any action needed on this bug? If so, could we open a new bug with a more specific description?

If this is about Chrome specifically, it doesn't seem to really be relevant to the WebKit bug tracker any longer.

[Darin Adler]

There are two possibilities for WebKit:

1) Leave the behavior as "ignore out-of-range hot spot values"
2) Change behavior to "clamp out-of-range hot spot values"

I think Alexey suggests we should stick with (1), and not make a change based on something no actual web developer has ever complained about it. Further, we have no indication this is a practical problem for anyone.

On the other hand, given Chrome's dominant market share and website developer mind share, "what Chrome does" may need to be treated as a de facto standard. I wish it weren’t so, but wishing won’t change things. Thus we can’t entirely rule out that it might be marginally helpful for interoperability to switch to (2).

[Rick Byers]

Note that this originally came up in Chrome as a surprise/confusion from a web developer when I changed Chrome to match Safari instead of all the other browsers: https://bugs.chromium.org/p/chromium/issues/detail?id=172857#c13. Happy to discuss and align in a standards forum if there's interest in unifying one way or the other. We could probably even collect some hard web compat data if that would be useful - like what fraction of pageloads we clamp hotspots on, and possibly even some example URLs. But I think we all agree this is far from being a high priority interop issue.

[Ahmad Saleem]

Blink Commit - https://src.chromium.org/viewvc/blink?view=revision&revision=188841

[Ahmad Saleem]

(In reply to Ahmad Saleem from comment #32)
> Blink Commit -
> https://src.chromium.org/viewvc/blink?view=revision&revision=188841

https://github.com/chromium/chromium/commit/16ffe820ce9aa984ee326fcd32b0ed4d91aaf167 (SVN is gone - so for future reference)