Closed Bug 216559 Opened 21 years ago Closed 20 years ago

fill="currentColor" does not work.

Categories

(Core :: SVG, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: darryl, Assigned: scootermorris)

References

()

Details

(Keywords: css3)

Attachments

(1 file, 3 obsolete files)

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>
Status: UNCONFIRMED → NEW
Ever confirmed: true
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
Yeah, looks to me like this should just be implemented throughout instead of
being SVG specific.
QA Contact: ian
Assignee: alex → scootermorris
Status: NEW → ASSIGNED
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.
(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
&lt;fill,fill-rule,fill-opacity properites&gt;
  </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+
'currentColor' is just a valid CSS <color> value, not SVG-specific. IMHO it
should be implemented as such. But that's just me. :-)
(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.
Attached patch Patch for CSS3 currentColor (obsolete) — Splinter Review
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.
Attachment #165858 - Flags: review?(tor)
Attachment #163614 - Attachment is obsolete: true
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+
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.
Here is the same patch, but I set inherited to PR_TRUE in SetColor as
recommended by dbaron.
Attachment #165858 - Attachment is obsolete: true
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+
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
Attachment #167503 - Flags: review?(bzbarsky)
Comment on attachment 167503 [details] [diff] [review]
Added dbaron's changes

r=bzbarsky
Attachment #167503 - Flags: review?(bzbarsky) → review+
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?
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
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).
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: