Closed Bug 286303 Opened 20 years ago Closed 19 years ago

Implement CSS3 syntax for cursor urls

Categories

(Core :: Widget, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.8beta2

People

(Reporter: Biesinger, Assigned: dbaron)

References

()

Details

(Keywords: css3, Whiteboard: [patch])

Attachments

(2 files, 4 obsolete files)

The CSS3 syntax adds x/y coordinates for the hotspot; would be nice to support it.
This depends on the nsCSSValue changes in bug 3247.
Depends on: 3247
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla1.8beta2
Attached patch patch for style system end (obsolete) — Splinter Review
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.
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.
Assignee: dbaron → general
Status: ASSIGNED → NEW
Component: Style System (CSS) → Widget
QA Contact: ian
Priority: P1 → --
Target Milestone: mozilla1.8beta2 → ---
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...)
Assignee: general → cbiesinger
Target Milestone: --- → mozilla1.8beta3
right, so I need to do more here.  Another patch in a few days, hopefully...
Assignee: cbiesinger → dbaron
Target Milestone: mozilla1.8beta3 → mozilla1.8beta2
Attached patch patch (untested) (obsolete) — Splinter Review
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.
Attachment #179662 - Attachment is obsolete: true
Attached patch patch (untested) (obsolete) — Splinter Review
This one should at least compile.

I guess I'm going to have to write testcases, since I don't see any here...
Attachment #186633 - Attachment is obsolete: true
Attached patch patch (obsolete) — Splinter Review
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).
Attachment #186634 - Attachment is obsolete: true
Attachment #186644 - Flags: superreview?(bzbarsky)
Attachment #186644 - Flags: review?(cbiesinger)
I added IID revs to nsIFrame and nsIWidget in my tree; I won't bother with a new
patch just for that.
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?
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?
(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 :-)
Be aware of bug 296191. It's not a blocker, but potentially it can affect some
testcases.
I'll get to this review tomorrow. Sorry for the delay, I didn't have much time
in the last week.
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?
Attachment #186644 - Flags: review?(cbiesinger) → review+
Comment on attachment 186644 [details] [diff] [review]
patch

actually, I'll make that r- because of the null request issue.
Attachment #186644 - Flags: review+ → review-
Attached patch patchSplinter Review
This should address biesi's comments (still compiling, but only minor changes).
Attachment #186644 - Attachment is obsolete: true
Attachment #187239 - Flags: superreview?(bzbarsky)
Attachment #187239 - Flags: review?(cbiesinger)
Attachment #186644 - Flags: superreview?(bzbarsky)
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 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 :)
Attachment #187239 - Flags: review?(cbiesinger) → review+
Attachment #187239 - Flags: superreview?(bzbarsky) → superreview+
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.
Attachment #187239 - Flags: approval1.8b3?
Attachment #187239 - Flags: approval1.8b3? → approval1.8b3+
Checked in to trunk, 2005-06-30 21:29 -0700.

But I still need to look into comment 20, probably with a followup patch.
Attachment #187993 - Flags: review?(cbiesinger) → review+
Attachment #187993 - Flags: superreview?(bzbarsky) → superreview+
Attachment #187993 - Flags: approval1.8b3? → approval1.8b3+
Followup fix checked in to trunk, 2005-07-02 15:24 -0700.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: