Closed
Bug 216559
Opened 21 years ago
Closed 20 years ago
fill="currentColor" does not work.
Categories
(Core :: SVG, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: darryl, Assigned: scootermorris)
References
()
Details
(Keywords: css3)
Attachments
(1 file, 3 obsolete files)
20.39 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.5b) Gecko/20030809 Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.5b) Gecko/20030809 fill="currentColor" should fill with the value of the current "color" attribute (see http://www.w3.org/TR/SVG11/color.html#ColorProperty ). It does not. "currentColor" spits out a parsing error and the "fill" property is ignored. Reproducible: Always Steps to Reproduce: Open w3c tiny test suite test "painting-fill-02-t.svg". Actual Results: Two black rectangles are rendered Expected Results: A green rectangle and a blue rectangle should be rendered. A chopped down version of the test would be: <?xml version="1.0" encoding="UTF-8"?> <!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1 Tiny//EN" "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11-tiny.dtd"> <svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" version="1.1" baseProfile="tiny" id="svg-root" width="100%" height="100%" viewBox="0 0 480 360"> <g id="test-body-content" color="green"> <rect id="fill-03" fill="currentColor" stroke="#000000" x="75" y="110" width="100" height="140"/> <rect id="fill-04" color="blue" fill="currentColor" stroke="#000000" x="275" y="110" width="100" height="140"/> </g> </svg>
Updated•20 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 1•20 years ago
|
||
This calor is actually part of the CSS3 color module as well so maybe it will makes sense to put this under CSS instead?
Keywords: css3
Comment 2•20 years ago
|
||
Yeah, looks to me like this should just be implemented throughout instead of being SVG specific.
QA Contact: ian
Updated•20 years ago
|
Assignee: alex → scootermorris
Assignee | ||
Updated•20 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•20 years ago
|
||
Assignee | ||
Updated•20 years ago
|
Attachment #163614 -
Flags: review?(tor)
Reading through this patch it seems that currentColor isn't live - if something modifies the color attribute, it won't get reflected in anything using currentColor.
Assignee | ||
Comment 5•20 years ago
|
||
(In reply to comment #4) > Reading through this patch it seems that currentColor isn't live - if > something modifies the color attribute, it won't get reflected in > anything using currentColor. No, it actually does modify it. Try this example: <svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" version="1.1" baseProfile="tiny" id="svg-root" width="100%" height="100%" viewBox="0 0 480 360"> <title id="test-title">painting-fill-02-t.svg</title> <desc id="test-desc"> Test that viewer has the basic capability to handle the <fill,fill-rule,fill-opacity properites> </desc> <g id="test-body-content" color="green"> <text font-family="Arial" font-size="36" x="30" y="42">Basic paint: fill properties.</text> <text font-family="Arial" font-size="36" x="100" y="80">fill="currentColor"</text> <rect id="fill-03" fill="currentColor" stroke="#000000" x="75" y="110" width="100" height="140" onclick="changeColor();"/> <rect id="fill-04" style="color:blue;" fill="currentColor" stroke="#000000" x="275" y="110" width="100" height="140"/> <text font-family="Arial" font-size="36" x="80" y="280">green</text> <text font-family="Arial" font-size="36" x="290" y="280">blue</text> </g> <text id="revision" x="10" y="340" font-size="40" stroke="none" fill="black">$Revision: 1.5 $</text> <rect id="test-frame" x="1" y="1" width="478" height="358" fill="none" stroke="#000000"/> <script> function changeColor() { gE = document.getElementById("test-body-content"); gE.setAttribute("color", "red"); } </script> </svg> When you click on the green rectangle, it changes color to red.
Comment on attachment 163614 [details] [diff] [review] Initial patch to implement currentColor for SVG If the style people are ok with doing this for SVG only at this point, it looks fine to me. You'll probably want to remove the debug printfs from the style system, but I'll let the sr have the final say on that.
Attachment #163614 -
Flags: review?(tor) → review+
Comment 7•20 years ago
|
||
'currentColor' is just a valid CSS <color> value, not SVG-specific. IMHO it should be implemented as such. But that's just me. :-)
Assignee | ||
Comment 8•20 years ago
|
||
(In reply to comment #7) > 'currentColor' is just a valid CSS <color> value, not SVG-specific. IMHO it > should be implemented as such. But that's just me. :-) I'm happy to do this, and it actually simplifies things, but I'm concerned that getting r/sr outside of SVG will be much more difficult since currentColor is not in common use outside of SVG (not a lot of CSS3 websites, yet). Could I get a reading from the CSS module owners (dbaron?) as to whether they would be interested in seeing a patch to add the currentColor keyword to attribute and style values that accept a color? I've looked at it and it would involve an additional keyword in kColorKTable and some handling code in SetColor in nsRuleNode, plus some special-casing for the "color" attribute itself.
I agree with comment 7. I haven't yet looked at the patch here, though.
Assignee | ||
Comment 10•20 years ago
|
||
OK, here is a patch that implements currentColor in accordance with CSS3. I've tested it against the SVG test suite and it behaves as expected. I didn't test it outside of SVG.
Assignee | ||
Updated•20 years ago
|
Attachment #165858 -
Flags: review?(tor)
Assignee | ||
Updated•20 years ago
|
Attachment #163614 -
Attachment is obsolete: true
Comment 11•20 years ago
|
||
Comment on attachment 165858 [details] [diff] [review] Patch for CSS3 currentColor Sneaked in a gradient fix, I see. Looks good from my outsider's view of the style system. Minor nit would be the ordering of kColorKTable.
Attachment #165858 -
Flags: review?(tor) → review+
Assignee | ||
Updated•20 years ago
|
Attachment #165858 -
Flags: superreview?(dbaron)
Comment on attachment 165858 [details] [diff] [review] Patch for CSS3 currentColor SetColor needs to set aInherited to PR_TRUE in the new case that you're adding, with a comment something like: // The data computed from this can't be shared in the rule tree because they could be used on a node with a different color. Other than that this looks OK.
Attachment #165858 -
Flags: superreview?(dbaron) → superreview-
The case that that breaks is the style rules: rect { fill: currentColor; } rect#a { color: orange; } rect#b { color: blue; } with markup: <rect id="a" ... /> <rect id="b" ... /> With your current code, both rects will end up with orange fill.
Er, actually, it wouldn't work with 'fill', since that's an inherited property, so you'd need to specify every other property in nsStyleSVG in the first rule in order to see the bug.. But s/rect/p/g, and s/fill/background-color/g, and you should see the bug with the simple testcase.
Assignee | ||
Comment 15•20 years ago
|
||
Here is the same patch, but I set inherited to PR_TRUE in SetColor as recommended by dbaron.
Attachment #165858 -
Attachment is obsolete: true
Assignee | ||
Updated•20 years ago
|
Attachment #167493 -
Flags: superreview?(dbaron)
Comment on attachment 167493 [details] [diff] [review] Updated patch for CSS3 currentColor including dbaron's requested change >Index: content/base/src/nsRuleNode.cpp >+ // The data computed from this can't be shared in the rule tree because Wrap this at less than 80 chars. >- // color: color, string, inherit Leave this line above the section. sr=dbaron
Attachment #167493 -
Flags: superreview?(dbaron) → superreview+
Assignee | ||
Comment 17•20 years ago
|
||
This patch incorporates dbaron's changes and also corrects a bug in handling the intValue units (result was never set to PR_TRUE). Thanks to Boris Zbarsky (bz) for catching this!
Attachment #167493 -
Attachment is obsolete: true
Assignee | ||
Updated•20 years ago
|
Attachment #167503 -
Flags: review?(bzbarsky)
Comment 18•20 years ago
|
||
Comment on attachment 167503 [details] [diff] [review] Added dbaron's changes r=bzbarsky
Attachment #167503 -
Flags: review?(bzbarsky) → review+
Comment 19•20 years ago
|
||
Comment on attachment 167503 [details] [diff] [review] Added dbaron's changes >RCS file: /cvsroot/mozilla/content/shared/src/nsCSSProps.cpp,v >diff -p -u -8 -r3.118 nsCSSProps.cpp >@@ -422,16 +422,17 @@ const PRInt32 nsCSSProps::kColorKTable[] ... > eCSSKeyword__moz_visitedhyperlinktext, NS_COLOR_MOZ_VISITEDHYPERLINKTEXT, >+ eCSSKeyword_currentcolor, NS_COLOR_CURRENTCOLOR, > eCSSKeyword_UNKNOWN,-1 > }; Shouldn't this have been kept alphabetical as with current conventions in this file?
Assignee | ||
Updated•20 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Comment 20•19 years ago
|
||
Hmm... I suck (and the patch didn't get enough testing). "color: currentColor" ends up using the user's default color instead of inheriting, as it should. See comments in bug 290664 for the reason.
The issue brought up in comment 20 is fixed in bug 332333 (since I noticed it while working on that).
Comment 22•16 years ago
|
||
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
•