Bug 5689

Summary: add support for CSS "custom cursors" (cursor images)
Product: WebKit Reporter: Darin Adler <darin>
Component: CSSAssignee: Darin Adler <darin>
Status: RESOLVED FIXED    
Severity: Normal CC: eric
Priority: P2    
Version: 420+   
Hardware: Mac   
OS: OS X 10.4   
Attachments:
Description Flags
adds custom cursor support, fixes some image renderer issues eric: review+

Description Darin Adler 2005-11-09 09:03:11 PST
 
Comment 1 Darin Adler 2005-11-09 09:05:17 PST
Created attachment 4652 [details]
adds custom cursor support, fixes some image renderer issues
Comment 2 Eric Seidel (no email) 2005-12-03 14:42:09 PST
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.
Comment 3 Eric Seidel (no email) 2005-12-03 14:43:18 PST
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).
Comment 4 Darin Adler 2005-12-03 16:33:41 PST
(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.
Comment 5 Eric Seidel (no email) 2005-12-06 23:51:31 PST
I've read your comments.  The only action item remaining is the completeURL call change.
Comment 6 Darin Adler 2005-12-08 00:05:05 PST
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.
Comment 7 Eric Seidel (no email) 2005-12-08 00:14:14 PST
Good to know.  Thanks for looking into this.