Bug 5689 – add support for CSS "custom cursors" (cursor images)

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

[Darin Adler]

[Darin Adler]

Created attachment 4652 [details]
adds custom cursor support, fixes some image renderer issues

[Eric Seidel (no email)]

Comment on attachment 4652 [details]
adds custom cursor support, fixes some image renderer issues

Wow, what a great cleanup.

I will warn you, this change is great:
+	 if (style->cursorImage())
+	     return new CSSPrimitiveValueImpl(style->cursorImage()->url(),
CSSPrimitiveValue::CSS_URI);
+	 return new CSSPrimitiveValueImpl(CSS_VAL_AUTO + style->cursor() -
CURSOR_AUTO);

but I just had to go through and remove a bunch of those same assumptions from
KSVG2 (going the other way, during css parsing) when landing on top of our DOM.
 Best IMO would be to add some sort of ASSERT like:
ASSERT((style->cursor() > CSS_AUTO) && (style->cursor() < CURSOR_HELP))
Whenever such assumptions are made when converting back and forth from ints to
enums.

Should this:
+		 parsedValue = new
CSSImageValueImpl(DOMString(KURL(styleElement->baseURL().qstring(),
uri.qstring()).url()), styleElement);

be:
parsedValue = new
CSSImageValueImpl(styleElement.getDocument().completeURL(uri), styleElement);

instead?

Shouldn't this:

+	 if (isInherit) {
+	     style->setCursor(parentStyle->cursor());
+	     style->setCursorImage(parentStyle->cursorImage());
+	     return;
+	 }
+	 if (isInitial) {
+	     style->setCursor(RenderStyle::initialCursor());
+	     style->setCursorImage(0);
+	     return;
+	 }

just be:

HANDLE_INHERIT_AND_INITIAL(cursor, Cursor)
HANDLE_INHERIT_AND_INITIAL(cursorImage, CursorImage)

?  Seems easier to add RenderStyle::initialCursorImage() if that was the
blocking factor.

Is this to avoid the obj-c message dispatch?
+    return m_imageRenderer == nil || [m_imageRenderer isNull];
Possibly unnecessary.

QSize will take an NSSize (and CGSize) directly, instead of:
+    NSSize sz = [m_imageRenderer size];
     return QSize((int)sz.width, (int)sz.height);

What an awful method name: QPixmap::xForm()


I'm not sure I understand why you have this:
+	     if ([view respondsToSelector:@selector(documentCursor)] &&
cur.handle() == [view documentCursor])
+		 break;

Is that to work around a bug in AppKit where setting the same cursor twice does
extra work?

So the patch looks fine.  IMO It doesn't need another round of review assuming
you respond to comments above.

[Eric Seidel (no email)]

Also, just FYI, SVG uses the same exact cursor property, and allow you to specify an arbitrary URI (just like 
your patch adds):
http://www.w3.org/TR/SVG/interact.html#CursorProperty
One difference is this uri might point to an SVG (or <cursor> element) which we'll need to convert into a 
pixmap and set as the cursorImage.  I will probably just need to extend CSSImageValueImpl (or the 
imaging system below CSSImageValueImpl).

[Darin Adler]

(In reply to comment #2)
> I will warn you, this change is great:
> +	 if (style->cursorImage())
> +	     return new CSSPrimitiveValueImpl(style->cursorImage()->url(),
> CSSPrimitiveValue::CSS_URI);
> +	 return new CSSPrimitiveValueImpl(CSS_VAL_AUTO + style->cursor() -
> CURSOR_AUTO);
> 
> but I just had to go through and remove a bunch of those same assumptions from
> KSVG2 (going the other way, during css parsing) when landing on top of our DOM.
> Best IMO would be to add some sort of ASSERT like:
> ASSERT((style->cursor() > CSS_AUTO) && (style->cursor() < CURSOR_HELP))
> Whenever such assumptions are made when converting back and forth from ints to
> enums.

I think we need a compile time check, not just a runtime assert. I think I'll just leave that part of the code as-is. What bothered me was 
that some code was making the assumption about the constants and other code was not.

> Should this:
> +		 parsedValue = new
> CSSImageValueImpl(DOMString(KURL(styleElement->baseURL().qstring(),
> uri.qstring()).url()), styleElement);
> 
> be:
> parsedValue = new
> CSSImageValueImpl(styleElement.getDocument().completeURL(uri), styleElement);
> 
> instead?

Maybe. I copied that code from CSS_PROP_LIST_STYLE_IMAGE, so we should make it the same in both places.

> Shouldn't this:
> 
> +	 if (isInherit) {
> +	     style->setCursor(parentStyle->cursor());
> +	     style->setCursorImage(parentStyle->cursorImage());
> +	     return;
> +	 }
> +	 if (isInitial) {
> +	     style->setCursor(RenderStyle::initialCursor());
> +	     style->setCursorImage(0);
> +	     return;
> +	 }
> 
> just be:
> 
> HANDLE_INHERIT_AND_INITIAL(cursor, Cursor)
> HANDLE_INHERIT_AND_INITIAL(cursorImage, CursorImage)
>
> ?

No, because if the style is inherit or initial, the first macro will return and the second macro won't do anything at all.

> Is this to avoid the obj-c message dispatch?
> +    return m_imageRenderer == nil || [m_imageRenderer isNull];
> Possibly unnecessary.

No. If the renderer is nil, then isNull will return false. We need to check for both.

> QSize will take an NSSize (and CGSize) directly, instead of:
> +    NSSize sz = [m_imageRenderer size];
>      return QSize((int)sz.width, (int)sz.height);

You're right. I believe i was the one who added the constructor to make a QSize from an NSSize, in fact, but that was after this code was 
written. I'm not sure it's mandatory to do that cleanup at this time, but maybe I'll do it.

> I'm not sure I understand why you have this:
> +	     if ([view respondsToSelector:@selector(documentCursor)] &&
> cur.handle() == [view documentCursor])
> +		 break;
> 
> Is that to work around a bug in AppKit where setting the same cursor twice does
> extra work?

Yes.

[Eric Seidel (no email)]

I've read your comments.  The only action item remaining is the completeURL call change.

[Darin Adler]

Turns out the completeURL change would be wrong. The URL is relative to the style sheet, not the 
document the style sheet is attached to.

[Eric Seidel (no email)]

Good to know.  Thanks for looking into this.