Bug 164443 – Rendering support for ExtendedColors

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dean Jackson]

Rendering support for ExtendedColors

[Dean Jackson]

<rdar://problem/29123243>

[Dean Jackson]

Created attachment 293968 [details]
Patch

[Build Bot]

Comment on attachment 293968 [details]
Patch

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

New failing tests:
editing/mac/attributed-string/anchor-element.html
editing/mac/attributed-string/basic.html

[Build Bot]

Created attachment 293977 [details]
Archive of layout-test-results from ews100 for mac-yosemite

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

[Build Bot]

Comment on attachment 293968 [details]
Patch

Attachment 293968 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2464847

New failing tests:
css3/color/composited-solid-backgrounds.html

[Build Bot]

Created attachment 293980 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Simon Fraser (smfr)]

Comment on attachment 293968 [details]
Patch

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

> Source/WebCore/platform/graphics/Color.h:231
> +    // Returns a color that has the same RGB values, but with A *= amount.
> +    Color colorFadedByAmount(float) const;

I don't really like this name. "Faded" could imply "reduced saturation". Maybe colorWithAlphaMultipliedBy()? With the right name, the comment should not be necessary.

> Source/WebCore/platform/graphics/Color.h:424
> +inline bool isBlackColor(const Color& color)

If this were static, it would be Color::isBlack(color) which would be a bit nicer.

> Source/WebCore/platform/graphics/Color.h:428
> +        return !extendedColor.red() && !extendedColor.green() && !extendedColor.blue() && extendedColor.alpha() == 1;

extendedColor.alpha() == 1 - > isOpaque?

> Source/WebCore/platform/graphics/Color.h:434
> +inline bool isWhiteColor(const Color& color)

Same.

> Source/WebCore/platform/graphics/Color.h:438
> +        return extendedColor.red() == 1 && extendedColor.green() == 1 && extendedColor.blue() == 1 && extendedColor.alpha() == 1;

Ditto.

> Source/WebCore/platform/graphics/mac/ColorMac.mm:95
> +    static RetainPtr<NSColor>* cachedColors = new RetainPtr<NSColor>[cacheSize];

Why isn't this a NeverDestroyed?

> Source/WebCore/rendering/RenderElement.cpp:2129
> +        outlineColor = outlineColor.colorWithAlpha(1.0);

I wonder if Color should have a helper or colorWithAlpha(1.0), like opaqueColor().

[Darin Adler]

Comment on attachment 293968 [details]
Patch

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

Long term I would like to understand how we can differentiate call sites where calling something like red() or alpha() is OK, and call sites where it represents a bug. Do you think we might ever be able to remove those functions?

> Source/WebCore/html/ColorInputType.cpp:69
> +    // FIXME: Why does this have to be in the #RRGGBB form?
>      // We don't accept #rgb and #aarrggbb formats.
>      if (value.length() != 7)
>          return false;
>      Color color(value);
> -    return color.isValid() && !color.hasAlpha();
> +    return color.isValid() && color.isOpaque(); // FIXME: Why does it need to be opaque?

The answer to both your questions is that this is what the specification calls for:

    https://html.spec.whatwg.org/multipage/infrastructure.html#valid-simple-colour

I suggest we rename the functions here to match the wording in the specification as closely as possible. This function be probably be named isValidSimpleColorString.

I also don’t think this function should really use the Color class. The job of checking if a string is # followed by six ASCII hex digits doesn’t seem to be something we need to involve the Color class with. Unless the Color class defines the concept of simple color, valid simple color, valid lowercase simple color etc.

> Source/WebCore/platform/graphics/cocoa/FontCascadeCocoa.mm:276
> +        float fade = fillColor.isExtended() ? fillColor.asExtended().alpha() : (fillColor.alpha() / 255);

I think this should use alphaAsFloat().

[Dean Jackson]

Comment on attachment 293968 [details]
Patch

All done on Simon's comments.

[Dean Jackson]

(In reply to comment #8)

> Long term I would like to understand how we can differentiate call sites
> where calling something like red() or alpha() is OK, and call sites where it
> represents a bug. Do you think we might ever be able to remove those
> functions?

I'm hoping to get to that place. There shouldn't be too many places outside of rendering where we need to know the color component values. We also probably don't need to know anything beyond "is this color completely transparent" or "completely opaque". All other rendering shouldn't care and should just use the platform color representation.

One of the few places that does look at components is blending/animation, and gradients to some extent. For those I hope to again remove all the component access and just work through helpers on the Color class. After all, we'll need to do something to blend between colors in different color spaces, which will require platform support (e.g. the SPI CGColorTransform on Cocoa).

Work in progress!! It's a lot of fun so far.

[Dean Jackson]

Comment on attachment 293968 [details]
Patch

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

>> Source/WebCore/html/ColorInputType.cpp:69
>> +    return color.isValid() && color.isOpaque(); // FIXME: Why does it need to be opaque?
> 
> The answer to both your questions is that this is what the specification calls for:
> 
>     https://html.spec.whatwg.org/multipage/infrastructure.html#valid-simple-colour
> 
> I suggest we rename the functions here to match the wording in the specification as closely as possible. This function be probably be named isValidSimpleColorString.
> 
> I also don’t think this function should really use the Color class. The job of checking if a string is # followed by six ASCII hex digits doesn’t seem to be something we need to involve the Color class with. Unless the Color class defines the concept of simple color, valid simple color, valid lowercase simple color etc.

Thanks for the reference. This method has been renamed and now doesn't use color at all. It simply looks at the string to see if it is a valid #RRGGBB. (It never needed to test opaqueness anyway, since a 7 character hex can't have transparency)

>> Source/WebCore/platform/graphics/cocoa/FontCascadeCocoa.mm:276
>> +        float fade = fillColor.isExtended() ? fillColor.asExtended().alpha() : (fillColor.alpha() / 255);
> 
> I think this should use alphaAsFloat().

Done.

[Dean Jackson]

Created attachment 294189 [details]
Patch

[Build Bot]

Comment on attachment 294189 [details]
Patch

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

New failing tests:
editing/mac/attributed-string/anchor-element.html
editing/mac/attributed-string/basic.html

[Build Bot]

Created attachment 294207 [details]
Archive of layout-test-results from ews100 for mac-yosemite

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

[Dean Jackson]

Created attachment 294250 [details]
Patch

[Dean Jackson]

Committed r208460: <http://trac.webkit.org/changeset/208460>

[Csaba Osztrogonác]

(In reply to comment #16)
> Committed r208460: <http://trac.webkit.org/changeset/208460>

It broke the EFL build, see build.webkit.org for details.

[Csaba Osztrogonác]

(In reply to comment #17)
> (In reply to comment #16)
> > Committed r208460: <http://trac.webkit.org/changeset/208460>
> 
> It broke the EFL build, see build.webkit.org for details.

https://build.webkit.org/builders/EFL%20Linux%2064-bit%20Release%20WK2/builds/30523

[Csaba Osztrogonác]

The ENABLE(DATALIST_ELEMENT) build is broken, you simly should have search in the same file before committing.

[Csaba Osztrogonác]

Just to document, fix landed in https://trac.webkit.org/changeset/208506