Bug 201368 - Mail appears to be double inverting code copied from Notes, Xcode, or Terminal
Summary: Mail appears to be double inverting code copied from Notes, Xcode, or Terminal
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Timothy Hatcher
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-08-30 17:08 PDT by Timothy Hatcher
Modified: 2019-09-04 19:52 PDT (History)
5 users (show)

See Also:


Attachments
Patch (20.12 KB, patch)
2019-08-30 17:11 PDT, Timothy Hatcher
no flags Details | Formatted Diff | Diff
Patch (36.31 KB, patch)
2019-09-04 12:47 PDT, Timothy Hatcher
no flags Details | Formatted Diff | Diff
Patch (37.31 KB, patch)
2019-09-04 13:31 PDT, Timothy Hatcher
no flags Details | Formatted Diff | Diff
Patch (37.27 KB, patch)
2019-09-04 14:27 PDT, Timothy Hatcher
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Timothy Hatcher 2019-08-30 17:08:20 PDT
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>
Comment 1 Timothy Hatcher 2019-08-30 17:11:58 PDT Comment hidden (obsolete)
Comment 2 Timothy Hatcher 2019-09-03 08:16:03 PDT Comment hidden (obsolete)
Comment 3 Ryosuke Niwa 2019-09-03 14:19:57 PDT
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.
Comment 4 Timothy Hatcher 2019-09-03 17:56:33 PDT
(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.
Comment 5 Timothy Hatcher 2019-09-03 18:08:47 PDT
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?
Comment 6 Timothy Hatcher 2019-09-03 19:59:04 PDT
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.
Comment 7 Ryosuke Niwa 2019-09-03 22:08:07 PDT
(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.
Comment 8 Timothy Hatcher 2019-09-04 08:52:48 PDT
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.
Comment 9 Ryosuke Niwa 2019-09-04 08:57:21 PDT
(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.
Comment 10 Timothy Hatcher 2019-09-04 12:47:41 PDT Comment hidden (obsolete)
Comment 11 Ryosuke Niwa 2019-09-04 13:10:56 PDT
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?
Comment 12 Timothy Hatcher 2019-09-04 13:31:02 PDT Comment hidden (obsolete)
Comment 13 Timothy Hatcher 2019-09-04 14:27:25 PDT
Created attachment 378015 [details]
Patch
Comment 14 WebKit Commit Bot 2019-09-04 19:52:46 PDT
Comment on attachment 378015 [details]
Patch

Clearing flags on attachment: 378015

Committed r249517: <https://trac.webkit.org/changeset/249517>
Comment 15 WebKit Commit Bot 2019-09-04 19:52:48 PDT
All reviewed patches have been landed.  Closing bug.