Bug 201368 – Mail appears to be double inverting code copied from Notes, Xcode, or Terminal

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

[Timothy Hatcher]

Copied text from Xcode or Terminal in dark mode will show as light when pasted in a dark mode Mail compose window.

<rdar://problem/40529867>

[Timothy Hatcher]

Created attachment 377773 [details]
Patch

[Timothy Hatcher]

iOS EWS failures are unrelated.

[Ryosuke Niwa]

Comment on attachment 377773 [details]
Patch

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

Are we trying to detect the dark mode content? That doesn't sound right.
The attributed string itself should contain that information.

r- because I don't think this patch should be landed as is.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:530
> +    ContainerNode* context = insertedNodes.firstNodeInserted()->parentNode();

This is not safe. updateLayoutIgnorePendingStylesheets can delete this node. Need to use RefPtr.
Also, why is it okay to not do this work when we're inserting into a list item?

This work probably needs to be done right before we start inserting contents where hasBlankLinesBetweenParagraphs, etc... is computed.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:537
> +    auto* contextRenderer = context->renderer();
> +    if (!contextRenderer || !contextRenderer->style().hasAppleColorFilter())

Why don't we just check the style of the root editable element?
Realistically speaking, applying color filter to some part of the editable region isn't gonna work anyway.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:541
> +    // Only the apple-invert-lightness() color filter is supported. It is the only
> +    // FilterOperation that implements inverseTransformColor().

This is obvious from the code. I don't think we need this comment.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:564
> +    // Check the top-level inserted node's text and background colors. If any node has
> +    // dark text or light background colors, abort before the transformation step.

I don't think we need this comment. It's self-evident from the code.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:565
> +    RefPtr<Node> pastEndNode = insertedNodes.pastLastSibling();

Please use pastLastLeaf() as we've done elsewhere.
Introducing a new traversal code like this is very likely going to result in a hang or a crash.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:574
> +        auto* element = downcast<StyledElement>(node.get());
> +        auto* inlineStyle = element->inlineStyle();

Please don't use raw pointers.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:578
> +        // Abort if the text color is too dark.

Ditto.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:583
> +        // Abort if the background color is too light.

Ditto.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:597
> +        StyledElement* element = downcast<StyledElement>(node.get());

Please don't use raw pointers.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:599
> +        auto* inlineStyle = element->inlineStyle();

Ditto.

> Source/WebCore/editing/ReplaceSelectionCommand.h:85
> +        Node* pastLastSibling() const

Why do we need to add this?

> LayoutTests/editing/pasteboard/paste-dark-mode-color-filtered.html:41
> +        document.execCommand("SelectAll", false);
> +        document.execCommand("Copy", false);
> +
> +        copyNode.remove();

This will not test cross origin / cross app sanitization code path.
We should probably add an equivalent API test.

[Timothy Hatcher]

