[webkit-reviews] review granted: [Bug 187846] Picking a color from the color panel for typing attributes needs to inverse transform through color-filter : [Attachment 345431] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Ryosuke Niwa
<rniwa at webkit.org>'s request for review:
Bug 187846: Picking a color from the color panel for typing attributes needs to
inverse transform through color-filter
https://bugs.webkit.org/show_bug.cgi?id=187846

Attachment 345431: Patch

https://bugs.webkit.org/attachment.cgi?id=345431&action=review




--- Comment #4 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 345431
  --> https://bugs.webkit.org/attachment.cgi?id=345431
Patch

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

> Source/WebCore/editing/EditingStyle.cpp:1550
> +    bool hasColor = m_mutableStyle->getPropertyCSSValue(CSSPropertyColor);
> +    bool hasBackgroundColor =
m_mutableStyle->getPropertyCSSValue(CSSPropertyBackgroundColor);
> +    if (!hasColor && !hasBackgroundColor)
> +	   return *this;

There are other color properties (shadow color, stroke color etc), but I guess
we don't support them in editing yet.

> Source/WebCore/editing/EditingStyle.cpp:1553
> +    ASSERT(styleWithInvertedColors->m_mutableStyle);

Not sure this assert is useful, since you'll crash a couple of lines down
anyway.

> Source/WebCore/editing/EditingStyle.cpp:1555
> +    auto& colorFilter = renderer->style().appleColorFilter();

const auto&

> Source/WebCore/editing/EditingStyle.cpp:1566
> +	   Color newColor = textColorFromStyle(*m_mutableStyle);
> +	   colorFilter.inverseTransformColor(newColor);
> +	  
styleWithInvertedColors->m_mutableStyle->setProperty(CSSPropertyColor,
newColor.serialized());
> +    }
> +
> +    if (hasBackgroundColor) {
> +	   Color newColor = backgroundColorFromStyle(*m_mutableStyle);
> +	   colorFilter.inverseTransformColor(newColor);
> +	  
styleWithInvertedColors->m_mutableStyle->setProperty(CSSPropertyBackgroundColor
, newColor.serialized());
> +    }

If you just called cssValueToColor(extractPropertyValue(style,
<propertyID>).get()); instead of textColorFromStyle/backgroundColorFromStyle,
you could use a local lambda function to share a bit of code here.

Also, elsewhere we use cssText() for backgroundColor (to give rgb() colors)
rather than serialized (which gives hex colors). Should that happen here?

> Source/WebCore/editing/EditingStyle.h:171
> +    Ref<EditingStyle> inverseTransformColorIfNeeded(Element&);

This function can be const

> Source/WebCore/editing/EditorCommand.cpp:103
> +	   frame.editor().applyStyleToSelection(WTFMove(style), action,
Editor::ColorFilterMode::InvertColor); // Use InvertColor for testing purposes.

Does the comment still apply? Not sure if this is testing code left in.

>
LayoutTests/editing/execCommand/set-backColor-with-color-filter-from-scripts.ht
ml:7
> +Markup.description('Setting the background color should invert the color
through -apple-color-filter');

"should not", right?

>
LayoutTests/editing/execCommand/set-foreColor-with-color-filter-from-scripts.ht
ml:7
> +Markup.description('Setting the foreground color should invert the color
through -apple-color-filter');

Should not

>
LayoutTests/editing/mac/attributed-string/attribute-string-for-copy-with-color-
filter-expected.txt:25
> +    NSBackgroundColor: #336699 (sRGB)

I think should be a rgb() string, which cssText() would give you.

> LayoutTests/editing/style/set-backColor-with-color-filter.html:14
> +    testRunner.execCommand('backColor', false, '#224433');

I was really confused by this until I realized that testRunner.execCommand()
has different behavior to document.execCommmand(). Ick.