Rendering support for ExtendedColors
<rdar://problem/29123243>
Created attachment 293968 [details] Patch
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
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
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
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
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().
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().
Comment on attachment 293968 [details] Patch All done on Simon's comments.
(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.
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.
Created attachment 294189 [details] Patch
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
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
Created attachment 294250 [details] Patch
Committed r208460: <http://trac.webkit.org/changeset/208460>
(In reply to comment #16) > Committed r208460: <http://trac.webkit.org/changeset/208460> It broke the EFL build, see build.webkit.org for details.
(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
The ENABLE(DATALIST_ELEMENT) build is broken, you simly should have search in the same file before committing.
Just to document, fix landed in https://trac.webkit.org/changeset/208506