Bug 6002 – WebKit does not properly handle SVG <cursor> element

• Component: SVG
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

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.

[jay]

Created attachment 8206 [details]
cursor should be "hand" on circle 

mouseover circle which has anchor, cursor pointer should be "hand"

[jay]

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...

[Rob Buis]

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.

[Alexey Proskuryakov]

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.

[Rob Buis]

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.

[Eric Seidel (no email)]

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.

[Rob Buis]

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.

[Alexey Proskuryakov]

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.

[Rob Buis]

Created attachment 8669 [details]
Improved patch

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

Rob.

[Darin Adler]

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!

[Alexey Proskuryakov]

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.

[Alexey Proskuryakov]

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>.

[Eric Seidel (no email)]

Created attachment 10431 [details]
Another incomplete attempt

[Eric Seidel (no email)]

(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.

[Rob Buis]

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.

[Eric Seidel (no email)]

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.

[Rob Buis]

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.

[Eric Seidel (no email)]

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.

[Rob Buis]

Created attachment 10487 [details]
Another go

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

Rob.

[Darin Adler]

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.

[Rob Buis]

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.

[Rob Buis]

Created attachment 10491 [details]
Another attempt

This should address most of Darins issues.
Cheers,

Rob.

[Darin Adler]

(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.

[Darin Adler]

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".

[Rob Buis]

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. 

[Rob Buis]

(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.

[Rob Buis]

Created attachment 10499 [details]
Even better patch

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

Rob.

[Eric Seidel (no email)]

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.

[Rob Buis]

Created attachment 10574 [details]
Another attempt

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

Rob.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

testing (bugzilla is a bit wacky right now)

[Rob Buis]

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.
> 

[Rob Buis]

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.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

(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?

[Rob Buis]

Created attachment 10702 [details]
Improved testcase

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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.

[Rob Buis]

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.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

(In reply to comment #41)
> (From update of attachment 10726 [details] [edit])
> I'm still don't think this is needed:
...

My mistake.  disregard.

[Rob Buis]

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.

[Eric Seidel (no email)]

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