Bug 148879 – SVGColor custom text format is different from the CSS color custom text format

• Component: CSS
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

Created attachment 260669 [details]
test case

Open the attached test case.

Result: The css custom text format of the <div> tag is different from the css custom text format of the SVG <rect> tag.
Expected: Colors should always have the same format.

Notes: The problem is Color::serialized() function uses the hex format if the color is opaque. This function is used by SVGColor::customCSSText(). The CSS color itself does not use this function, but instead CSSPrimitiveValue::formatNumberForCustomCSSText() uses different format and code for the cases CSS_RGBCOLOR and CSS_PARSER_HEXCOLOR. This code seems to be the correct format for colors and it used by non SVG colors.

Also in the header file Color.h, this comment is there 

    // Returns the color serialized according to HTML5
    // - http://www.whatwg.org/specs/web-apps/current-work/#serialization-of-a-color
    WEBCORE_EXPORT String serialized() const;

The above link is broken and I could not find CSS specs for serialization of a color.

[Said Abou-Hallawa]

Created attachment 260671 [details]
Patch

[Build Bot]

Comment on attachment 260671 [details]
Patch

Attachment 260671 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/141770

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 260672 [details]
Archive of layout-test-results from ews106 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 260671 [details]
Patch

Attachment 260671 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/141796

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 260673 [details]
Archive of layout-test-results from ews101 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Said Abou-Hallawa]

Created attachment 260771 [details]
Patch

[Build Bot]

Comment on attachment 260771 [details]
Patch

Attachment 260771 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/151509

New failing tests:
svg/css/getComputedStyle-basic.xhtml
svg/dom/SVGPaint.html
svg/css/svg-attribute-parser-mode.html
svg/webarchive/svg-script-subresouces.svg
transitions/svg-transitions.html
svg/dom/SVGColor.html
svg/css/case-sensitive-tags.html
svg/dom/SVGStyleElement/disable-svg-style-element.html

[Build Bot]

Created attachment 260775 [details]
Archive of layout-test-results from ews103 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 260771 [details]
Patch

Attachment 260771 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/151513

New failing tests:
svg/dom/SVGStyleElement/disable-svg-style-element.html
svg/css/getComputedStyle-basic.xhtml
svg/dom/SVGPaint.html
svg/css/svg-attribute-parser-mode.html
svg/webarchive/svg-script-subresouces.svg
transitions/svg-transitions.html
svg/css/case-sensitive-tags.html
svg/dom/SVGColor.html

[Build Bot]

Created attachment 260778 [details]
Archive of layout-test-results from ews106 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Said Abou-Hallawa]

Created attachment 260782 [details]
Patch

[Daniel Bates]

Comment on attachment 260782 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=260782&action=review

> Source/WebCore/ChangeLog:10
> +        "2.4.6 Colors" in the spec http://www.w3.org/TR/html5/single-page.html only
> +        talks about parsing and deserializing simple colors which does not have

