rdar://83173693
Created attachment 451848 [details] Depends on #236535
Created attachment 451944 [details] Rebase on trunk
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 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 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 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 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)
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].