Bug 236546 - Refactor logic for writing selected text in image overlays to the pasteboard
Summary: Refactor logic for writing selected text in image overlays to the pasteboard
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar
Depends on: 236535
Blocks:
  Show dependency treegraph
 
Reported: 2022-02-12 17:20 PST by Wenson Hsieh
Modified: 2022-02-15 12:48 PST (History)
12 users (show)

See Also:


Attachments
Depends on #236535 (11.18 KB, patch)
2022-02-13 17:22 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Rebase on trunk (11.19 KB, patch)
2022-02-14 14:21 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Wenson Hsieh 2022-02-12 17:20:42 PST
rdar://83173693
Comment 1 Wenson Hsieh 2022-02-13 17:22:00 PST Comment hidden (obsolete)
Comment 2 Wenson Hsieh 2022-02-14 14:21:49 PST
Created attachment 451944 [details]
Rebase on trunk
Comment 3 Megan Gardner 2022-02-15 11:52:29 PST
Comment on attachment 451944 [details]
Rebase on trunk

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

> Source/WebCore/editing/Editor.cpp:1450
> +        if (action == CopyAction && !isSelectionInImageOverlay)

I'm a bit confused, because this seems like it used to work for selections in overlays, but now it won't? The logic just isn't lining up for me.
Comment 4 Wenson Hsieh 2022-02-15 11:55:16 PST
Comment on attachment 451944 [details]
Rebase on trunk

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

Thanks for the review!

>> Source/WebCore/editing/Editor.cpp:1450
>> +        if (action == CopyAction && !isSelectionInImageOverlay)
> 
> I'm a bit confused, because this seems like it used to work for selections in overlays, but now it won't? The logic just isn't lining up for me.

So before, if the selection was in an image overlay, we'd take the plain text writing codepath above; however, this patch removes the "image overlay" part of the check.

```
if (enclosingTextFormControl(m_document.selection().selection().start()) || (selection && ImageOverlay::isInsideOverlay(*selection)))
```

…so this adjustment ensures that we go down the `writeSelectionToPasteboard` codepath instead of the `writeImageToPasteboard` codepath when Live Text is selected inside of an image document (hopefully that clarifies things?)
Comment 5 Devin Rousso 2022-02-15 12:15:43 PST
Comment on attachment 451944 [details]
Rebase on trunk

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

r=me as well :)

> Source/WebCore/dom/ImageOverlay.cpp:132
> +    std::optional<SimpleRange> imageOverlayRange;

NIT: rather than bother with a `std::optional` you could just move the `return` inside the `for`

> Source/WebCore/dom/ImageOverlay.cpp:133
> +    for (auto& ancestor : ancestorsOfType<HTMLDivElement>(*selection.start().containerNode())) {

Do we need to check `containerNode()` before dereferencing?

> Source/WebCore/editing/cocoa/EditorCocoa.mm:110
> +    auto* page = selection.document()->page();

Do we need to check `document()` before dereferencing?

> Source/WebCore/editing/cocoa/EditorCocoa.mm:114
> +    RefPtr hostElement = dynamicDowncast<HTMLElement>(selection.start().containerNode()->shadowHost());

ditto (Source/WebCore/dom/ImageOverlay.cpp:133)
Comment 6 Wenson Hsieh 2022-02-15 12:21:46 PST
Comment on attachment 451944 [details]
Rebase on trunk

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

>> Source/WebCore/dom/ImageOverlay.cpp:132
>> +    std::optional<SimpleRange> imageOverlayRange;
> 
> NIT: rather than bother with a `std::optional` you could just move the `return` inside the `for`

I don't think we can, since we're using element iterators and `resolveCharacterLocation`/`characterRange` will trigger layout, so moving the `return` inside the `for` will release assert.

>> Source/WebCore/dom/ImageOverlay.cpp:133
>> +    for (auto& ancestor : ancestorsOfType<HTMLDivElement>(*selection.start().containerNode())) {
> 
> Do we need to check `containerNode()` before dereferencing?

It shouldn't be necessary here, since we otherwise would've bailed in the `!isInsideOverlay()` check above.
Comment 7 Wenson Hsieh 2022-02-15 12:24:53 PST
Comment on attachment 451944 [details]
Rebase on trunk

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

>> Source/WebCore/editing/cocoa/EditorCocoa.mm:110
>> +    auto* page = selection.document()->page();
> 
> Do we need to check `document()` before dereferencing?

I think we can assume `document()` is non null here, since we should only get here if `isInsideOverlay` is true.

>> Source/WebCore/editing/cocoa/EditorCocoa.mm:114
>> +    RefPtr hostElement = dynamicDowncast<HTMLElement>(selection.start().containerNode()->shadowHost());
> 
> ditto (Source/WebCore/dom/ImageOverlay.cpp:133)

(ditto)
Comment 8 EWS 2022-02-15 12:48:46 PST
Committed r289839 (247287@main): <https://commits.webkit.org/247287@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 451944 [details].