From my understanding, section 2.4.6 Colours in the HTML5 standard describes the parsing and serialization of a color with respect to HTML parsing and serialization (e.g. <body bgcolor="#00FF00">). The CSS Object Model spec defines the attribute cssText (<https://drafts.csswg.org/cssom/#dom-cssrule-csstext> - W3C Working Draft, 8 September 2015) and the parsing and serialization of CSS values. In particular, serialization of a CSS color value is described in <https://drafts.csswg.org/cssom/#serializing-css-values>.

> Source/WebCore/ChangeLog:19
> +        So for now I am going to unify the way the SVG color and the CSS colors are
> +        formatted when they are their custom text is required.

This sentence does not read well because of the phrase "they are".

> Source/WebCore/platform/graphics/Color.cpp:235
> +    result.reserveInitialCapacity(32);
> +    bool colorHasAlpha = hasAlpha();
> +    if (colorHasAlpha)
> +        result.append("rgba(", 5);
> +    else
> +        result.append("rgb(", 4);
>  
> +    appendNumber(result, static_cast<unsigned char>(red()));
> +    result.append(", ", 2);
> +
> +    appendNumber(result, static_cast<unsigned char>(green()));
> +    result.append(", ", 2);
> +
> +    appendNumber(result, static_cast<unsigned char>(blue()));
> +    if (colorHasAlpha) {
> +        result.append(", ", 2);

We should consider writing this code using StringBuilder. One of the benefits of using StringBuilder is that we can make use of StringBuilder::appendLiteral() to append a character literal without the need to specify the length of the character literal.

> Source/WebCore/platform/graphics/Color.cpp:238
> +        const char* alphaString = numberToFixedPrecisionString(alpha() / 255.0f, 6, buffer, true);

Is there any behavior change with using numberToFixedPrecisionString() instead of DecimalNumber::toStringDecimal() to serialize the alpha component?

Assuming it is acceptable to use numberToFixedPrecisionString() then it is unclear what the purpose of its last argument is without looking up the definition of this function. I suggest that we define a boolean local variable, say shouldTruncateTrailingZeros, and pass this variable instead of hardcoding the boolean value true.

> Source/WebCore/platform/graphics/Color.h:96
>      // - http://www.whatwg.org/specs/web-apps/current-work/#serialization-of-a-color

As you mentioned in <https://bugs.webkit.org/show_bug.cgi?id=148879#c0>, this URL is not valid as of the time of writing. We should update it to be <https://html.spec.whatwg.org/multipage/scripting.html#fill-and-stroke-styles> (10 September 2015). Please include the date of the standard, 10 September 2015, with the URL for historical preservation should the URL become invalid in the future.

> Source/WebCore/platform/graphics/Color.h:99
> +    // Retuns the color serialized as either rgb(r, g, b) or rgba(r, g, b, a)

I don't feel that this comment adds much value given that the name of this function, cssText, is standard terminology in the CSS Object Model. If you feel strongly about having a comment then I suggest the comment be a URL to the CSS Object Model specification, <https://drafts.csswg.org/cssom/#dom-cssrule-csstext>, and the date of the draft.

> LayoutTests/svg/css/computed-style-rgb-color.html:30
> +    <rect id="fully-opaque" width="100" height="100" fill="green"/>
> +    <rect id="fully-transparent" x="104" width="100" height="100" fill="none"/>
> +    <rect id="half-transparent" x="208" width="100" height="100" fill="rgba(0, 128, 0, 0.4)"/>

The trailing slashes (/) are not needed since this SVG markup is embedded in a HTML5 document.

[Simon Fraser (smfr)]

Comment on attachment 260782 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=260782&action=review

> Source/WebCore/platform/graphics/Color.cpp:239
> +    result.reserveInitialCapacity(32);
> +    bool colorHasAlpha = hasAlpha();
> +    if (colorHasAlpha)
> +        result.append("rgba(", 5);
> +    else
> +        result.append("rgb(", 4);
>  
> +    appendNumber(result, static_cast<unsigned char>(red()));
> +    result.append(", ", 2);
> +
> +    appendNumber(result, static_cast<unsigned char>(green()));
> +    result.append(", ", 2);
> +
> +    appendNumber(result, static_cast<unsigned char>(blue()));
> +    if (colorHasAlpha) {
> +        result.append(", ", 2);
> +
> +        NumberToStringBuffer buffer;
> +        const char* alphaString = numberToFixedPrecisionString(alpha() / 255.0f, 6, buffer, true);
> +        result.append(alphaString, strlen(alphaString));

I think this code should use StringBuilder.

[Said Abou-Hallawa]

Comment on attachment 260782 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=260782&action=review

>> Source/WebCore/ChangeLog:10
>> +        talks about parsing and deserializing simple colors which does not have
> 
> From my understanding, section 2.4.6 Colours in the HTML5 standard describes the parsing and serialization of a color with respect to HTML parsing and serialization (e.g. <body bgcolor="#00FF00">). The CSS Object Model spec defines the attribute cssText (<https://drafts.csswg.org/cssom/#dom-cssrule-csstext> - W3C Working Draft, 8 September 2015) and the parsing and serialization of CSS values. In particular, serialization of a CSS color value is described in <https://drafts.csswg.org/cssom/#serializing-css-values>.

The description is fixed in the new patch. I referred to the link you mentioned. I stated that the SVGColor is going to use the format for CSS color serialization always. The Color serialization is not affected which means that it still serializes the color in hex format if the alpha component is 1. This seems to be not what the new specs says but there was something unclear to me about the color being a component of a specified value. So I preferred to address this in a separate bug when I get clear about what the specs says. I would expect also the number of affected tests will be large.

>> Source/WebCore/ChangeLog:19
>> +        formatted when they are their custom text is required.
> 
> This sentence does not read well because of the phrase "they are".

Fixed.

>> Source/WebCore/platform/graphics/Color.cpp:235
>> +        result.append(", ", 2);
> 
> We should consider writing this code using StringBuilder. One of the benefits of using StringBuilder is that we can make use of StringBuilder::appendLiteral() to append a character literal without the need to specify the length of the character literal.

Done.

>> Source/WebCore/platform/graphics/Color.cpp:238
>> +        const char* alphaString = numberToFixedPrecisionString(alpha() / 255.0f, 6, buffer, true);
> 
> Is there any behavior change with using numberToFixedPrecisionString() instead of DecimalNumber::toStringDecimal() to serialize the alpha component?
> 
> Assuming it is acceptable to use numberToFixedPrecisionString() then it is unclear what the purpose of its last argument is without looking up the definition of this function. I suggest that we define a boolean local variable, say shouldTruncateTrailingZeros, and pass this variable instead of hardcoding the boolean value true.

I preferred to use DecimalNumber::toStringDecimal() since this was the one which Color::serialized() was calling originally while numberToFixedPrecisionString() was called by CSSPrimitiveValue::formatNumberForCustomCSSText(). DecimalNumber::toStringDecimal() is a lightweight function which might be more suitable for formatting a [0 1] value. However we may consider merging these two functions in one.

>> Source/WebCore/platform/graphics/Color.cpp:239
>> +        result.append(alphaString, strlen(alphaString));
> 
> I think this code should use StringBuilder.

Done.

>> Source/WebCore/platform/graphics/Color.h:96
>>      // - http://www.whatwg.org/specs/web-apps/current-work/#serialization-of-a-color
> 
> As you mentioned in <https://bugs.webkit.org/show_bug.cgi?id=148879#c0>, this URL is not valid as of the time of writing. We should update it to be <https://html.spec.whatwg.org/multipage/scripting.html#fill-and-stroke-styles> (10 September 2015). Please include the date of the standard, 10 September 2015, with the URL for historical preservation should the URL become invalid in the future.

A new comment is added.

>> Source/WebCore/platform/graphics/Color.h:99
>> +    // Retuns the color serialized as either rgb(r, g, b) or rgba(r, g, b, a)
> 
> I don't feel that this comment adds much value given that the name of this function, cssText, is standard terminology in the CSS Object Model. If you feel strongly about having a comment then I suggest the comment be a URL to the CSS Object Model specification, <https://drafts.csswg.org/cssom/#dom-cssrule-csstext>, and the date of the draft.

The comment is removed.

>> LayoutTests/svg/css/computed-style-rgb-color.html:30
>> +    <rect id="half-transparent" x="208" width="100" height="100" fill="rgba(0, 128, 0, 0.4)"/>
> 
> The trailing slashes (/) are not needed since this SVG markup is embedded in a HTML5 document.

Actually they are needed otherwise the <rect> elements will be nested and the query selectors below will fail. Instead of using 'svg>rect#fully-transparent' I have to use 'svg>rect>rect>#fully-transparent'. And instead of using 'svg>rect#half-transparent' I have to use 'svg>rect>rect>rect>#half-transparent'. I know I can use '#fully-transparent' and '#half-transparent' for selecting the <rect> elements in the SVG and this should be sufficient to get these elements. But I do not want to nest the <rect> elements that way since nesting them is meaningless.

[Said Abou-Hallawa]

Created attachment 260967 [details]
Patch

[Simon Fraser (smfr)]

We should make sure this patch doesn't break the "If <color> is a component of a specified value" part of the spec.

[Build Bot]

Comment on attachment 260967 [details]
Patch

Attachment 260967 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/158585

New failing tests:
css3/calc/color-hsl.html
editing/execCommand/query-command-value-background-color.html
fast/css3-text/css3-text-decoration/getComputedStyle/getComputedStyle-text-decoration-color.html
css3/calc/color-rgb.html

[Build Bot]

Created attachment 260973 [details]
Archive of layout-test-results from ews106 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 260967 [details]
Patch

Attachment 260967 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/158635

New failing tests:
css3/calc/color-rgb.html
css3/calc/color-hsl.html
editing/execCommand/query-command-value-background-color.html
fast/css3-text/css3-text-decoration/getComputedStyle/getComputedStyle-text-decoration-color.html

[Build Bot]

Created attachment 260975 [details]
Archive of layout-test-results from ews101 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Said Abou-Hallawa]

Created attachment 260979 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 260782 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=260782&action=review

>>> Source/WebCore/platform/graphics/Color.cpp:238
>>> +        const char* alphaString = numberToFixedPrecisionString(alpha() / 255.0f, 6, buffer, true);
>> 
>> Is there any behavior change with using numberToFixedPrecisionString() instead of DecimalNumber::toStringDecimal() to serialize the alpha component?
>> 
>> Assuming it is acceptable to use numberToFixedPrecisionString() then it is unclear what the purpose of its last argument is without looking up the definition of this function. I suggest that we define a boolean local variable, say shouldTruncateTrailingZeros, and pass this variable instead of hardcoding the boolean value true.
> 
> I preferred to use DecimalNumber::toStringDecimal() since this was the one which Color::serialized() was calling originally while numberToFixedPrecisionString() was called by CSSPrimitiveValue::formatNumberForCustomCSSText(). DecimalNumber::toStringDecimal() is a lightweight function which might be more suitable for formatting a [0 1] value. However we may consider merging these two functions in one.

I had to go back and use numberToFixedPrecisionString() because it allows specifying the precision of the formatted string while DecimalNumber does not. Many expected results have alpha output with precision of 6 digits. So instead of fixing many expected results, I am going to keep using numberToFixedPrecisionString(). 

However according to the new specs: https://drafts.csswg.org/cssom/#serializing-css-values, it seems neither of these functions should be used anymore for formatting the alpha as a float number since the <alphavalue> section in the specs says clearly how the unsigned char alpha value should be converted to float. But the specs does not state what precision should be used when formatting the floating alpha to a string. Because of that, I will not try to follow the specs here and I think we should wait until we know what precision should be used.

[Said Abou-Hallawa]

(In reply to comment #16)
> We should make sure this patch doesn't break the "If <color> is a component
> of a specified value" part of the spec.

I could not understand this part of the specs. But this patch is about ensuring the hex format is not used when serializing the SVGColor if the alpha component is 1. This should not disagree with the specs  https://drafts.csswg.org/cssom/#serializing-css-values since it did not mention hex format at all.

[WebKit Commit Bot]

Comment on attachment 260979 [details]
Patch

Clearing flags on attachment: 260979

Committed r189646: <http://trac.webkit.org/changeset/189646>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.