Bug 187846 – Picking a color from the color panel for typing attributes needs to inverse transform through color-filter

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Ryosuke Niwa]

If you pick pale yellow from the color panel in mail compose, we set the typing attributes to dark yellow, and text appears dark yellow. This is wrong; we need to transform the color through the -apple-color-filter style for the current insertion location so that when the user types, the inserted text appears with the color that they picked from the color picker.

<rdar://problem/42350765>

[Ryosuke Niwa]

Created attachment 345431 [details]
Patch

[EWS Watchlist]

Comment on attachment 345431 [details]
Patch

Attachment 345431 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/8596795

New failing tests:
editing/mac/attributed-string/attribute-string-for-copy-with-color-filter.html
editing/mac/attributed-string/attrib-string-range-with-color-filter.html

[EWS Watchlist]

Created attachment 345433 [details]
Archive of layout-test-results from ews103 for mac-sierra

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

[Simon Fraser (smfr)]

Comment on attachment 345431 [details]
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.html: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.html: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.

[Ryosuke Niwa]

Comment on attachment 345431 [details]
Patch

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

>> Source/WebCore/editing/EditingStyle.cpp:1550
>> +        return *this;
> 
> There are other color properties (shadow color, stroke color etc), but I guess we don't support them in editing yet.

Yeah, we don't really care those.

>> 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.

This is to document the fact copy() should have made non-null m_mutableStyle.

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

Fixed.

>> Source/WebCore/editing/EditingStyle.cpp:1566
>> +    }
> 
> 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?

I don't think it really matters because we're just parsing it back as color.
Anyway, I'm gonna just use cssText(). Extracted this as invertedColor.

>> Source/WebCore/editing/EditingStyle.h:171
>> +    Ref<EditingStyle> inverseTransformColorIfNeeded(Element&);
> 
> This function can be const

Well, but then we'd have to const_cast this since we sometimes return *this.

>> 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.

Yes. ColorFilterMode::InvertColor is only needed for foreColor & backColor
but we never trigger those with source=CommandFromMenuOrKeyBinding.
I revised the comment to say: // Use InvertColor for testing purposes. foreColor and backColor are never triggered with CommandFromMenuOrKeyBinding outside DRT/WTR.

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

Fixed.

>> 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.

This is dump-attributed-string.js doing the conversion, not our C++ code. What we have here is NSColor anyway.

>> 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.

Yeah, we should probably rename testRunner.execCommand to something like testRunner.execCommandFromMenu.
But we probably shouldn't make that change in this patch.

[Ryosuke Niwa]

Created attachment 345475 [details]
Patch for landing

[Ryosuke Niwa]

Comment on attachment 345475 [details]
Patch for landing

Wait for EWS first.

[WebKit Commit Bot]

Comment on attachment 345475 [details]
Patch for landing

Clearing flags on attachment: 345475

Committed r234064: <https://trac.webkit.org/changeset/234064>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.