Closed
Bug 414550
Opened 16 years ago
Closed 16 years ago
style="property:currentColor" broken since style attribute reparsed after incorrect serialization
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
People
(Reporter: petr.pisar, Assigned: dbaron)
References
()
Details
Attachments
(4 files, 1 obsolete file)
594 bytes,
image/svg+xml
|
Details | |
407 bytes,
image/png
|
Details | |
2.97 KB,
patch
|
Details | Diff | Splinter Review | |
13.16 KB,
patch
|
bzbarsky
:
review+
bzbarsky
:
superreview+
mtschrep
:
approval1.9+
|
Details | Diff | Splinter Review |
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).
Reporter | ||
Comment 1•16 years ago
|
||
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.
Reporter | ||
Comment 2•16 years ago
|
||
This picture shows how the testcase should look.
Updated•16 years ago
|
Component: General → Style System (CSS)
Product: Firefox → Core
QA Contact: general → style-system
Comment 3•16 years ago
|
||
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.
Assignee | ||
Comment 4•16 years ago
|
||
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•16 years ago
|
||
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
Assignee | ||
Comment 6•16 years ago
|
||
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> ?
Reporter | ||
Comment 7•16 years ago
|
||
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.
Assignee | ||
Comment 8•16 years ago
|
||
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
Assignee | ||
Updated•16 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: style="property:currentColor" doesn't work → style="property:currentColor" broken since style attribute reparsed after incorrect serialization
Assignee | ||
Comment 9•16 years ago
|
||
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•16 years ago
|
||
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•16 years ago
|
||
Comment on attachment 300459 [details] [diff] [review] fix serialization bug (quick fix; untested) This does fix the testcase.
Comment 12•16 years ago
|
||
test 2 fails without the patch and passes with it. The other tests pass either way.
Reporter | ||
Comment 13•16 years ago
|
||
(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•16 years ago
|
||
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 | ||
Updated•16 years ago
|
Assignee: nobody → dbaron
OS: Linux → All
Hardware: PC → All
Assignee | ||
Comment 15•16 years ago
|
||
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)
Assignee | ||
Comment 16•16 years ago
|
||
Note that I searched the tree for all occurrences of eCSSUnit_Integer when writing this patch.
![]() |
||
Comment 17•16 years ago
|
||
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+
Assignee | ||
Comment 18•16 years ago
|
||
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?
Updated•16 years ago
|
Attachment #300597 -
Flags: approval1.9? → approval1.9+
Assignee | ||
Comment 19•16 years ago
|
||
Checked in to trunk, 2008-02-08 11:51 -0800. Filed bug 416437 about the double parsing.
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Comment 20•16 years ago
|
||
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
Flags: in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•