(In reply to Ryosuke Niwa from comment #3)
> Comment on attachment 377773 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=377773&action=review
> 
> Are we trying to detect the dark mode content? That doesn't sound right.
> The attributed string itself should contain that information.

Attributed strings and the HTML pasteboard data does not contain any information on dark or light appearance of the original content. So unfortunately, we have to detect based on the color information. This is an area I plan to bring up with the AppKit/UIKit team.

[Timothy Hatcher]

Comment on attachment 377773 [details]
Patch

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

>> Source/WebCore/editing/ReplaceSelectionCommand.cpp:530
>> +    ContainerNode* context = insertedNodes.firstNodeInserted()->parentNode();
> 
> This is not safe. updateLayoutIgnorePendingStylesheets can delete this node. Need to use RefPtr.
> Also, why is it okay to not do this work when we're inserting into a list item?
> 
> This work probably needs to be done right before we start inserting contents where hasBlankLinesBetweenParagraphs, etc... is computed.

I'll update this to use RefPtr.

I don't understand why this needs to happen before hasBlankLinesBetweenParagraphs. Are you suggesting modifying the fragment? Nothing else seems to do that.

Pasting into a list as list items works as expected with this code. I'll add a test to show it.

>> Source/WebCore/editing/ReplaceSelectionCommand.cpp:537
>> +    if (!contextRenderer || !contextRenderer->style().hasAppleColorFilter())
> 
> Why don't we just check the style of the root editable element?
> Realistically speaking, applying color filter to some part of the editable region isn't gonna work anyway.

This matches other code we have in EditingStyle::inverseTransformColorIfNeeded(). A color filter can be applied to any element, undoing filtering or applying other filters. It is best to check the most relevant element for the correct color filter. It is also inherited.

>> Source/WebCore/editing/ReplaceSelectionCommand.cpp:541
>> +    // FilterOperation that implements inverseTransformColor().
> 
> This is obvious from the code. I don't think we need this comment.

Sure.

>> Source/WebCore/editing/ReplaceSelectionCommand.cpp:564
>> +    // dark text or light background colors, abort before the transformation step.
> 
> I don't think we need this comment. It's self-evident from the code.

Sure.

>> Source/WebCore/editing/ReplaceSelectionCommand.cpp:565
>> +    RefPtr<Node> pastEndNode = insertedNodes.pastLastSibling();
> 
> Please use pastLastLeaf() as we've done elsewhere.
> Introducing a new traversal code like this is very likely going to result in a hang or a crash.

I use NodeTraversal::nextSkippingChildren() in this loop, not NodeTraversal::next(), so I needed a matching past-end node.

>> Source/WebCore/editing/ReplaceSelectionCommand.cpp:574
>> +        auto* inlineStyle = element->inlineStyle();
> 
> Please don't use raw pointers.

I'll fix StyledElement, but the inlineStyle pointer is unavoidable. No?

>> Source/WebCore/editing/ReplaceSelectionCommand.cpp:597
>> +        StyledElement* element = downcast<StyledElement>(node.get());
> 
> Please don't use raw pointers.

See above.

>> Source/WebCore/editing/ReplaceSelectionCommand.cpp:599
>> +        auto* inlineStyle = element->inlineStyle();
> 
> Ditto.

See above.

>> Source/WebCore/editing/ReplaceSelectionCommand.h:85
>> +        Node* pastLastSibling() const
> 
> Why do we need to add this?

See above.

>> LayoutTests/editing/pasteboard/paste-dark-mode-color-filtered.html:41
>> +        copyNode.remove();
> 
> This will not test cross origin / cross app sanitization code path.
> We should probably add an equivalent API test.

If my change touched these areas, sure. Otherwise is that really needed?

[Timothy Hatcher]

Comment on attachment 377773 [details]
Patch

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

>>> Source/WebCore/editing/ReplaceSelectionCommand.cpp:565
>>> +    RefPtr<Node> pastEndNode = insertedNodes.pastLastSibling();
>> 
>> Please use pastLastLeaf() as we've done elsewhere.
>> Introducing a new traversal code like this is very likely going to result in a hang or a crash.
> 
> I use NodeTraversal::nextSkippingChildren() in this loop, not NodeTraversal::next(), so I needed a matching past-end node.

After thinking about it more, I'm just going to use NodeTraversal::next() and pastLastLeaf() for simplicity and consistency.

[Ryosuke Niwa]

(In reply to Timothy Hatcher from comment #5)
> Comment on attachment 377773 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=377773&action=review
> 
> >> Source/WebCore/editing/ReplaceSelectionCommand.cpp:530
> >> +    ContainerNode* context = insertedNodes.firstNodeInserted()->parentNode();
> > 
> > This is not safe. updateLayoutIgnorePendingStylesheets can delete this node. Need to use RefPtr.
> > Also, why is it okay to not do this work when we're inserting into a list item?
> > 
> > This work probably needs to be done right before we start inserting contents where hasBlankLinesBetweenParagraphs, etc... is computed.
> 
> I'll update this to use RefPtr.
> 
> I don't understand why this needs to happen before
> hasBlankLinesBetweenParagraphs. Are you suggesting modifying the fragment?
> Nothing else seems to do that.

No, I mean that you'd have to determine whether color filter is used or not prior to the fragment being inserted. The thing about editing code is that we need to be always mindful of scripts in the document synchronously modifying things. It's not safe to assume the container means what we think it means once nodes are inserted.

> Pasting into a list as list items works as expected with this code. I'll add
> a test to show it.

Okay.

> >> Source/WebCore/editing/ReplaceSelectionCommand.cpp:537
> >> +    if (!contextRenderer || !contextRenderer->style().hasAppleColorFilter())
> > 
> > Why don't we just check the style of the root editable element?
> > Realistically speaking, applying color filter to some part of the editable region isn't gonna work anyway.
> 
> This matches other code we have in
> EditingStyle::inverseTransformColorIfNeeded(). A color filter can be applied
> to any element, undoing filtering or applying other filters. It is best to
> check the most relevant element for the correct color filter. It is also
> inherited.

I disagree. What happens if only a part of the selection was inside the inverted color and the part was not inverted? That just makes no sense. To make matters worse, as you insert more content, CSS selectors that depend on sibling combinator, etc... could change whether a given node has a filter or not. So it's not really a fruitful endeavor. 

> >> Source/WebCore/editing/ReplaceSelectionCommand.cpp:565
> >> +    RefPtr<Node> pastEndNode = insertedNodes.pastLastSibling();
> > 
> > Please use pastLastLeaf() as we've done elsewhere.
> > Introducing a new traversal code like this is very likely going to result in a hang or a crash.
> 
> I use NodeTraversal::nextSkippingChildren() in this loop, not
> NodeTraversal::next(), so I needed a matching past-end node.

That's likely wrong. We can't skip leaf nodes in editing because leaf nodes are often ones that have useful styling infomation.

> >> Source/WebCore/editing/ReplaceSelectionCommand.cpp:574
> >> +        auto* inlineStyle = element->inlineStyle();
> > 
> > Please don't use raw pointers.
> 
> I'll fix StyledElement, but the inlineStyle pointer is unavoidable. No?

StyleProperties is ref counted you can do: makeRefPtr(element->inlineStyle()).

> >> LayoutTests/editing/pasteboard/paste-dark-mode-color-filtered.html:41
> >> +        copyNode.remove();
> > 
> > This will not test cross origin / cross app sanitization code path.
> > We should probably add an equivalent API test.
> 
> If my change touched these areas, sure. Otherwise is that really needed?

Yes. Whenever given this bug is about coping from Xcode / Terminal, you'd have to test cross-origin / cross-app sanitization code path. Otherwise, we're just testing the code path of copying & pasting within Mail.

[Timothy Hatcher]

Comment on attachment 377773 [details]
Patch

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

>>>> Source/WebCore/editing/ReplaceSelectionCommand.cpp:530
>>>> +    ContainerNode* context = insertedNodes.firstNodeInserted()->parentNode();
>>> 
>>> This is not safe. updateLayoutIgnorePendingStylesheets can delete this node. Need to use RefPtr.
>>> Also, why is it okay to not do this work when we're inserting into a list item?
>>> 
>>> This work probably needs to be done right before we start inserting contents where hasBlankLinesBetweenParagraphs, etc... is computed.
>> 
>> I'll update this to use RefPtr.
>> 
>> I don't understand why this needs to happen before hasBlankLinesBetweenParagraphs. Are you suggesting modifying the fragment? Nothing else seems to do that.
>> 
>> Pasting into a list as list items works as expected with this code. I'll add a test to show it.
> 
> No, I mean that you'd have to determine whether color filter is used or not prior to the fragment being inserted. The thing about editing code is that we need to be always mindful of scripts in the document synchronously modifying things. It's not safe to assume the container means what we think it means once nodes are inserted.

I see, will change.

>>>> Source/WebCore/editing/ReplaceSelectionCommand.cpp:574
>>>> +        auto* inlineStyle = element->inlineStyle();
>>> 
>>> Please don't use raw pointers.
>> 
>> I'll fix StyledElement, but the inlineStyle pointer is unavoidable. No?
> 
> StyleProperties is ref counted you can do: makeRefPtr(element->inlineStyle()).

No existing code does this when accessing inlineStyle(). inlineStyle() returns const StyleProperties*. Using makeRefPtr() with a const is a build failure.

[Ryosuke Niwa]

(In reply to Timothy Hatcher from comment #8)
> Comment on attachment 377773 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=377773&action=review
>
> >>>> Source/WebCore/editing/ReplaceSelectionCommand.cpp:574
> >>>> +        auto* inlineStyle = element->inlineStyle();
> >>> 
> >>> Please don't use raw pointers.
> >> 
> >> I'll fix StyledElement, but the inlineStyle pointer is unavoidable. No?
> > 
> > StyleProperties is ref counted you can do: makeRefPtr(element->inlineStyle()).
> 
> No existing code does this when accessing inlineStyle(). inlineStyle()
> returns const StyleProperties*. Using makeRefPtr() with a const is a build
> failure.

Ugh... we should probably fix that but not in this patch I guess. We'd need to be very careful about when we call inlineStyle() then.

[Timothy Hatcher]

Created attachment 378001 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 378001 [details]
Patch

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

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:548
> +        if (auto color = inlineStyle.propertyAsColor(propertyID)) {
> +            if (color.value().isVisible() && !color.value().isSemantic()) {

Can we use an early return instead of nested if's?

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:563
> +        next = NodeTraversal::next(*node);

Since we're not modifying code there is no need to compute next early.
Just do: for (RefPtr<Node> node = fragment.firstChild(); node; node = NodeTraversal::next(*node))

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:573
> +        // Abort if the text color is too dark.

I don't think this comment is necessary. It's pretty self-evident from the code.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:575
> +        if (textLightness && *textLightness < textLightnessThreshold)

textLightnessThreshold doesn't quite tell us what kind threshold it is.
How about lightnessDarkEnoughForText?

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:578
> +        // Abort if the background color is too light.

Ditto.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:580
> +        if (backgroundLightness && *backgroundLightness > backgroundLightnessThreshold)

How about lightnessLightEnoughForBackground?

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:592
> +    for (RefPtr<Node> node = insertedNodes.firstNodeInserted(); node && node != pastEndNode; node = next) {
> +        next = NodeTraversal::next(*node);

Ditto about traversal. Updating the attribute doesn't necessitate storing the next node.
If sync DOM mutation were to occur (I don't think it could due to event scope),
then such a script would be mutated a node further down in the traversal
so it's not really meaningful to get just the next immediate node for traversal regardless.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/PasteHTML.mm:389
> +    [webView setAppearance:[NSAppearance appearanceNamed:NSAppearanceNameDarkAqua]];

Maybe we can add a helper function instead of repeating it twice?

[Timothy Hatcher]

Created attachment 378009 [details]
Patch

[Timothy Hatcher]

Created attachment 378015 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 378015 [details]
Patch

Clearing flags on attachment: 378015

Committed r249517: <https://trac.webkit.org/changeset/249517>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.