Bug 147932 - Browser does not fall back to SVG attribute value when CSS style value is invalid or not supported
Summary: Browser does not fall back to SVG attribute value when CSS style value is inv...
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Antoine Quint
Keywords: InRadar
Depends on:
Reported: 2015-08-12 01:44 PDT by Sara Soueidan
Modified: 2015-11-29 14:03 PST (History)
9 users (show)

See Also:

Screenshot of what the demo should look like when CSS variables are not supported (88.05 KB, image/png)
2015-08-12 01:46 PDT, Sara Soueidan
no flags Details
Testcase (129 bytes, image/svg+xml)
2015-11-24 05:12 PST, Antoine Quint
no flags Details
Patch (12.32 KB, patch)
2015-11-26 06:15 PST, Antoine Quint
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sara Soueidan 2015-08-12 01:44:06 PDT
An SVG element that has a fill color specified using both a fill attribute and a style attribute with fill property declaration, the browser needs to be able to fall back to the fill attribute value if the value in the style attribute is not valid.

In this particular case, an SVG element is styled using CSS variables inside a style attribute. Since Safari does not support CSS variables yet, it is supposed to ignore the fill value in the style tag (using the variable) and fall back to the fill value in the fill attribute.

Live example: http://codepen.io/SaraSoueidan/pen/c30e270090b2460ba6e6833c611e5a76

The robots should all be colored and look alike in Safari, as they do in Chrome, for example. But instead, Safari falls back to all black colors (the user agent defaults) because it does not use the fill attribute fallback provided.

More reference on this technique, how it works, and what's supposed to happen here: http://tympanus.net/codrops/2015/07/16/styling-svg-use-content-css/
Comment 1 Sara Soueidan 2015-08-12 01:46:07 PDT
Created attachment 258817 [details]
Screenshot of what the demo should look like when CSS variables are not supported

This screenshot shows how the robots should look when CSS variables are not supported; i.e. if Safari does fall back appropriately. It currently shows all-black robots instead.
Comment 2 Dean Jackson 2015-08-12 18:00:13 PDT
Comment 3 Antoine Quint 2015-11-24 05:12:46 PST
Created attachment 266130 [details]

Simple test case showing that setting style="fill: whatever", which is invalid, means that the valid fill="green" attribute won't get picked up.
Comment 4 Antoine Quint 2015-11-24 06:17:11 PST
For some background on this, I think this is the relevant part of the SVG 1.1 specification:


In particular, this excerpt:

"For user agents that support CSS, the presentation attributes must be translated to corresponding CSS style rules according to rules described in Precedence of non-CSS presentational hints ([CSS2], section 6.4.4), with the additional clarification that the presentation attributes are conceptually inserted into a new author style sheet which is the first in the author style sheet collection. The presentation attributes thus will participate in the CSS2 cascade as if they were replaced by corresponding CSS style rules placed at the start of the author style sheet with a specificity of zero. In general, this means that the presentation attributes have lower priority than other CSS style rules specified in author style sheets or ‘style’ attributes."
Comment 5 Antoine Quint 2015-11-24 08:28:26 PST
As far as I can understand so far, the MatchResult passed to StyleResolver::applyCascadedProperties has all the properties needed to make the right decision and fall back on a valid value. It doesn't use the valid property for reasons that aren't clear yet.
Comment 6 Antoine Quint 2015-11-24 08:46:05 PST
However, still in StyleResolver::applyCascadedProperties(), the cascaded properties don't have the expected values, so my guess is that something is going wrong when setting up the cascade in StyleResolver::applyMatchedProperties():

    CascadedProperties cascade(direction, writingMode);
    cascade.addMatches(matchResult, false, 0, matchResult.matchedProperties().size() - 1, applyInheritedOnly);
    cascade.addMatches(matchResult, true, matchResult.ranges.firstAuthorRule, matchResult.ranges.lastAuthorRule, applyInheritedOnly);
    cascade.addMatches(matchResult, true, matchResult.ranges.firstUserRule, matchResult.ranges.lastUserRule, applyInheritedOnly);
    cascade.addMatches(matchResult, true, matchResult.ranges.firstUARule, matchResult.ranges.lastUARule, applyInheritedOnly);
Comment 7 Antoine Quint 2015-11-26 06:15:04 PST
Created attachment 266184 [details]
Comment 8 Antoine Quint 2015-11-26 06:20:33 PST
Another fix I had in mind was to `return false` if `valueWithCalculation.value().id == CSSValueInvalid` was true in `CSSParser::parseSVGValue()`. This would mean that any value that is deemed invalid would be dropped. But since it was a much broader fix, I went for a more specialised fix for the case of SVGPaint values. I also wasn't entirely sure of the meaning of `CSSValueInvalid`, does it inform us with absolute certainty that the value is invalid, or only that with checks applied at this point that the value would likely be invalid?
Comment 9 WebKit Commit Bot 2015-11-29 14:03:29 PST
Comment on attachment 266184 [details]

Clearing flags on attachment: 266184

Committed r192788: <http://trac.webkit.org/changeset/192788>
Comment 10 WebKit Commit Bot 2015-11-29 14:03:33 PST
All reviewed patches have been landed.  Closing bug.