Bug 144423 – Create a named CSS property for system colors

• Component: New Bugs
• Status: RESOLVED FIXED

[Dean Jackson]

Create a named CSS property for system colors

[Dean Jackson]

<rdar://problem/20491011>

[Dean Jackson]

Created attachment 252016 [details]
Patch

[Dean Jackson]

The UIColorSPI.h is in the wrong dir in the patch. Will fix on commit.

[Simon Fraser (smfr)]

Comment on attachment 252016 [details]
Patch

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

I'd like to see test that used currentColor, and inherit with both system colors, and system colors combined with currentColor

> Source/WebCore/rendering/RenderThemeIOS.h:126
> +    mutable HashMap<int, RGBA32> m_systemColorCache;

int -> CSSValueID?

> Source/WebCore/rendering/RenderThemeIOS.mm:1321
> +        HashMap<int, RGBA32>::iterator it = m_systemColorCache.find(cssValueId);

auto!

> Source/WebCore/rendering/RenderThemeIOS.mm:1356
> +    if (!color.isValid())
> +        color = RenderTheme::systemColor(cssValueId);
> +
> +    if (color.isValid())

else

> Source/WebCore/rendering/RenderThemeIOS.mm:1357
> +        m_systemColorCache.set(cssValueId, color.rgb());

Is this OK with colors in a non-RGB colorspace? It looks like Color(CGColorRef color) handles that, but not sure.

[Tim Horton]

Comment on attachment 252016 [details]
Patch

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

> Source/WebCore/css/CSSParser.cpp:517
> +bool CSSParser::validSystemColorValue(CSSValueID valueID)

Does it make sense to have "system" in the name here?

Also, does it want an "is" prefix?

Also, is it const?

