Closed Bug 414550 Opened 13 years ago Closed 13 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: petr.pisar, Assigned: dbaron)

References

()

Details

Attachments

(4 files, 1 obsolete file)

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).
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.
This picture shows how the testcase should look.
Component: General → Style System (CSS)
Product: Firefox → Core
QA Contact: general → style-system
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.
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.
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
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>
?
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.
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
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: style="property:currentColor" doesn't work → style="property:currentColor" broken since style attribute reparsed after incorrect serialization
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.
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 on attachment 300459 [details] [diff] [review]
fix serialization bug (quick fix; untested)

This does fix the testcase.
Attached patch reftestsSplinter Review
test 2 fails without the patch and passes with it. The other tests pass either way.
(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.
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.
Assignee: nobody → dbaron
OS: Linux → All
Hardware: PC → All
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.
Attachment #300459 - Attachment is obsolete: true
Attachment #300597 - Flags: superreview?(bzbarsky)
Attachment #300597 - Flags: review?(bzbarsky)
Note that I searched the tree for all occurrences of eCSSUnit_Integer when writing this patch.
Comment on attachment 300597 [details] [diff] [review]
fix serialization bug

Looks good.
Attachment #300597 - Flags: superreview?(bzbarsky)
Attachment #300597 - Flags: superreview+
Attachment #300597 - Flags: review?(bzbarsky)
Attachment #300597 - Flags: review+
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.
Attachment #300597 - Flags: approval1.9?
Attachment #300597 - Flags: approval1.9? → approval1.9+
Checked in to trunk, 2008-02-08 11:51 -0800.

Filed bug 416437 about the double parsing.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Duplicate of this bug: 418997
Duplicate of this bug: 420898
You need to log in before you can comment on or make changes to this bug.