Bug 6002 - WebKit does not properly handle SVG <cursor> element
Summary: WebKit does not properly handle SVG <cursor> element
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P4 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 6001
Blocks:
  Show dependency treegraph
 
Reported: 2005-12-08 01:50 PST by Eric Seidel (no email)
Modified: 2006-09-26 05:39 PDT (History)
2 users (show)

See Also:


Attachments
cursor should be "hand" on circle  (488 bytes, image/svg+xml)
2006-05-10 02:51 PDT, jay
no flags Details
First attempt (21.15 KB, patch)
2006-05-21 03:35 PDT, Rob Buis
ap: review-
Details | Formatted Diff | Diff
Improved patch after ap's feedback (28.50 KB, patch)
2006-05-23 13:26 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch addressing MacDome's suggestions (28.81 KB, patch)
2006-05-24 07:25 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Improved patch (26.77 KB, patch)
2006-06-02 13:56 PDT, Rob Buis
darin: review-
Details | Formatted Diff | Diff
test case for HTML (put in WebCore/manual-tests) (2.29 KB, application/octet-stream)
2006-07-25 05:19 PDT, Alexey Proskuryakov
no flags Details
test case for SVG (7.65 KB, application/zip)
2006-07-26 03:49 PDT, Alexey Proskuryakov
no flags Details
Another incomplete attempt (12.13 KB, patch)
2006-09-07 02:05 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Improved patch (34.13 KB, patch)
2006-09-08 14:54 PDT, Rob Buis
eric: review-
Details | Formatted Diff | Diff
Another go (33.06 KB, patch)
2006-09-10 00:18 PDT, Rob Buis
eric: review-
Details | Formatted Diff | Diff
Another go (31.87 KB, patch)
2006-09-10 11:00 PDT, Rob Buis
darin: review-
Details | Formatted Diff | Diff
Another attempt (31.46 KB, patch)
2006-09-10 13:58 PDT, Rob Buis
darin: review-
Details | Formatted Diff | Diff
Even better patch (31.59 KB, patch)
2006-09-11 05:22 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Another attempt (31.18 KB, patch)
2006-09-15 04:42 PDT, Rob Buis
eric: review-
Details | Formatted Diff | Diff
css parser fixes (28.94 KB, patch)
2006-09-22 02:48 PDT, Rob Buis
eric: review-
Details | Formatted Diff | Diff
Improved testcase (3.02 KB, text/html)
2006-09-22 03:16 PDT, Rob Buis
no flags Details
Improved patch+testcases (37.80 KB, patch)
2006-09-23 13:55 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Improved testcases (44.64 KB, patch)
2006-09-24 07:01 PDT, Rob Buis
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2005-12-08 01:50:41 PST
WebKit does not properly handle SVG <cursor> element

Nearly all the code is there, it just needs to be wired into Darin's new custom cursor code:
http://bugzilla.opendarwin.org/show_bug.cgi?id=5689
Comment 1 Eric Seidel (no email) 2005-12-08 02:30:00 PST
This is going to need to have at least part of the code in CSSParser::parseValue, and could look 
something like this:

            DOMString uri = parseURL(domString(value->string));
#if SVG_SUPPORT
            if (uri.find("#") == 0) {
                ElementImpl *e = document.getElementById(uri.qstring.mid(1));
                DOMString url = e->getAttribute(XLinkNames::hrefAttr);
                parsedValue = new CSSImageValueImpl(DOMString(KURL(styleElement->baseURL().qstring(), 
uri.qstring()).url()), styleElement);
                valueList->next();
            } else
#endif
            if (!uri.isEmpty()) {
                parsedValue = new CSSImageValueImpl(DOMString(KURL(styleElement->baseURL().qstring(), 
uri.qstring()).url()), styleElement);
                valueList->next();
            }
	}

That still doesn't handle the fallback case (as addressed by 6001), nor does it assure that the targeted 
element is a <cursor>, nor does it recursively handle cursor referencing some other <cursor> (if that's 
even possible).  But it it should give anyone looking to implement this, a head start.
Comment 2 jay 2006-05-10 02:51:23 PDT
Created attachment 8206 [details]
cursor should be "hand" on circle 

