Bug 69152 – Web Inspector: rgb() with percentages shows wrong hex/hsl values

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Masataka Yakura]

Steps to reproduce

1. Open http://test.csswg.org/suites/css2.1/20110323/html4/border-bottom-color-057.htm and inspect the 2nd white box.
2. Check out the style panel reports the background-color as #FFFFFF

Actual result:
I see #646464. Clicking the color box besides the value to see the HSL value, it shows hsl(0, 0%, 39%) not hsl(0, 100%, 100%).

CSS rgb() and rgba() color values usually takes three integer but also accept percentages.
http://www.w3.org/TR/css3-color/#rgb-color
100% maps to 255, and pecentages over 100% will be clipped to 100%, hence rgb(100%, 100%, 100%) or rgb(101%, 101%, 101%) should show #FFFFFF or hsl(0, 100%, 100%) in the style panel.

However, both _rgbToHex() and _rgbToHSL() in inspector/front-end/Color.js just passes values through parseInt().toString(16) even if the values are percentages, so it causes wrong base conversion. given 'rgb(100%, 100%, 100%)', toString('100%') just drop the '%' and toString(16) will convert the left '100' to '64', so it'll end up showing #646464 in the style panel.

[Joseph Pecoraro]

Good catch, I'll take a look at this since I wrote this original code.

[Joseph Pecoraro]

Created attachment 109336 [details]
[PATCH] Fix RGB Percentage Values and Other Clamping

This patch:

  • handles individual RGB values that could be either a % or a decimal/float.
  • handles clamping RGB values

  • handles clamping alpha/opacity values for RGBA/HSLA
    http://www.w3.org/TR/css3-color/#transparency
    """The same as the specified value after clipping the <alphavalue> to the range [0.0,1.0]."""

  • handles negative saturation HSL values:
    http://www.w3.org/TR/css3-color/#hsl-color
    """If saturation is less than 0%, implementations must clip it to 0%"""

I don't know much about the handling of other unclamped HSL values, but we seemed to do okay.
I didn't include that in my test, another bug might need to be opened to add specific tests for HSL.

[Pavel Feldman]

Comment on attachment 109336 [details]
[PATCH] Fix RGB Percentage Values and Other Clamping

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

r- for the test, otherwise looks good!

> LayoutTests/ChangeLog:8
> +        THe test outputs the Original (Authored), RGB, Nickname, Hex, and HSL

Nit: The test

> LayoutTests/ChangeLog:9
> +        representations for a specified nodes. For some of the CSS values their

Please consider rephrasing.

> LayoutTests/inspector/styles/styles-invalid-color-values.html:23
> +    InspectorTest.runTestSuite([

Style tests are slow and flaky. In this case, you should do a simple test suite that would execute code snippets against Color object.

> LayoutTests/inspector/styles/styles-invalid-color-values.html:31
> +            WebInspector.settings.colorFormat.set(WebInspector.StylesSidebarPane.ColorFormat.Original);

Mutating settings from tests is risky: we don't reset them between the test runs + since this is localStorage, parallel test runner might suffer.

[Joseph Pecoraro]

> > LayoutTests/ChangeLog:8
> > +        THe test outputs the Original (Authored), RGB, Nickname, Hex, and HSL
> 
> Nit: The test
> 
> > LayoutTests/ChangeLog:9
> > +        representations for a specified nodes. For some of the CSS values their
> 
> Please consider rephrasing.

Yah, my grammar and spelling in ChangeLogs recently has been horrible. I've rewritten this. Thanks.

> > LayoutTests/inspector/styles/styles-invalid-color-values.html:23
> > +    InspectorTest.runTestSuite([
> 
> Style tests are slow and flaky. In this case, you should do a simple test suite that would
> execute code snippets against Color object.

Done. Tests are run directly against WebInspector.Color.

[Joseph Pecoraro]

Created attachment 109498 [details]
[PATCH] Updated Test

[Pavel Feldman]

Comment on attachment 109498 [details]
[PATCH] Updated Test

Thanks!

[Joseph Pecoraro]

Committed r96543: <http://trac.webkit.org/changeset/96543>.