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>
Created attachment 345431 [details] Patch
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
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
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.
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.
Created attachment 345475 [details] Patch for landing
Comment on attachment 345475 [details] Patch for landing Wait for EWS first.
Comment on attachment 345475 [details] Patch for landing Clearing flags on attachment: 345475 Committed r234064: <https://trac.webkit.org/changeset/234064>
All reviewed patches have been landed. Closing bug.