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.
Created attachment 260671 [details] Patch
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.
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
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.
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
Created attachment 260771 [details] Patch
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
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
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
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
Created attachment 260782 [details] Patch
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.
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.
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.
Created attachment 260967 [details] Patch
We should make sure this patch doesn't break the "If <color> is a component of a specified value" part of the spec.
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
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
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
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
Created attachment 260979 [details] Patch
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.
(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.
Comment on attachment 260979 [details] Patch Clearing flags on attachment: 260979 Committed r189646: <http://trac.webkit.org/changeset/189646>
All reviewed patches have been landed. Closing bug.