mouseover circle which has anchor, cursor pointer should be "hand"
Comment 3 jay 2006-05-10 02:53:52 PDT
Eric,

have added attachment demonstrating that pointer should change to "hand" over anchor.

otherwise how is visitor to know this is a link?

status bar should also display URI, but perhaps that's another bug...
Comment 4 Rob Buis 2006-05-21 03:35:07 PDT
Created attachment 8441 [details]
First attempt

My first stab at fixing this. It works for svg cursors, including hotspot.
It turned out the handling of fallback for the urls was so related
that this patch also attempts to fix that, that means this patch also
should fix bug 6001.
Cheers,

Rob.
Comment 5 Alexey Proskuryakov 2006-05-21 05:00:35 PDT
Comment on attachment 8441 [details]
First attempt

I think I have enough comments to make a first round of a review :)

+        if (style->cursors())
+            return new CSSPrimitiveValue(style->cursors()->cursor_image->url(), CSSPrimitiveValue::CSS_URI);

In Firefox, the computed style includes the whole list, not just one URI (I tested with a bookmarklet from <http://www.squarefree.com/bookmarklets/webdevel.html>).

+class CSSCursorImageValue : public CSSImageValue

We usually have one class per file.

+        //  [<uri>,]* [ auto | crosshair | default | pointer | progress | move | e-resize | ne-resize |
...
+            valueList->next(); // comma
+            value = valueList->next();

It's probably OK not to support CSS3 hotspots at first, but here, the parser would just get confused by them, which isn't good at all. Also, is it possible to move the parsing logic to CSSGrammar.y?

+            if (!value) { // no value after url list (MSIE 5 compatibility)

Nitpick: I think custom cursors were implemented in MSIE 6.

+struct CursorInfo {
+    ~CursorInfo();
+    void clearCursor();
+    CachedImage *cursor_image;
+#if SVG_SUPPORT
+    IntPoint cursor_hotspot;
+#endif
+    struct CursorInfo *next;
+};

Special-casing SVG doesn't seem necessary for dealing with the hotspot (even if we don't support CSS3 hotspots, the parser can just pass {0,0}).

-      cursor_image( o.cursor_image ), font( o.font ),
+      cursor_info( o.cursor_info ), font( o.font ),

StyleInheritedData didn't own cursor_image, but it owns cursor_info, so just copying a pointer isn't enough.

+void RenderStyle::addCursor( CachedImage *v )

Style violation - we don't use extra spaces in braces (same issue elsewhere).


All fallback cursors are being loaded and decoded here, even if only the first one gets used. I am not sure if this can be considered acceptable, but it's definitely not good.

Of course, this change needs extensive test cases.
Comment 6 Rob Buis 2006-05-23 13:26:22 PDT
Created attachment 8494 [details]
Improved patch after ap's feedback

This should address most of ap's issues with the first patch. I now do really lazy loading,
only trying to load the cursor image when requested on a mouse move. If that is too lazy,
in cssstyleselector we could start loading at least the first image cursor (if there are one or
more obviously). The CursorInfo list is getting really complex, so I wonder if it is correct
and could be simplified. Finally I'd like some pointers for testcases, testing problems 6001+6002
...
Cheers,

Rob.
Comment 7 Eric Seidel (no email) 2006-05-23 13:39:19 PDT
Comment on attachment 8494 [details]
Improved patch after ap's feedback

+        } else if (value)
+            return value;
         ASSERT_NOT_REACHED();

this would be clearer as
ASSERT(value);
return value;

style issues:
+        CSSValueList* list = 0;
+        CSSValue *value = 0;

This shoudl be broken up into multiple lines to be more readable:
+                    list->append(new CSSCursorImageValue(String(KURL(styleElement->baseURL().deprecatedString(), url.deprecatedString()).url()), IntPoint(x, y), styleElement));

no {} needed here:
+            } else if (!strict && value->id == CSS_VAL_HAND) { // MSIE 5 compatibility :/
+                list->append(new CSSPrimitiveValue(CSS_VAL_POINTER));
+            } else if (value->id >= CSS_VAL_AUTO && value->id <= CSS_VAL_HELP)