> Source/WebCore/platform/spi/UIColorSPI.h:1
> +/*

As you noted, this header is not in the right place.

> Source/WebCore/platform/spi/cocoa/NSColorSPI.h:30
> +#if PLATFORM(MAC) || USE(APPLE_INTERNAL_SDK)

Do you really want an OR here? What about people who are PLATFORM(MAC) and don't have NSColor_Private.h?

> Source/WebCore/rendering/RenderThemeIOS.mm:77
> ++ (UIApplication *)sharedApplication;

I'm sure this is in some SPI header that you can use instead

> Source/WebCore/rendering/RenderThemeIOS.mm:82
> +@property (nonatomic, readonly) CGColorRef CGColor;

Why is this not in the SPI header. Also, isn't this public? (it is) What's going on here?

[Tim Horton]

(In reply to comment #4)
> Comment on attachment 252016 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=252016&action=review
> 
> I'd like to see test that used currentColor, and inherit with both system
> colors, and system colors combined with currentColor

Good idea.

> > Source/WebCore/rendering/RenderThemeIOS.mm:1356
> > +    if (!color.isValid())
> > +        color = RenderTheme::systemColor(cssValueId);
> > +
> > +    if (color.isValid())
> 
> else

No! Reassigning.

[Dean Jackson]

Comment on attachment 252016 [details]
Patch

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

>> Source/WebCore/css/CSSParser.cpp:517
>> +bool CSSParser::validSystemColorValue(CSSValueID valueID)
> 
> Does it make sense to have "system" in the name here?
> 
> Also, does it want an "is" prefix?
> 
> Also, is it const?

I think it does because this is mostly testing for the UI colors like buttonhighlight, text focus, etc, but nothing else. In other words, these are special colors that are not defined in a specification.

Yes, it should have the "is" prefix.

No, not const because it is a static method.

>> Source/WebCore/platform/spi/cocoa/NSColorSPI.h:30
>> +#if PLATFORM(MAC) || USE(APPLE_INTERNAL_SDK)
> 
> Do you really want an OR here? What about people who are PLATFORM(MAC) and don't have NSColor_Private.h?

Err... yeah.

>> Source/WebCore/rendering/RenderThemeIOS.h:126
>> +    mutable HashMap<int, RGBA32> m_systemColorCache;
> 
> int -> CSSValueID?

Yes. I should fix RenderThemeMac too.

>> Source/WebCore/rendering/RenderThemeIOS.mm:77
>> ++ (UIApplication *)sharedApplication;
> 
> I'm sure this is in some SPI header that you can use instead

This isn't new code. But, I did have to put it in the USE guard because UIApplication now comes in via UIColorSPI.h. So this is just for the people who don't have the internal SDK.

>> Source/WebCore/rendering/RenderThemeIOS.mm:82
>> +@property (nonatomic, readonly) CGColorRef CGColor;
> 
> Why is this not in the SPI header. Also, isn't this public? (it is) What's going on here?

I guess I should put it in the SPI header, but it was weird to put non-SPI there. Alternatively, I should just work out which part of UIKit to include to get the UIColor definition.

>> Source/WebCore/rendering/RenderThemeIOS.mm:1321
>> +        HashMap<int, RGBA32>::iterator it = m_systemColorCache.find(cssValueId);
> 
> auto!

ok

>> Source/WebCore/rendering/RenderThemeIOS.mm:1357
>> +        m_systemColorCache.set(cssValueId, color.rgb());
> 
> Is this OK with colors in a non-RGB colorspace? It looks like Color(CGColorRef color) handles that, but not sure.

Yes. I originally wrote some colorspace code and then realised that Color would do it all for me.

[Dean Jackson]

Created attachment 252025 [details]
Patch

[Darin Adler]

Comment on attachment 252016 [details]
Patch

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

> Source/WebCore/ChangeLog:24
> +        On platforms other than OS X Yosemite and iOS, the
> +        actual color values are undefined and become transparent
> +        black. (In fact, not all are defined on iOS either.)

Or should they perhaps just be hardcoded pure hues of the named colors? What’s the rationale for having them mean “transparent”?

> Source/WebCore/css/CSSParser.h:124
> +    static bool validSystemColorValue(CSSValueID);

A function named “valid value” should return a value. I suggest naming this “isSystemColorValue” or “isSystemColor”.

> Source/WebCore/rendering/RenderThemeIOS.h:114
> +    virtual Color systemColor(CSSValueID) const override;

Maybe make this private. I don’t think it needs to be called non-polymorphically.

>>> Source/WebCore/rendering/RenderThemeIOS.mm:1357
>>> +        m_systemColorCache.set(cssValueId, color.rgb());
>> 
>> Is this OK with colors in a non-RGB colorspace? It looks like Color(CGColorRef color) handles that, but not sure.
> 
> Yes. I originally wrote some colorspace code and then realised that Color would do it all for me.

This should be done with add so that we can modify the cache in place. This will avoid the find/set combo with two hash table lookups:

    auto addResult = m_systemColorCache.add(cssValueID, 0);
    if (!addResult.isNewEntry)
        return addResult.iterator->value;

    ...

    addResult.iterator->value = color.rgb();

It’s not good that when the color is not valid we were not putting it in the hash cache all. That means we’d run this code over and over every time, always doing the work and never caching anything. We need to cache the failure.

> Source/WebCore/rendering/RenderThemeIOS.mm:1359
> +    return color;

Should probably return addResult.iterator->value because we don’t want to return an invalid color the first time, and then return a valid color subsequent times. Or store Color objects in the hash map.

[Dean Jackson]

Created attachment 252030 [details]
Patch

[Dean Jackson]

Committed r183610: <http://trac.webkit.org/changeset/183610>

[Joseph Pecoraro]

Comment on attachment 252030 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        Expose the following values to CSS color properties:

Should we include them in Web Inspector autocompletion?

[Joseph Pecoraro]

Any idea with the Windows 7 bots is failing this test? Somehow it is expecting non (0,0,0,0) output. So does it fallback to mac instead of using the default platform results in this case?

Builder:
https://build.webkit.org/builders/Apple%20Win%207%20Release%20%28Tests%29/builds/51471

Results:
https://build.webkit.org/results/Apple%20Win%207%20Release%20(Tests)/r183613%20(51471)/results.html

[Joseph Pecoraro]

(In reply to comment #13)
> Any idea with the Windows 7 bots is failing this test? Somehow it is
> expecting non (0,0,0,0) output. So does it fallback to mac instead of using
> the default platform results in this case?
> 
> Builder:
> https://build.webkit.org/builders/Apple%20Win%207%20Release%20%28Tests%29/
> builds/51471
> 
> Results:
> https://build.webkit.org/results/Apple%20Win%207%20Release%20(Tests)/
> r183613%20(51471)/results.html

Addressed by:
https://trac.webkit.org/changeset/183617

[Brent Fulgham]

(In reply to comment #13)
> Any idea with the Windows 7 bots is failing this test? Somehow it is
> expecting non (0,0,0,0) output. So does it fallback to mac instead of using
> the default platform results in this case?

Yes. Windows tries to use Mac results whenever it doesn't have its own.

[Darin Adler]

Dean, did you ever see my comments?

[Dean Jackson]

Comment on attachment 252016 [details]
Patch

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

>> Source/WebCore/ChangeLog:24
>> +        black. (In fact, not all are defined on iOS either.)
> 
> Or should they perhaps just be hardcoded pure hues of the named colors? What’s the rationale for having them mean “transparent”?

I flipflopped on this. I figured there were three choices:

- black (at least you'd see it)
- something that looks like what you asked for (but then you don't really know you have the wrong color)
- do what we do for the other system colors like "buttonhighlight" (which becomes "transparent")

I figured that for consistency, I'd go for the latter.

Note that this is not like an unrecognized property value. The keywords will exist on other ports, which is a bit annoying because you can't easily make fallbacks. However, I guess that is the idea of the system values.

>> Source/WebCore/css/CSSParser.h:124
>> +    static bool validSystemColorValue(CSSValueID);
> 
> A function named “valid value” should return a value. I suggest naming this “isSystemColorValue” or “isSystemColor”.

Yes, Tim mentioned this in review. It landed as isSystemColorValue.

>> Source/WebCore/rendering/RenderThemeIOS.h:114
>> +    virtual Color systemColor(CSSValueID) const override;
> 
> Maybe make this private. I don’t think it needs to be called non-polymorphically.

OK. I changed RenderThemeMac too.

>>>> Source/WebCore/rendering/RenderThemeIOS.mm:1357
>>>> +        m_systemColorCache.set(cssValueId, color.rgb());
>>> 
>>> Is this OK with colors in a non-RGB colorspace? It looks like Color(CGColorRef color) handles that, but not sure.
>> 
>> Yes. I originally wrote some colorspace code and then realised that Color would do it all for me.
> 
> This should be done with add so that we can modify the cache in place. This will avoid the find/set combo with two hash table lookups:
> 
>     auto addResult = m_systemColorCache.add(cssValueID, 0);
>     if (!addResult.isNewEntry)
>         return addResult.iterator->value;
> 
>     ...
> 
>     addResult.iterator->value = color.rgb();
> 
> It’s not good that when the color is not valid we were not putting it in the hash cache all. That means we’d run this code over and over every time, always doing the work and never caching anything. We need to cache the failure.

Great suggestion. I've changed this and RenderThemeMac to have a map of Colors rather than RGBA32s, that way I can keep invalid/missing color values in the cache.

[Dean Jackson]

Committed r183778: <http://trac.webkit.org/changeset/183778>