Live Text text selection in image goes offscreen in Mail #13865

rr-codes · 2023-05-14T22:31:05Z

`9477772`

Live Text text selection in image goes offscreen in Mail
https://bugs.webkit.org/show_bug.cgi?id=256772
rdar://108597859

Reviewed by Wenson Hsieh.

In `ImageOverlay::updateWithTextRecognitionResult`, the `sizeBeforeTransform` is empty in the case
of selecting text in an image in Mail. This is because the renderer returned by `textContainer->renderBoxModelObject()`
is `nullptr`, so `sizeBeforeTransform` is never set.

This renderer is null because it is never created, because `shouldCreateRenderer` returns `false`,
ultimately because `RenderImage::hasShadowContent` returns `false`. This method returns `false`
because `m_hasImageOverlay` is `false`.

In the constructor of `RenderImage`, `m_hasImageOverlay` is initialized to the result of
`ImageOverlay::hasOverlay`. _This_ method returns `false` because the shadow root has no element
who's id is that of `imageOverlayElementIdentifier`.

`updateSubtree` in `ImageOverlay` is what is responsible for setting the id of the root shadow
element to `imageOverlayElementIdentifier`.

In the case of Safari, `updateSubtree` is called first, and then the constructor of `RenderImage`
is called, which ensures that the renderer is ultimately created. However, in Mail, the `RenderImage`
constructor is called first, and so at that time there's no `imageOverlayElementIdentifier` element,
which leads to the renderer never being created.

This PR fixes this by ensuring that `RenderImage::hasShadowContent` returns `true` if the element
has a shadow root in `updateSubtree`, by setting `RenderImage::setHasImageOverlay`.

* LayoutTests/fast/images/text-recognition/image-overlay-with-attachment-element-expected.txt: Added.
* LayoutTests/fast/images/text-recognition/image-overlay-with-attachment-element.html: Added.
* Source/WebCore/dom/ImageOverlay.cpp:
(WebCore::ImageOverlay::updateSubtree):

Canonical link: https://commits.webkit.org/264156@main

14221b0

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🛠 gtk
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2-stress
✅ 🛠 🧪 merge	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2023-05-14T22:31:17Z

EWS run on previous version of this PR (hash e5915a7)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🛠 gtk
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

rr-codes · 2023-05-14T22:39:05Z

Source/WebCore/dom/ImageOverlay.cpp

