Bug 69152

Summary: Web Inspector: rgb() with percentages shows wrong hex/hsl values
Product: WebKit Reporter: Masataka Yakura <myakura.web>
Component: Web Inspector (Deprecated)Assignee: Joseph Pecoraro <joepeck>
Status: RESOLVED FIXED    
Severity: Normal CC: apavlov, bweinstein, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
URL: http://test.csswg.org/suites/css2.1/20110323/html4/border-bottom-color-057.htm
Attachments:
Description Flags
[PATCH] Fix RGB Percentage Values and Other Clamping
pfeldman: review-
[PATCH] Updated Test pfeldman: review+

Description Masataka Yakura 2011-09-30 09:54:20 PDT
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.
Comment 1 Joseph Pecoraro 2011-09-30 12:57:56 PDT
Good catch, I'll take a look at this since I wrote this original code.
Comment 2 Joseph Pecoraro 2011-09-30 13:13:08 PDT
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.
Comment 3 Pavel Feldman 2011-10-02 10:20:21 PDT
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.
Comment 4 Joseph Pecoraro 2011-10-03 11:17:46 PDT
> > 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.
Comment 5 Joseph Pecoraro 2011-10-03 11:18:04 PDT
Created attachment 109498 [details]
[PATCH] Updated Test
Comment 6 Pavel Feldman 2011-10-03 13:03:51 PDT
Comment on attachment 109498 [details]
[PATCH] Updated Test

Thanks!
Comment 7 Joseph Pecoraro 2011-10-03 14:49:25 PDT
Committed r96543: <http://trac.webkit.org/changeset/96543>.