style:
+            for (int i = 0; i < len; i++)
+            {

we don't generally commit commented out code:
+                    //if (!style->cursors()) // load first
+                    //    image->image(element->document()->docLoader());

more style:
+                } else if (type == CSSPrimitiveValue::CSS_IDENT) {
+                    style->setCursor((ECursor)(primitiveValue->getIdent() - CSS_VAL_AUTO));
+                }

more style:
+        if (SVGURIReference::parseMappedAttribute(attr)) return;

(return should be on a separate line)

style:
+            if (!cimage) continue;

this shoudl probably have an argument label to indicate that the IntPoint is a hotspot:

+        Cursor(Image*, IntPoint = IntPoint());

same here, ideally p should be named "hotspot" or something more descriptive:
+static NSCursor* createCustomCursor(Image* image, IntPoint p)

These don't need argument labels, as they provide no value here:
+    void setCursors(CursorInfo* o);
+    void addCursor(CSSCursorImageValue* v);

more style:
+: m_cursor(cursor), m_next(0)

+    if (m_cursor) m_cursor->ref();

Mostly I ran through style issues in my review.
Comment 8 Rob Buis 2006-05-24 07:25:35 PDT
Created attachment 8512 [details]
Patch addressing MacDome's suggestions

This patch should fix the style issues raised by MacDome. Please let me know
whether technically the patch is right and how to do the testcases. In particular
I am still a bit worried about the refcounting and handling of CursorInfo in various
situations...
Cheers,

Rob.
Comment 9 Alexey Proskuryakov 2006-05-24 12:13:09 PDT
Comment on attachment 8512 [details]
Patch addressing MacDome's suggestions