@@ -280,6 +280,9 @@ static Elements updateSubtree(HTMLElement& element, const TextRecognitionResult&
 #endif // ENABLE(MODERN_MEDIA_CONTROLS)

    if (RefPtr shadowRoot = element.shadowRoot()) {
+        if (auto* renderer = dynamicDowncast<RenderImage>(element.renderer()))
+            renderer->setHasImageOverlay();


I'm basically just co-opting "hasImageOverlay" to mean "has shadow content", since that's what it effectively means, and I figured this was preferable to introducing a new method / member variable to RenderImage

whsieh · 2023-05-14T23:25:24Z

Is it possible to write a test for this?

rr-codes · 2023-05-14T23:29:32Z

Is it possible to write a test for this?

I tried to, but I wasn't sure how to artificially get it into the same state as Mail such that the RenderImage constructor is called before updateSubtree.

rr-codes · 2023-05-14T23:35:22Z

Is it possible to write a test for this?

I tried to, but I wasn't sure how to artificially get it into the same state as Mail such that the RenderImage constructor is called before updateSubtree.

oh maybe I can replicate it if I use < attachment>

whsieh · 2023-05-14T23:37:53Z

oh maybe I can replicate it if I use < attachment>

Yeah, makes sense — HTMLAttachmentElement.getAttachmentIdentifier() is specific to Mail/Notes, and would cause the image element to gain a shadow root before Live Text code is triggered, which might in turn explain why Live Text code ends up getting confused in Mail and not Safari.

webkit-early-warning-system · 2023-05-16T16:40:25Z

EWS run on previous version of this PR (hash 2e8b50a)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	~~🛠 wpe~~	~~🛠 wincairo~~
~~🧪 bindings~~	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	~~🧪 wpe-wk2~~
~~🧪 webkitperl~~	~~🧪 ios-wk2~~	~~🧪 api-mac~~	~~🛠 gtk~~
	~~🧪 ios-wk2-wpt~~	~~🧪 mac-wk1~~	~~🧪 gtk-wk2~~
	⏳ 🧪 api-ios	~~🧪 mac-wk2~~	~~🧪 api-gtk~~
	✅ 🛠 tv	~~🧪 mac-AS-debug-wk2~~
	✅ 🛠 tv-sim	~~🧪 mac-wk2-stress~~
	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2023-05-16T16:43:00Z

EWS run on previous version of this PR (hash 19fd020)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🛠 gtk
	⏳ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	❌ 🧪 gtk-wk2
	⏳ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2-stress
	✅ 🛠 watch
	✅ 🛠 watch-sim

LayoutTests/fast/images/text-recognition/image-overlay-with-attachment-element.html

webkit-early-warning-system · 2023-05-16T20:16:35Z

EWS run on current version of this PR (hash 14221b0)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🛠 gtk
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2-stress
✅ 🛠 🧪 merge	✅ 🛠 watch
	✅ 🛠 watch-sim

https://bugs.webkit.org/show_bug.cgi?id=256772 rdar://108597859 Reviewed by Wenson Hsieh. In `ImageOverlay::updateWithTextRecognitionResult`, the `sizeBeforeTransform` is empty in the case of selecting text in an image in Mail. This is because the renderer returned by `textContainer->renderBoxModelObject()` is `nullptr`, so `sizeBeforeTransform` is never set. This renderer is null because it is never created, because `shouldCreateRenderer` returns `false`, ultimately because `RenderImage::hasShadowContent` returns `false`. This method returns `false` because `m_hasImageOverlay` is `false`. In the constructor of `RenderImage`, `m_hasImageOverlay` is initialized to the result of `ImageOverlay::hasOverlay`. _This_ method returns `false` because the shadow root has no element who's id is that of `imageOverlayElementIdentifier`. `updateSubtree` in `ImageOverlay` is what is responsible for setting the id of the root shadow element to `imageOverlayElementIdentifier`. In the case of Safari, `updateSubtree` is called first, and then the constructor of `RenderImage` is called, which ensures that the renderer is ultimately created. However, in Mail, the `RenderImage` constructor is called first, and so at that time there's no `imageOverlayElementIdentifier` element, which leads to the renderer never being created. This PR fixes this by ensuring that `RenderImage::hasShadowContent` returns `true` if the element has a shadow root in `updateSubtree`, by setting `RenderImage::setHasImageOverlay`. * LayoutTests/fast/images/text-recognition/image-overlay-with-attachment-element-expected.txt: Added. * LayoutTests/fast/images/text-recognition/image-overlay-with-attachment-element.html: Added. * Source/WebCore/dom/ImageOverlay.cpp: (WebCore::ImageOverlay::updateSubtree): Canonical link: https://commits.webkit.org/264156@main

webkit-commit-queue · 2023-05-17T17:13:39Z

Committed 264156@main (9477772): https://commits.webkit.org/264156@main

Reviewed commits have been landed. Closing PR #13865 and removing active labels.

rr-codes requested review from rniwa and cdumez as code owners May 14, 2023 22:31

rr-codes self-assigned this May 14, 2023

rr-codes added the HTML Editing For bugs in HTML editing support (including designMode and contentEditable). label May 14, 2023

rr-codes requested a review from whsieh May 14, 2023 22:31

rr-codes commented May 14, 2023

View reviewed changes

rr-codes force-pushed the eng/108597859 branch from e5915a7 to 2e8b50a Compare May 16, 2023 16:40

rr-codes force-pushed the eng/108597859 branch from 2e8b50a to 19fd020 Compare May 16, 2023 16:42

whsieh approved these changes May 16, 2023

View reviewed changes

LayoutTests/fast/images/text-recognition/image-overlay-with-attachment-element.html Outdated Show resolved Hide resolved

LayoutTests/fast/images/text-recognition/image-overlay-with-attachment-element.html Outdated Show resolved Hide resolved

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 16, 2023

rr-codes removed the merging-blocked Applied to prevent a change from being merged label May 16, 2023

rr-codes force-pushed the eng/108597859 branch from 19fd020 to 14221b0 Compare May 16, 2023 20:15

rr-codes added the merge-queue Applied to send a pull request to merge-queue label May 17, 2023

webkit-commit-queue force-pushed the eng/108597859 branch from 14221b0 to 9477772 Compare May 17, 2023 17:13

webkit-commit-queue merged commit 9477772 into WebKit:main May 17, 2023

webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label May 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Live Text text selection in image goes offscreen in Mail #13865

Live Text text selection in image goes offscreen in Mail #13865

rr-codes commented May 14, 2023 •

edited by webkit-early-warning-system

webkit-early-warning-system commented May 14, 2023 •

edited

rr-codes May 14, 2023

whsieh commented May 14, 2023

rr-codes commented May 14, 2023

rr-codes commented May 14, 2023 •

edited

whsieh commented May 14, 2023

webkit-early-warning-system commented May 16, 2023 •

edited

webkit-early-warning-system commented May 16, 2023 •

edited

webkit-early-warning-system commented May 16, 2023 •

edited

webkit-commit-queue commented May 17, 2023

Live Text text selection in image goes offscreen in Mail #13865

Live Text text selection in image goes offscreen in Mail #13865

Conversation

rr-codes commented May 14, 2023 • edited by webkit-early-warning-system

9477772

webkit-early-warning-system commented May 14, 2023 • edited

rr-codes May 14, 2023

Choose a reason for hiding this comment

whsieh commented May 14, 2023

rr-codes commented May 14, 2023

rr-codes commented May 14, 2023 • edited

whsieh commented May 14, 2023

webkit-early-warning-system commented May 16, 2023 • edited

webkit-early-warning-system commented May 16, 2023 • edited

webkit-early-warning-system commented May 16, 2023 • edited

webkit-commit-queue commented May 17, 2023

rr-codes commented May 14, 2023 •

edited by webkit-early-warning-system

`9477772`

webkit-early-warning-system commented May 14, 2023 •

edited

rr-codes commented May 14, 2023 •

edited

webkit-early-warning-system commented May 16, 2023 •

edited

webkit-early-warning-system commented May 16, 2023 •

edited

webkit-early-warning-system commented May 16, 2023 •

edited