Bug 236842 - [Live Text] Add a mechanism to inject images into image overlays
Summary: [Live Text] Add a mechanism to inject images into image overlays
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar
Depends on:
Blocks: 236845
  Show dependency tree / graph
 
Reported: 2022-02-18 09:23 PST by Wenson Hsieh
Modified: 2022-02-18 15:52 PST (History)
12 users (show)

See Also:

Attachments
For EWS (24.70 KB, patch)
2022-02-18 10:06 PST, Wenson Hsieh 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Fix watchOS build (24.72 KB, patch)
2022-02-18 10:11 PST, Wenson Hsieh 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Fix Big Sur build (24.63 KB, patch)
2022-02-18 10:18 PST, Wenson Hsieh 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Fix non-internal iOS build (24.65 KB, patch)
2022-02-18 10:35 PST, Wenson Hsieh 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Fix watchOS build (again) (24.92 KB, patch)
2022-02-18 10:51 PST, Wenson Hsieh 		no flags Details | Formatted Diff | Diff
Patch for landing (25.02 KB, patch)
2022-02-18 13:00 PST, Wenson Hsieh 		no flags Details | Formatted Diff | Diff
Patch for landing (26.42 KB, patch)
2022-02-18 13:44 PST, Wenson Hsieh 		no flags Details | Formatted Diff | Diff
Show Obsolete (6) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Wenson Hsieh 2022-02-18 09:23:18 PST 
.
Comment 1 Wenson Hsieh 2022-02-18 10:06:54 PST Comment hidden (obsolete)
Comment 2 Wenson Hsieh 2022-02-18 10:11:47 PST Comment hidden (obsolete)
Comment 3 Wenson Hsieh 2022-02-18 10:18:48 PST Comment hidden (obsolete)
Comment 4 Wenson Hsieh 2022-02-18 10:35:04 PST Comment hidden (obsolete)
Comment 5 Wenson Hsieh 2022-02-18 10:51:18 PST 
Created attachment 452545 [details]
Fix watchOS build (again)
Comment 6 Aditya Keerthi 2022-02-18 12:18:01 PST 
Comment on attachment 452545 [details]
Fix watchOS build (again)

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

> Source/WebCore/dom/ImageOverlay.cpp:227
> +static void installStyleSheet(ShadowRoot& shadowRoot)

Consider renaming to `installImageOverlayStyleSheet`.

> Source/WebCore/dom/ImageOverlay.cpp:679
> +    RefPtr imageOverlayRoot = dynamicDowncast<HTMLDivElement>(static_cast<TreeScope&>(shadowRoot.get()).getElementById(imageOverlayElementIdentifier()));

Is the static cast to `TreeScope` necessary?

> Source/WebCore/dom/ImageOverlay.h:28
> +#include "FloatRect.h"

Can this be forward declared?
Comment 7 Wenson Hsieh 2022-02-18 12:47:48 PST 
Comment on attachment 452545 [details]
Fix watchOS build (again)

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

Thanks for the review!

>> Source/WebCore/dom/ImageOverlay.cpp:227
>> +static void installStyleSheet(ShadowRoot& shadowRoot)
> 
> Consider renaming to `installImageOverlayStyleSheet`.

Makes sense! Renamed.

>> Source/WebCore/dom/ImageOverlay.cpp:679
>> +    RefPtr imageOverlayRoot = dynamicDowncast<HTMLDivElement>(static_cast<TreeScope&>(shadowRoot.get()).getElementById(imageOverlayElementIdentifier()));
> 
> Is the static cast to `TreeScope` necessary?

That's a good question…the issue is that `getElementById(const AtomString&)` exists on both TreeScope and DocumentFragment, so the compiler doesn't know which one to pick :/

I tried to work around this by initializing a String and using the `const String&` version, but that didn't seem to alleviate the issue from above. I think I'll keep it as is (though, maybe there's a cleaner way to avoid this?)

>> Source/WebCore/dom/ImageOverlay.h:28
>> +#include "FloatRect.h"
> 
> Can this be forward declared?

Good catch — fixed!
Comment 8 Wenson Hsieh 2022-02-18 13:00:43 PST 
Created attachment 452567 [details]
Patch for landing
Comment 9 Aditya Keerthi 2022-02-18 13:19:56 PST 
(In reply to Wenson Hsieh from comment #7)
> Comment on attachment 452545 [details]
> Fix watchOS build (again)
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=452545&action=review
> 
> Thanks for the review!
> 
> >> Source/WebCore/dom/ImageOverlay.cpp:227
> >> +static void installStyleSheet(ShadowRoot& shadowRoot)
> > 
> > Consider renaming to `installImageOverlayStyleSheet`.
> 
> Makes sense! Renamed.
> 
> >> Source/WebCore/dom/ImageOverlay.cpp:679
> >> +    RefPtr imageOverlayRoot = dynamicDowncast<HTMLDivElement>(static_cast<TreeScope&>(shadowRoot.get()).getElementById(imageOverlayElementIdentifier()));
> > 
> > Is the static cast to `TreeScope` necessary?
> 
> That's a good question…the issue is that `getElementById(const AtomString&)`
> exists on both TreeScope and DocumentFragment, so the compiler doesn't know
> which one to pick :/
> 
> I tried to work around this by initializing a String and using the `const
> String&` version, but that didn't seem to alleviate the issue from above. I
> think I'll keep it as is (though, maybe there's a cleaner way to avoid this?)

I think we can solve this by adding `using TreeScope::getElementById;` in `ShadowRoot.h`.

Note that the `DocumentFragment` implementation already calls into the `TreeScope` implementation for shadow roots.
 
> >> Source/WebCore/dom/ImageOverlay.h:28
> >> +#include "FloatRect.h"
> > 
> > Can this be forward declared?
> 
> Good catch — fixed!
Comment 10 Wenson Hsieh 2022-02-18 13:39:16 PST 
Comment on attachment 452545 [details]
Fix watchOS build (again)

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

>>>> Source/WebCore/dom/ImageOverlay.cpp:227
>>>> +static void installStyleSheet(ShadowRoot& shadowRoot)
>>> 
>>> Consider renaming to `installImageOverlayStyleSheet`.
>> 
>> Makes sense! Renamed.
> 
> I think we can solve this by adding `using TreeScope::getElementById;` in `ShadowRoot.h`.
> 
> Note that the `DocumentFragment` implementation already calls into the `TreeScope` implementation for shadow roots.

Thanks! That works — and it also looks like we do something similar already for `rootNode()` on ShadowRoot, so there's precedent.

I'll add that, and replace all the static_casts to TreeScope here.
Comment 11 Wenson Hsieh 2022-02-18 13:44:41 PST 
Created attachment 452577 [details]
Patch for landing
Comment 12 EWS 2022-02-18 15:51:49 PST 
Committed r290178 (247508@main): <https://commits.webkit.org/247508@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 452577 [details].
Comment 13 Radar WebKit Bug Importer 2022-02-18 15:52:23 PST 
<rdar://problem/89169696>