I think that a few additional cases can be added to manual-tests/custom-cursors.html, and a similar SVG test should be added (based on the attached test?). Some ideas about what to test:
- standard and IE6 (incomplete) declarations should parse OK;
- CSS3 hotspot values should not break parsing;
- fallback should work for both missing images and ones that failed to decode;
- should be able to fallback to a named cursor (implied, in incomplete case), as well as to a custom one;
- should support SVG cursors (as in url(cursor.svg) and url(example.svg#linkcursor)).

Google suggests that this may be of use: http://www.gtalbot.org/DHTMLSection/Cursors.html

The handling of CursorInfo doesn't look quite right to me - StyleInheritedData::operator==() will not compare copies as equal, because the address of cursorInfo will be different. Can CursorInfo itself be made refcountable? I'd also try to use Vector, rather that create a brand new list implementation.

I haven't reviewed the rest of the patch, so I'm leaving the flag as is, hoping that someone will comment more.
Comment 10 Rob Buis 2006-06-02 13:56:05 PDT
Created attachment 8669 [details]
Improved patch

This version uses Vector instead of yet-another-list.
Cheers,

Rob.
Comment 11 Darin Adler 2006-06-03 17:36:46 PDT
Comment on attachment 8669 [details]
Improved patch

+    NSPoint hotSpotPoint = {hotspot.x(), hotspot.y()};
+    return [[NSCursor alloc] initWithImage:img hotSpot:hotSpotPoint];

You don't need the local variable. IntPoint objects already know how to convert themselves into NSPoint objects if you pass them to an expression that expects an NSPoint.

+    void addCursor(PassRefPtr<CSSCursorImageValue> v);

This would read better without the "v" -- no need to name the parameter if you're not using it here and if the name isn't helpful for documentation.

+    if (style && style->cursors()) {
+        Vector<RefPtr<CSSCursorImageValue> > cursors = style->cursors();
+        for (unsigned i = 0;i < cursors.size();++i) {
+            CachedImage* cimage = cursors[i].get()->image(node->document()->docLoader());
+            if (!cimage)
+                 continue;
+            if (cimage->image()->isNull())
+                break;
+            else if (!cimage->isErrorImage())
+                return Cursor(cimage->image(), cursors[i].get()->hotspot());
+        };
+    }

There's a spurious semicolon here.

Also, formatting trouble with an extra space before "continue" and missing spaces after the semicolons in the if statement.

There's no need for an else after  the break.

I don't understand why a null CachedImage means continue, but an image that's "isNull" means break, and an image that's an error image means continue. That's confusing.

cursors[i].get()->image can just be cursors[i]->image -- you only need the get() if you're passing to something that takes a raw pointer. You don't need it just to use the pointer. Same for cursors[i].get()->hotspot().

Why use the term "info" in cursorInfo. Maybe cursors is the best name for the property.

Could change cursors so it returns a const reference to the Vector. That way you won't have to copy the vector just to access the cursors.

Do we really need a reference-counted class for the cursors? I think it might be better to have the vector hold a struct with a RefPtr<CSSImageValue> plus an IntPoint for the hot spot. That struct would not need to be a while class and have a source file to itself. What do you think?

+            for (int i = 0; i < len; i++)
+            {

Braces should go on the same line as the for statement.

+                if (!item->isPrimitiveValue()) continue;

If statements should not be packed into one line like this.

-    case CSS_PROP_RESIZE: // none | both | horizontal | vertical | inherit
-        if (id == CSS_VAL_NONE || id == CSS_VAL_BOTH || id == CSS_VAL_HORIZONTAL || id == CSS_VAL_VERTICAL)
-            valid_primitive = true;
-        break;

Please don't roll out our new resize property!

+            for (unsigned i = 0;i < cursors.size();++i) {
+                list->append(new CSSPrimitiveValue(cursors[i].get()->image(node->document()->docLoader())->url(), CSSPrimitiveValue::CSS_URI));
+            }

We usually don't put braces around single line for statements either (like single-line if statements).

No test cases attached to the patch.

Looking good!
Comment 12 Alexey Proskuryakov 2006-07-25 05:19:57 PDT
Created attachment 9670 [details]
test case for HTML (put in WebCore/manual-tests)

The tests won't work in IE or Firefox, because they don't support .tiff cursors (and WebKit has major problems with .cur ones). I've tested WinIE with a version that used .cur.

To close this bug (as opposed to bug 6001), one would also need something similar for SVG. The existing SVG test case doesn't demonstrate the original issue, which is about <cursor>, rather than links.
Comment 13 Alexey Proskuryakov 2006-07-26 03:49:04 PDT
Created attachment 9696 [details]
test case for SVG

Tested with Adove SVG Viewer 6 beta (the first row doesn't work for whatever reason, but it should).

See also: <http://pilat.free.fr/asv6/cursor.svg>.
Comment 14 Eric Seidel (no email) 2006-09-07 02:05:59 PDT
Created attachment 10431 [details]
Another incomplete attempt
Comment 15 Eric Seidel (no email) 2006-09-07 02:08:31 PDT
(In reply to comment #14)
> Created an attachment (id=10431) [edit]
> Another incomplete attempt
> 

The code I just posted is missing proper list-based parsing, does not properly clean-up CursorData pointers when deleting the RenderStyle, does not add parsing for CSS3 cursor hotspots (even though CursorData now supports them), finally it does not expose the list data via the CSSComputedStyleDeclaration methods.
Comment 16 Rob Buis 2006-09-08 14:54:44 PDT
Created attachment 10468 [details]
Improved patch

This is my "old" patch, but compiling against ToT and addressing most of Darin's (style) issues.
It also includes aps test from comment 12. The patch seems to behave well, but maybe could pick
up some things from Erics patch and more testing against aps testcases. Nevertheless, I think it
may be worthwhile to review once again, and see what should be fixed, and how much of cursor
support we need to land.
Cheers,

Rob.
Comment 17 Eric Seidel (no email) 2006-09-08 15:48:24 PDT
Comment on attachment 10468 [details]
Improved patch

A few things here.

I recently re-read the spec, and I realize now that the grammar requires one to always specify a CSS cursor type, but that one can also optionally specify a list of URIs before that cursor type.  One may not specify a list of cursor types (which is unfortunate).

This however, leads itself to a nice design on our part.  RenderStyle::cursor() can always hold the last CSS cursor type, and there can be a separate fallback list (RenderStyle::cursors() in your patch) which is preferred over that cursor type if available.

1. We can't to SVG <cursor> element lookup at parse time.  That breaks an js insertion/modification/removal of <cursor> elements.

2. Might as well fix CSS3 hotspots while we're here... right now the code goes out of its way to not support them. ;)

3. This should probably only be allowed during quirks mode:
+            if (!value) { // no value after url list (MSIE 5 compatibility)

4.  This needs to use a setter:
+            style->cursors() = parentStyle->cursors();

5.  I'm not sure storing a Vector of CSSCursorImageValues is a good idea.  RenderStyles are really tight on space, and I think that Vector is at least 8 bytes in size (which is at least 4 bytes bigger than we need to be using here).  See my patch for how I used a pointer to a singly-linked list instead.

I'm not sure it's a good idea to only do the image load at runtime.
+            CachedImage* cimage = cursors[i]->image(node->document()->docLoader());

This can be a const IntPoint&, although that's not critical:
+static NSCursor* createCustomCursor(Image* image, IntPoint hotspot)

So this is going to need at least one more round.
Comment 18 Rob Buis 2006-09-10 00:18:59 PDT
Created attachment 10483 [details]
Another go

This patch is a "merge" between Eric's and my old patch. It should
address most of the current issues.
Cheers,

Rob.
Comment 19 Eric Seidel (no email) 2006-09-10 01:45:50 PDT
Comment on attachment 10483 [details]
Another go

Storing a Vector on RenderStyle is not a good idea.  That uses at least 4 more bytes than it needs to.  Storing a pointer to a vector would be OK, or using a list, both would work (personally I'm partial to the list).

Creating a CSSValueList unconditionally, and then deleting it if not needed is wasteful.

+        CSSValueList* list = new CSSValueList;

A better method is to have an if in your while statement which checks
if (!list)
    list = new CSSValueList;

Copy-paste error, you set the X value of the hotspot twice:
+                    hotspot.setX(value->fValue);

Also, you don't properly error out when it's
cursor: url("foo.png") 1, auto;
a single number should be an error.

I still would argue that a missing last value should only be allowed in quirks mode:

+            if (!value) { // no value after url list (MSIE 5 compatibility)

+                        style->addCursor(primitiveValue->getStringValue().deprecatedString().mid(1));

This doesn't need to use a deprecatedString.  .substring(1) will do the same as .mid(1)

Otherwise it looks fine.
Need one more round on this.
Comment 20 Rob Buis 2006-09-10 11:00:27 PDT
Created attachment 10487 [details]
Another go

Again, another attempt to address the issues in the last review.
Cheers,

Rob.
Comment 21 Darin Adler 2006-09-10 12:58:28 PDT
Comment on attachment 10487 [details]
Another go

+            if (uri.find("#") == 0) {

This is an inefficient way to figure out if the first character of a string is a "#" that needlessly searches the entire string for "#" characters when there are none. An efficient way would be to use uri[0] == '#' or uri.startsWith("#").

+        if (!strict && value->id == CSS_VAL_HAND) { // MSIE 5 compatibility :/
+            id = CSS_VAL_POINTER;
+            valid_primitive = true;
+        } else if (value->id >= CSS_VAL_AUTO && value->id <= CSS_VAL_HELP)

How does the CSS_VAL_AUTO case here work? I don't see it setting id, and it looks to me like code below in the if (valid_primitive) block isn't going to work if id is not set.

I would fine the code easier to read without this else:

+        else
+            delete list;
+        if (!strict && value->id == CSS_VAL_HAND) { // MSIE 5 compatibility :/

Both the "delete list" and the "if (!strict" line run only if the if above returns false, but it's hard to figure that out because of the else.

The RenderPath.cpp seems unrelated to cursors, so should be landed separately.

+                Element* e = node->document()->getElementById((*cursors)[i].cursorFragmentId.deprecatedString());

What's the .deprecatedString() for here? I think that will just cause it to copy the string an extra time or two!

Looks like there's a logic error. If SVG_SUPPORT is on and the tag does not have the name "cursorTag", then cimage can be null, and we'll crash on the:

+            if (cimage->image()->isNull())

Why "WebCore::" in WebCore::SVGNames::cursorTag? The usual idiom is to put "using namespace SVGNames" at the top of the file and then just say "cursorTag", but in any case there's no reason to say WebCore:: here.

Maybe we could overload the Cursor constructor instead of adding an optional parameter. That's more efficient when not passing the parameter, and eliminates the need to include the IntPoint.h header in Cursor.h. Or maybe the hotSpot parameter should not be optional -- I think that would be good, and in that case there's no need to include IntPoint.h either (a forward declaration will suffice).

+    RefPtr<CursorList> cursor_data;

I suggest cursorData instead.

+    Vector<CursorData> m_impl;

How about making that member private?

+    const CursorData &operator[](int i) const {
+        return m_impl[i];
+    }
+    const CursorData &operator[](int i) {
+        return m_impl[i];
+    }

If both overloads return a "const CursorData&" then we don't need overloading (can just keep the const version). Also, the "&" should be next to "CursorData", not "operator". I think we could call it m_vector instead of m_impl.
Comment 22 Rob Buis 2006-09-10 13:45:47 PDT
Hi Darin,

(In reply to comment #21)
> +        if (!strict && value->id == CSS_VAL_HAND) { // MSIE 5 compatibility :/
> +            id = CSS_VAL_POINTER;
> +            valid_primitive = true;
> +        } else if (value->id >= CSS_VAL_AUTO && value->id <= CSS_VAL_HELP)
> 
> How does the CSS_VAL_AUTO case here work? I don't see it setting id, and it
> looks to me like code below in the if (valid_primitive) block isn't going to
> work if id is not set.

The counter ids are only added at the end of the list, and I think the parsedValue
is the block that should work (for the list) instead of valid_primitive block. Correct
me if you think that is wong, but that is how I remember from the original patch.
Cheers,

Rob.
Comment 23 Rob Buis 2006-09-10 13:58:18 PDT
Created attachment 10491 [details]
Another attempt

This should address most of Darins issues.
Cheers,

Rob.
Comment 24 Darin Adler 2006-09-10 19:50:18 PDT
(In reply to comment #22)
> (In reply to comment #21)
> > +        if (!strict && value->id == CSS_VAL_HAND) { // MSIE 5 compatibility :/
> > +            id = CSS_VAL_POINTER;
> > +            valid_primitive = true;
> > +        } else if (value->id >= CSS_VAL_AUTO && value->id <= CSS_VAL_HELP)
> > 
> > How does the CSS_VAL_AUTO case here work? I don't see it setting id, and it
> > looks to me like code below in the if (valid_primitive) block isn't going to
> > work if id is not set.
> 
> The counter ids are only added at the end of the list, and I think the
> parsedValue is the block that should work (for the list) instead of
> valid_primitive block. Correct me if you think that is wong, but that
> is how I remember from the original patch.

I do think that's wrong. Look carefully at the part of the patch I'm quoting. If we ever hit this code, then there is no list. Either that code is wrong and unreachable (and should be deleted), or it's wrote and reachable (so we need a test case), or I'm mistaken. But I think I'm reading this right.
Comment 25 Darin Adler 2006-09-10 19:56:35 PDT
Comment on attachment 10491 [details]
Another attempt

Some nitpicks:

+            for (unsigned i = 0;i < cursors->size();++i)

Missing spaces after the semicolons.

+        if (list) {
+            list->append(value);
+            return list;
+        }
+        ASSERT(value);

The ASSERT(value) should go before the if.

+                    if (primitiveValue->getStringValue().find("#") == 0)

You fixed one of these, but not the other!

+        Cursor(Image*, IntPoint hotspot);

Should be const IntPoint& instead of IntPoint, and in fact some of the implementations, like the CursorQt.cpp ones, are like that.

+        inherited.access()->cursorData = new CursorList();

It's slightly nicer to just do "new CursorList" in a case like this, I think.

+class CursorList : public Shared<CursorList>
+{

Brace should be on the same line as the class declaration.

+    void setCursors(CursorList*);

This should take a PassRefPtr<CursorList> since it takes ownership of the passed-in object.

+    CachedImage* cursorImage; // weak pointer, the CSSValueImage takes care of deleting cursorImage

I'm a little concerned about this. What's the guarantee that the RenderStyle won't outlast the CSSValueImage? Is there any other RenderStyle code that uses this ownership model?

I'm going to say review- because of the find() and because of my previous comment about what I called the "CSS_VAL_AUTO case".
Comment 26 Rob Buis 2006-09-11 05:17:34 PDT
Hi Darin,

> I do think that's wrong. Look carefully at the part of the patch I'm quoting.
> If we ever hit this code, then there is no list. Either that code is wrong and
> unreachable (and should be deleted), or it's wrote and reachable (so we need a
> test case), or I'm mistaken. But I think I'm reading this right.

I think I see what you mean. I'll post a new patch, with hopefully cleaner
structure and fixing this issue.
Cheers,

Rob. 
Comment 27 Rob Buis 2006-09-11 05:20:53 PDT
(In reply to comment #25)
Hi Darin,

> I'm a little concerned about this. What's the guarantee that the RenderStyle
> won't outlast the CSSValueImage? Is there any other RenderStyle code that uses
> this ownership model?

I think setListStyleImage does the same.
Cheers,

Rob.
Comment 28 Rob Buis 2006-09-11 05:22:11 PDT
Created attachment 10499 [details]
Even better patch

This one should hopefully address Darin's issues.
Cheers,

Rob.
Comment 29 Eric Seidel (no email) 2006-09-13 15:52:05 PDT
Comment on attachment 10499 [details]
Even better patch

+                    hotSpot.setX(int(static_cast<SVGCursorElement*>(e)->xBaseValue()->value()));
+                    hotSpot.setY(int(static_cast<SVGCursorElement*>(e)->yBaseValue()->value()));

BaseValue is wrong here.  I we should probably make those accessors private, and only expose them to the classes that need them.

x() and y() is what you want.
Comment 30 Rob Buis 2006-09-15 04:42:17 PDT
Created attachment 10574 [details]
Another attempt

This patch should address Eric's issue with xBaseVal.
Cheers,

Rob.
Comment 31 Eric Seidel (no email) 2006-09-15 15:10:58 PDT
Comment on attachment 10574 [details]
Another attempt

I'm still not a big fan of needing a separate class "CursorList" to hold all the nodes, when the individual CursorData nodes could just form a singly linked list and know how to delete their "next" pointer when they are themselves deleted.

This would also remove the need for RenderStyle::addCursor...


+using namespace SVGNames;

needs to be inside a SVG_SUPPORT block in FrameView.cpp

Is there any need to check the type here?
+            value = valueList->next(); // comma

We'll need some more tests.

It's not clear to me how the CursorList is ever destroyed.

It's also not clear to me that two divs, one inside the other, both which use independent cursor fallback will properly function (i.e. when is the cursors() list cleared?)

I don't think we can land this without at least answering the leak question.
Comment 32 Eric Seidel (no email) 2006-09-19 00:35:33 PDT
testing (bugzilla is a bit wacky right now)
Comment 33 Rob Buis 2006-09-19 00:36:43 PDT
Yet another test

(In reply to comment #31)
> (From update of attachment 10574 [details] [edit])
> I'm still not a big fan of needing a separate class "CursorList" to hold all
> the nodes, when the individual CursorData nodes could just form a singly linked
> list and know how to delete their "next" pointer when they are themselves
> deleted.
> 
> This would also remove the need for RenderStyle::addCursor...
> 
> 
> +using namespace SVGNames;
> 
> needs to be inside a SVG_SUPPORT block in FrameView.cpp
> 
> Is there any need to check the type here?
> +            value = valueList->next(); // comma
> 
> We'll need some more tests.
> 
> It's not clear to me how the CursorList is ever destroyed.
> 
> It's also not clear to me that two divs, one inside the other, both which use
> independent cursor fallback will properly function (i.e. when is the cursors()
> list cleared?)
> 
> I don't think we can land this without at least answering the leak question.
> 
Comment 34 Rob Buis 2006-09-22 02:48:06 PDT
Created attachment 10701 [details]
css parser fixes

I added some tests for incorrect css3 cursor syntax and css parser code to handle these errors.
Cheers,

Rob.
Comment 35 Eric Seidel (no email) 2006-09-22 03:03:52 PDT
Comment on attachment 10701 [details]
css parser fixes

I don't think we want this:
+#if SVG_SUPPORT
+            if (uri.startsWith("#")) {
+                if (!list)
+                    list = new CSSValueList; 
+                list->append(new CSSPrimitiveValue(uri, CSSPrimitiveValue::CSS_URI));
+                value = valueList->next();
+            } else
+#endif

That treats url(#foo) differently from url(foo.png).  The latter allows CSS3 hotspots, the former does not.  I'm not sure which should win out (the <cursor> specified hotspot or the css3  one, but I don't think it should be an error.)

I'm surprised this is only an error in strict mode:
+                if (strict && value && !(value->unit == Value::Operator && value->iValue == ','))
+                    return false;

If it's only going to be an error in strict mode, maybe it only needs to be required in strict mode?  Or maybe in non-strict mode an ; is also allowed?

I think this function should have a different name to denote its different behavior:
style->addCursor(primitiveValue->getStringValue().substring(1))
perhaps addSVGCursor ?

You might also want to call the parameter fragmentId:
+void RenderStyle::addCursor(const String& id)

Great patch.  This has been an awfully long journey to get this landed, but I think you're basically there.  I don't need to see the code again.   But if you could make those tweaks before landing that would be great.  I still need to check the test case.

r=me.
Comment 36 Eric Seidel (no email) 2006-09-22 03:11:07 PDT
(In reply to comment #35)
> I'm surprised this is only an error in strict mode:
> +                if (strict && value && !(value->unit == Value::Operator &&
> value->iValue == ','))
> +                    return false;
> 
> If it's only going to be an error in strict mode, maybe it only needs to be
> required in strict mode?  Or maybe in non-strict mode an ; is also allowed?

What I meant there, was that perhaps things should be "more strict" in non-strict mode.  Only a few non-comma values are allowed in non-strict mode, yes?

Also, I'm wondering if in strict mode .myclass { cursor: url(foo.png) } would crash?  Or at least it would end up calling valueList->next() a few more times than you meant it too, or?
Comment 37 Rob Buis 2006-09-22 03:16:57 PDT
Created attachment 10702 [details]
Improved testcase
Comment 38 Eric Seidel (no email) 2006-09-22 03:18:28 PDT
Comment on attachment 10701 [details]
css parser fixes

After discussing this with rob more over irc, this needs one more round.  At least to fix inheritance if nothing else.
Comment 39 Eric Seidel (no email) 2006-09-22 03:40:46 PDT
Comment on attachment 10702 [details]
Improved testcase

Rob,
So it seems after discussing this with you over IRC, we'll need at least 3 test cases:
1.  a quirks mode test
2.  a strict mode test
3.  an svg test

The latter two could probably be rolled into a single CDF document (xhtml wrapping inline svg) if you'd like.
Comment 40 Rob Buis 2006-09-23 13:55:04 PDT
Created attachment 10726 [details]
Improved patch+testcases

This patch should address most of Erics issues. It adds a new test for cursors in svg + strict mode, using cdf, called cursorfallback.xml. I am not sure a quirks test is needed, since it should behave the same as
strict according to ap and myself, so it would be a copy of css3-cursor-test2, just without the doctype. Let me know whether the tests should be improved.
Cheers,

Rob.
Comment 41 Eric Seidel (no email) 2006-09-24 02:18:37 PDT
Comment on attachment 10726 [details]
Improved patch+testcases

I'm still don't think this is needed:
+#if SVG_SUPPORT
+            if (uri.startsWith("#")) {
+                if (!list)
+                    list = new CSSValueList; 
+                list->append(new CSSPrimitiveValue(uri, CSSPrimitiveValue::CSS_URI));
+            } else
+#endif
All you're doing is intentionally disabling hot-spot parsing for SVGs.  I don't think that's right considering CSS3.

Other than that, things look fine.  We'll talk over irc.
Comment 42 Eric Seidel (no email) 2006-09-24 02:29:07 PDT
(In reply to comment #41)
> (From update of attachment 10726 [details] [edit])
> I'm still don't think this is needed:
...

My mistake.  disregard.
Comment 43 Rob Buis 2006-09-24 07:01:44 PDT
Created attachment 10738 [details]
Improved testcases

This patch has testcases splitted out for quirks and strict mode. New testcases added
for different inheritance scenarios, and out-of-range hotspots.
Cheers,

Rob.
Comment 44 Eric Seidel (no email) 2006-09-24 07:24:20 PDT
Comment on attachment 10738 [details]
Improved testcases

This looks great.

We discussed a spelling error, and testing in IE before landing over IRC.  But this looks good to go.

r=me