286303 - Implement CSS3 syntax for cursor urls

Status: RESOLVED FIXED

(Core :: Widget, enhancement)

Description

[Christian :Biesinger (don't email me, ping me on IRC)]

The CSS3 syntax adds x/y coordinates for the hotspot; would be nice to support it.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This depends on the nsCSSValue changes in bug 3247.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch for style system end

This is what I was thinking of for the style system end.  I'm not sure what
changes need to happen for the imglib end, though, and it doesn't look like
there's already an API.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Imglib or widget, I should say.  It would be more convenient for the CSS code if
it passed the coords off to imagelib, but it could store them so that the layout
code can pass them directly to the widget code.

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

passing them to imagelib (like container->SetProperty("hotspotX", wrappedX);)
would be possible, but wouldn't it lead to wrong behaviour if the same image is
used with different hotspots in a page? (or maybe even across pages...)

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

right, so I need to do more here.  Another patch in a few days, hopefully...

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch (untested)

Not yet tested, and only partly compiled.

This should either fix or create an off-by-one bug in the OS/2 code; see bug
286306 comment 14.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch (untested)

This one should at least compile.

I guess I'm going to have to write testcases, since I don't see any here...

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

I tested this by using DOM inspector on attachment 185005 [details]; this turned up a bug
regarding serialization of style rules (the code assumed all ValueLists were
space-separated, but some are comma-separated).

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I added IID revs to nsIFrame and nsIWidget in my tree; I won't bother with a new
patch just for that.

Comment 10

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 186644 [details] [diff] [review]
patch

serialization is bug 254650, right?

content/events/src/nsEventStateManager.cpp
+    // css3-ui says that if the CSS specifies a hotspot, otherwise use
+    // the intrinsic hotspot, otherwise use the top left corner.

I think the first part of the sentence is missing a "use it" or something?

Comment 11

[Boris Zbarsky [:bzbarsky]]

David, I'm probably not going to get to this review until I get back in July. 
Please let me know asap if you really want me to try to look before then, ok?

Comment 12

[Rich Walsh]

(In reply to comment #6)
> This should either fix or create an off-by-one bug in the OS/2 code; see bug
> 286306 comment 14.

You're quite right, the OS/2 code should read "hotspotY = height - 1;" rather
than "hotspotY = height;".  I'll fix it.  However, the error has no real effect
because the os won't permit a hotspot to be out of bounds - it gets repositioned
at the appropriate extremity (in this case, height-1).  Thanks for noticing :-)

Comment 13

[Sergey Sokoloff]

Be aware of bug 296191. It's not a blocker, but potentially it can affect some
testcases.

Comment 14

[Christian :Biesinger (don't email me, ping me on IRC)]

I'll get to this review tomorrow. Sorry for the delay, I didn't have much time
in the last week.

Comment 15

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 186644 [details] [diff] [review]
patch

layout/style/nsStyleStruct.cpp
+nsStyleUserInterface::CopyCursorArrayFrom(const nsStyleUserInterface& aSource)
+{
+  mCursorArray = nsnull;

So.. this will leak if the array is non-null... maybe the header should contain
a comment about that?

layout/style/nsCSSDataBlock.cpp
+				 l->mValue.GetArrayValue()->Item(0).GetUnit()
== eCSSUnit_URL)

hm, can that ever be not a URL?

layout/style/nsRuleNode.cpp
+	     imgIRequest *req = arr->Item(0).GetImageValue();
+	     if (req) {
+	       item->mImage = req;

So... will this really do the right thing if the request is null? Above, you
only counted non-null image requests. So the array only has space for the
non-null requests.

But here, if it is null, you still increase item. Wouldn't that write past the
array bounds?

(iirc the request can be null for invalid uris and such, so it is a case that
can happen)

content/events/src/nsEventStateManager.cpp
+      hotspotX = PRUint32(aHotspotX);
+      hotspotY = PRUint32(aHotspotY);

should this make sure that the number is nonnegative, maybe?

Comment 16

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 186644 [details] [diff] [review]
patch

actually, I'll make that r- because of the null request issue.

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

This should address biesi's comments (still compiling, but only minor changes).

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Oops, I missed comment 10, and just fixed that in my tree to be:

    // css3-ui says to use the CSS-specified hotspot if present,
    // otherwise use the intrinsic hotspot, otherwise use the top left
    // corner.

Comment 19

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 187239 [details] [diff] [review]
patch

shouldn't content/events/public/nsIEventStateManager.h also get a new IID?
(sorry for missing that before)

content/events/src/nsEventStateManager.cpp
+      hotspotX = aHotspotX > 0.0f ? PRUint32(aHotspotX) : PRUint32(0);
+      hotspotY = aHotspotY > 0.0f ? PRUint32(aHotspotY) : PRUint32(0);

maybe this should round, not truncate? guess that's an edge case :)

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 187239 [details] [diff] [review]
patch

I'd like to get this in because I think it's important for url() cursors, which
are a new feature in 1.8.

Note to self:  worth thinking about what should happen when the coordinate is
larger than the height/width of the image.  We should probably do something
consistent to avoid platform differences.

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Checked in to trunk, 2005-06-30 21:29 -0700.

But I still need to look into comment 20, probably with a followup patch.

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: followup patch to prevent huge numbers from getting through to the various platform APIs

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Followup fix checked in to trunk, 2005-07-02 15:24 -0700.