Bug 236546 – Refactor logic for writing selected text in image overlays to the pasteboard

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

[Wenson Hsieh]

rdar://83173693

[Wenson Hsieh]

Created attachment 451848 [details]
Depends on #236535

[Wenson Hsieh]

Created attachment 451944 [details]
Rebase on trunk

[Megan Gardner]

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.

[Wenson Hsieh]

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?)

[Devin Rousso]

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)

[Wenson Hsieh]

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.

[Wenson Hsieh]

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)

[EWS]

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