414550 - style="property:currentColor" broken since style attribute reparsed after incorrect serialization

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[petr.pisar]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b3pre) Gecko/2008012704 Minefield/3.0b3pre
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b3pre) Gecko/2008012704 Minefield/3.0b3pre

Setting painting property to currentColor via @style attribute doesn't work in SVG (e.g. <svg:rect style="stroke:currentColor"/>).

Reproducible: Always

Steps to Reproduce:
1.Render SVG that uses currentColor in style attribute

Actual Results:  
fill:currentColor effects into black and stroke:currentColor into invisible.

Expected Results:  
The property should inherit color as defined in parent's color property.

OTOH, using currentColor directly via @fill or @stroke attributes is Ok (fixed bug #216559).

Comment 1

[petr.pisar]

Attachment: Testcase showing 3 rectangles

Upper rectangle is filled blue because @fill="currentColor" works. (Included for reference and regresion check.)
Middle rectangle is filled blask, it should be filled blue. @style="fill:currentColor" doesn't work.
Lower rectangle is invisible, it should be stroked blue. style="stroke:currentColor" doesn't work.

Comment 2

[petr.pisar]

Attachment: Comparison of correct rendering

This picture shows how the testcase should look.

Comment 3

[Robert Longson [:longsonr]]

When the parser gets to the style attributes it reads -4 from the scanner which is NS_COLOR_CURRENTCOLOR. This is not an ident, though so the parser rejects it.

Comment 4

[David Baron :dbaron:]

Where does the parser reject it?  CSSParserImpl::ParsePaint looks like it doesn't otherwise postprocess the results of the ParseColor call within ParseVariant, and SetSVGPaint in nsRuleNode.cpp should handle it by calling SetColor.

Comment 5

[Robert Longson [:longsonr]]

If currentColor is an attribute value then CSSParserImpl::ParseVariant reads "currentColor" as a token. 
This token is determined to be an ident and the test to see whether we should parse it using ParseColor succeeds.

If currentColor comes from a style then CSSParserImpl::ParseVariant reads -4 as a token. This token is determined to be a number. The token is not parsed with parseColor and we reject it as being invalid.

I guess the -4 is somehow originating from CSSParserImpl::ParseColor which calls aValue.SetIntValue for currentColor

Comment 6

[David Baron :dbaron:]

Is the problem that something in the SVG code is parsing, serializing incorrectly, and then reparsing?

Which of the following is this bug saying don't work:
  fill="currentColor"
  style="fill:currentColor"
  <style> rect { fill: currentColor; } </style>
?

Comment 7

[petr.pisar]

Works:
  fill="currentColor"
Don't work:
  style="fill:currentColor"
  <style> rect { fill: currentColor; } </style>

Interested is that only the attribute form 'style="fill:currentColor"' produces following message in error console:

Warning: Error in parsing value for property 'fill'.  Declaration dropped.

The last form, you've written but doesn't present in test case, doesn't produce the message.

Comment 8

[David Baron :dbaron:]

Sure enough, it seems like we parse every SVG style attribute twice.  The first time we get the correct string, and the second time we get an incorrectly serialized one:

Breakpoint 1, CSSParserImpl::ParseStyleAttribute (this=0x86de8a0, 
    aAttributeValue=@0xbf9c4f70, aDocURL=0x8a0c7b0, aBaseURL=0x8a0c7b0, 
    aNodePrincipal=0x87a6068, aResult=0xbf9c4cf4)
    at /builds/trunk/mozilla/layout/style/nsCSSParser.cpp:821
821       NS_PRECONDITION(aNodePrincipal, "Must have principal here!");
(gdb) bt 7
#0  CSSParserImpl::ParseStyleAttribute (this=0x86de8a0, 
    aAttributeValue=@0xbf9c4f70, aDocURL=0x8a0c7b0, aBaseURL=0x8a0c7b0, 
    aNodePrincipal=0x87a6068, aResult=0xbf9c4cf4)
    at /builds/trunk/mozilla/layout/style/nsCSSParser.cpp:821
#1  0x0147285f in nsStyledElement::ParseStyleAttribute (aContent=0x8b4edb0, 
    aValue=@0xbf9c4f70, aResult=@0xbf9c4f28)
    at /builds/trunk/mozilla/content/base/src/nsStyledElement.cpp:260
