216559 - fill="currentColor" does not work.

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[darryl]

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>

Comment 1

[Erik Arvidsson]

This calor is actually part of the CSS3 color module as well so maybe it will
makes sense to put this under CSS instead?

Comment 2

[Hixie (not reading bugmail)]

Yeah, looks to me like this should just be implemented throughout instead of
being SVG specific.

Comment 3

[Scooter Morris]

Attachment: Initial patch to implement currentColor for SVG

Comment 4

[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.

Comment 5

[Scooter Morris]

(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 6

[tor]

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.

Comment 7

[Hixie (not reading bugmail)]

'currentColor' is just a valid CSS <color> value, not SVG-specific. IMHO it
should be implemented as such. But that's just me. :-)

Comment 8

[Scooter Morris]

(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. 

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I agree with comment 7.  I haven't yet looked at the patch here, though.

Comment 10

[Scooter Morris]

Attachment: Patch for CSS3 currentColor

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.

Comment 11

[tor]

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.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 15

[Scooter Morris]

Attachment: Updated patch for CSS3 currentColor including dbaron's requested change

Here is the same patch, but I set inherited to PR_TRUE in SetColor as
recommended by dbaron.

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 17

[Scooter Morris]

Attachment: Added dbaron's changes

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!

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 167503 [details] [diff] [review]
Added dbaron's changes

r=bzbarsky

Comment 19

[Justin Wood (:Callek)]

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?

Comment 20

[Boris Zbarsky [:bzbarsky]]

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.

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The issue brought up in comment 20 is fixed in bug 332333 (since I noticed it while working on that).

Comment 22

[Robert Longson [:longsonr]]

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