#2  0x014730c8 in nsStyledElement::ParseAttribute (this=0x8b4edb0, 
    aNamespaceID=0, aAttribute=0x840ba04, aValue=@0xbf9c4f70, 
    aResult=@0xbf9c4f28)
    at /builds/trunk/mozilla/content/base/src/nsStyledElement.cpp:89
#3  0x017fbe32 in nsSVGElement::ParseAttribute (this=0x8b4edb0, 
    aNamespaceID=0, aAttribute=0x840ba04, aValue=@0xbf9c4f70, 
    aResult=@0xbf9c4f28)
    at /builds/trunk/mozilla/content/svg/content/src/nsSVGElement.cpp:380
#4  0x0143d8e2 in nsGenericElement::SetAttr (this=0x8b4edb0, aNamespaceID=0, 
    aName=0x840ba04, aPrefix=0x0, aValue=@0xbf9c4f70, aNotify=0)
    at /builds/trunk/mozilla/content/base/src/nsGenericElement.cpp:3642
#5  0x01575d42 in nsXMLContentSink::AddAttributes (this=0x87b1a40, 
    aAtts=0x8d1de00, aContent=0x8b4edb0)
    at /builds/trunk/mozilla/content/xml/document/src/nsXMLContentSink.cpp:1478
#6  0x01577617 in nsXMLContentSink::HandleStartElement (this=0x87b1a40, 
    aName=0x8cedb98, aAtts=0x8d1dde0, aAttsCount=10, aIndex=-1, 
    aLineNumber=10, aInterruptable=1)
    at /builds/trunk/mozilla/content/xml/document/src/nsXMLContentSink.cpp:1034
(More stack frames follow...)
(gdb) p aAttributeValue
$10 = (
    const nsAString_internal &) @0xbf9c4f70: {<nsSubstring_base> = {<No data fields>}, mData = 0x8b40698, mLength = 17, mFlags = 1}
(gdb) x/17hc $.mData
0x8b40698:      102 'f' 105 'i' 108 'l' 108 'l' 58 ':'  99 'c'  117 'u' 114 'r'
0x8b406a8:      114 'r' 101 'e' 110 'n' 116 't' 67 'C'  111 'o' 108 'l' 111 'o'
0x8b406b8:      114 'r'
(gdb) c
Continuing.

Breakpoint 1, CSSParserImpl::ParseStyleAttribute (this=0x86de8a0, 
    aAttributeValue=@0xbf9c4df4, aDocURL=0x8a0c7b0, aBaseURL=0x8a0c7b0, 
    aNodePrincipal=0x87a6068, aResult=0xbf9c4da4)
    at /builds/trunk/mozilla/layout/style/nsCSSParser.cpp:821
821       NS_PRECONDITION(aNodePrincipal, "Must have principal here!");
(gdb) bt 7
#0  CSSParserImpl::ParseStyleAttribute (this=0x86de8a0, 
    aAttributeValue=@0xbf9c4df4, aDocURL=0x8a0c7b0, aBaseURL=0x8a0c7b0, 
    aNodePrincipal=0x87a6068, aResult=0xbf9c4da4)
    at /builds/trunk/mozilla/layout/style/nsCSSParser.cpp:821
#1  0x0147285f in nsStyledElement::ParseStyleAttribute (aContent=0x8b4edb0, 
    aValue=@0xbf9c4df4, aResult=@0xbf9c4e88)
    at /builds/trunk/mozilla/content/base/src/nsStyledElement.cpp:260
#2  0x017fc40f in nsSVGElement::BindToTree (this=0x8b4edb0, 
    aDocument=0x8c2fc40, aParent=0x84f9fb0, aBindingParent=0x0, 
    aCompileEventHandlers=1)
    at /builds/trunk/mozilla/content/svg/content/src/nsSVGElement.cpp:200
#3  0x01441168 in nsGenericElement::doInsertChildAt (aKid=0x8b4edb0, aIndex=3, 
    aNotify=0, aParent=0x84f9fb0, aDocument=0x8c2fc40, aChildArray=@0x84f9fc8)
    at /builds/trunk/mozilla/content/base/src/nsGenericElement.cpp:2710
#4  0x01441441 in nsGenericElement::InsertChildAt (this=0x84f9fb0, 
    aKid=0x8b4edb0, aIndex=3, aNotify=0)
    at /builds/trunk/mozilla/content/base/src/nsGenericElement.cpp:2662
#5  0x01162691 in nsINode::AppendChildTo (this=0x84f9fb0, aKid=0x8b4edb0, 
    aNotify=0) at ../../../dist/include/content/nsINode.h:296
#6  0x01577783 in nsXMLContentSink::HandleStartElement (this=0x87b1a40, 
    aName=0x8cedb98, aAtts=0x8d1dde0, aAttsCount=5, aIndex=-1, aLineNumber=10, 
    aInterruptable=1)
    at /builds/trunk/mozilla/content/xml/document/src/nsXMLContentSink.cpp:1041
(More stack frames follow...)
(gdb) p aAttributeValue
$11 = (
    const nsAString_internal &) @0xbf9c4df4: {<nsSubstring_base> = {<No data fields>}, mData = 0xbf9c4e08, mLength = 9, mFlags = 65553}
(gdb) x/9hc $.mData
0xbf9c4e08:     102 'f' 105 'i' 108 'l' 108 'l' 58 ':'  32 ' '  45 '-'  52 '4'
0xbf9c4e18:     59 ';'


So there are two bugs here:
 (1) we parse twice
 (2) we serialize wrong

Comment 9

[David Baron :dbaron:]

Attachment: fix serialization bug (quick fix; untested)

My guess is that this will fix the serialization bug; I haven't had a chance to test it yet.

I think we need another unit type here, though.

Comment 10

[Boris Zbarsky [:bzbarsky]]

The reason we parse twice is because of the checkin for bug 387466: we parse (eagerly) when we insert the attr and again when we insert the node into the tree.  Perhaps we should parse lazily or something, if this is a serious issue.

Comment 11

[Robert Longson [:longsonr]]

Comment on attachment 300459 [details] [diff] [review]
fix serialization bug (quick fix; untested)

This does fix the testcase.

Comment 12

[Robert Longson [:longsonr]]

Attachment: reftests

test 2 fails without the patch and passes with it. The other tests pass either way.

Comment 13

[petr.pisar]

(In reply to comment #12 and comment #7)
> test 2 fails without the patch and passes with it. The other tests pass either
> way.
> 
Your test 3 and test 3 from comment #6 differ. Mine is missing required @type="text/css". FF nightbuild from 2008-01-28 ignores such style sheet, other implementations (e.g. librsvg) accept it. Not sure which practice is better. I think I like FF behaviour more.

Comment 14

[Robert Longson [:longsonr]]

Although you can omit the type attribute from the style element in html, you cannot do so in SVG per http://www.w3.org/TR/SVG/styling.html#StyleElement 

I suggest you raise a bugs on those other implementations.

Comment 15

[David Baron :dbaron:]

Attachment: fix serialization bug

This is, I think, a better fix for the serialization bug, since it doesn't require adding a property to the list every time we add a property that takes color values.

I confirmed that a bunch of the serialization tests in test_value_storage.html fail with the mochitest changes here (in today's nightly, which doesn't have my patch).  They pass with my patch, of course.

Comment 16

[David Baron :dbaron:]

Note that I searched the tree for all occurrences of eCSSUnit_Integer when writing this patch.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 300597 [details] [diff] [review]
fix serialization bug

Looks good.

Comment 18

[David Baron :dbaron:]

Comment on attachment 300597 [details] [diff] [review]
fix serialization bug

This is a relatively simple fix for a serialization bug (in reasonably well-tested code) that currently affects SVG pretty badly, since it breaks the use of currentColor (introduced for/by SVG) on a number of properties.

Comment 19

[David Baron :dbaron:]

Checked in to trunk, 2008-02-08 11:51 -0800.

Filed bug 416437 about the double parsing.

Comment 20

[Robert Longson [:longsonr]]

Checked in reftests:
http://lxr.mozilla.org/seamonkey/source/layout/reftests/svg/currentColor-01.svg
http://lxr.mozilla.org/seamonkey/source/layout/reftests/svg/currentColor-02.svg
http://lxr.mozilla.org/seamonkey/source/layout/reftests/svg/currentColor-03.svg