Bug 221236 - Introduce image overlays and add UA shadow root support for image extraction
Summary: Introduce image overlays and add UA shadow root support for image extraction
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:
 
Reported: 2021-02-01 17:40 PST by Wenson Hsieh
Modified: 2021-02-10 14:44 PST (History)
11 users (show)

See Also:


Attachments
WIP (29.53 KB, patch)
2021-02-02 18:07 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff
WIP (28.97 KB, patch)
2021-02-02 18:10 PST, Wenson Hsieh
drousso: review+
Details | Formatted Diff | Diff
Patch (35.92 KB, patch)
2021-02-04 17:01 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Patch (35.32 KB, patch)
2021-02-04 22:33 PST, Wenson Hsieh
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Fix GTK/WPE builds (35.86 KB, patch)
2021-02-04 23:16 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Patch (24.12 KB, patch)
2021-02-05 10:25 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Fix title (24.11 KB, patch)
2021-02-05 10:27 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Move code out of HTMLImageElement (23.95 KB, patch)
2021-02-06 13:16 PST, Wenson Hsieh
rniwa: review+
Details | Formatted Diff | Diff
Patch (23.69 KB, patch)
2021-02-06 17:14 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 2021-02-01 17:40:46 PST
SSIA
Comment 1 Wenson Hsieh 2021-02-02 18:07:49 PST Comment hidden (obsolete)
Comment 2 Wenson Hsieh 2021-02-02 18:10:52 PST
Created attachment 419090 [details]
WIP
Comment 3 Devin Rousso 2021-02-03 14:38:50 PST
Comment on attachment 419090 [details]
WIP

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

r=me

> Source/WebCore/html/shadow/ImageExtractionContentContainer.cpp:41
> +class RenderImageExtractionContentContainer : public RenderBlockFlow {

should the entire class be `final`?

> Source/WebCore/html/shadow/ImageExtractionContentContainer.h:35
> +class HTMLDivElement;

NIT: you've already `#include` this so no need to forward declare again :P

> Source/WebCore/html/shadow/ImageExtractionContentContainer.h:39
> +class ImageExtractionContentContainer : public HTMLDivElement {

should the entire class be `final`?

> Source/WebCore/rendering/RenderImage.cpp:839
> +        LayoutStateMaintainer statePusher(*this, locationOffset(), hasTransform() || hasReflection() || style().isFlippedBlocksWritingMode());

does this need to be hoisted outside the `for` so that it also covers `clearChildNeedsLayout()` like before?
Comment 4 Wenson Hsieh 2021-02-03 15:23:08 PST
Comment on attachment 419090 [details]
WIP

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

I'm going to additionally try to simplify the logic here by avoiding a custom renderer subclass (RenderImageExtractionContentContainer).

>> Source/WebCore/html/shadow/ImageExtractionContentContainer.cpp:41
>> +class RenderImageExtractionContentContainer : public RenderBlockFlow {
> 
> should the entire class be `final`?

Good call. Although, from chatting with Simon and Alan, it looks like I should be able to remove this custom renderer subclass altogether, so I'll try that first.

>> Source/WebCore/html/shadow/ImageExtractionContentContainer.h:35
>> +class HTMLDivElement;
> 
> NIT: you've already `#include` this so no need to forward declare again :P

Whoops, good catch.

>> Source/WebCore/html/shadow/ImageExtractionContentContainer.h:39
>> +class ImageExtractionContentContainer : public HTMLDivElement {
> 
> should the entire class be `final`?

πŸ‘πŸ»

>> Source/WebCore/rendering/RenderImage.cpp:839
>> +        LayoutStateMaintainer statePusher(*this, locationOffset(), hasTransform() || hasReflection() || style().isFlippedBlocksWritingMode());
> 
> does this need to be hoisted outside the `for` so that it also covers `clearChildNeedsLayout()` like before?

From reading the code, it looks like the `LayoutStateMaintainer` just needs to encompass the recursive layout calls on the children, and clearChildNeedsLayout() just clears a few "needs layout" flags, so I *think* this should be okay...
Comment 5 Wenson Hsieh 2021-02-04 15:40:32 PST
(In reply to Wenson Hsieh from comment #4)
> Comment on attachment 419090 [details]
> WIP
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=419090&action=review
> 
> I'm going to additionally try to simplify the logic here by avoiding a
> custom renderer subclass (RenderImageExtractionContentContainer).

Alan helped me figure out how to do this β€” we needed to make a slight adjustment in RenderImage::layoutShadowControls(), so that we set fixed layout dimensions on the child renderers even in the case where `shadowControlsNeedCustomLayoutMetrics()` is `false` (that is, in the case where the renderer is not a RenderMedia).

This method (shadowControlsNeedCustomLayoutMetrics) was added as a part of original support for "image controls", but is currently only used to ensure that the size of media controls (e.g. controls in video elements) matches that of the media element's renderer, since support for "image controls" has been removed.

This means we should be able to remove `shadowControlsNeedCustomLayoutMetrics` altogether, and just always set a fixed width and height in RenderImage::layoutShadowControls(), since both regular images and media (video) elements now require it.
Comment 6 Simon Fraser (smfr) 2021-02-04 15:57:31 PST
How does all this work in the face of object-fit?
Comment 7 Wenson Hsieh 2021-02-04 17:01:00 PST
Created attachment 419342 [details]
Patch
Comment 8 Wenson Hsieh 2021-02-04 17:06:43 PST
(In reply to Simon Fraser (smfr) from comment #6)
> How does all this work in the face of object-fit?

We'll eventually need to transform the normalized quads in the coordinate space of the image into bounding rects in the coordinate space of the image element. In the case of `object-fit: contain;`, this is going to mean applying an additional x or y offset.

This patch doesn't attempt to implement that part yet (it currently just scales each quad's bounds against the size of the container).
Comment 9 Ryosuke Niwa 2021-02-04 20:30:29 PST
Comment on attachment 419342 [details]
Patch

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

> Source/WebCore/dom/Node.h:211
> +#if ENABLE(IMAGE_EXTRACTION)
> +    virtual bool isImageExtractionContentContainer() const { return false; }
> +#endif

Please define this in HTMLElement instead under isTextControlInnerTextElement() and such.
Comment 10 Wenson Hsieh 2021-02-04 22:33:04 PST
Created attachment 419363 [details]
Patch
Comment 11 Wenson Hsieh 2021-02-04 23:16:07 PST
Created attachment 419365 [details]
Fix GTK/WPE builds
Comment 12 Wenson Hsieh 2021-02-04 23:19:07 PST
Comment on attachment 419342 [details]
Patch

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

>> Source/WebCore/dom/Node.h:211
>> +#endif
> 
> Please define this in HTMLElement instead under isTextControlInnerTextElement() and such.

Done! (and also renamed to isImageOverlayElement())
Comment 13 Antti Koivisto 2021-02-05 07:02:07 PST
Comment on attachment 419365 [details]
Fix GTK/WPE builds

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

> Source/WebCore/dom/Element.cpp:4586
> +    auto container = ImageOverlayElement::create(document());
> +    ensureUserAgentShadowRoot().appendChild(container);

Is updateWithImageExtractionResult() guaranteed to be only called once? I don't see any code to update or remove the existing ImageOverlayElement.

> Source/WebCore/html/shadow/ImageOverlayElement.h:39
> +class ImageOverlayElement final : public HTMLDivElement {

Why do we need this subclass? As far as I can see it currently has no data members or unique behaviours in virtual functions. Could you just use HTMLDivElement and put the related functions somewhere else?
Comment 14 Antti Koivisto 2021-02-05 07:18:18 PST
Comment on attachment 419365 [details]
Fix GTK/WPE builds

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

> Source/WebCore/style/UserAgentStyle.cpp:193
> +        else if (!imageOverlayStyleSheet && is<ImageOverlayElement>(element)) {
> +            imageOverlayStyleSheet = parseUASheet(imageOverlayUserAgentStyleSheet, sizeof(imageOverlayUserAgentStyleSheet));
> +            addToDefaultStyle(*imageOverlayStyleSheet);
> +        }

Please don't add anything to default style sheet for this. Instead insert a <style> element in the shadow tree. All selectors will be scoped to that tree so nothing leaks out. See HTMLMeterElement::didAddUserAgentShadowRoot for an example.
Comment 15 Wenson Hsieh 2021-02-05 08:33:32 PST
Comment on attachment 419365 [details]
Fix GTK/WPE builds

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

>> Source/WebCore/dom/Element.cpp:4586
>> +    ensureUserAgentShadowRoot().appendChild(container);
> 
> Is updateWithImageExtractionResult() guaranteed to be only called once? I don't see any code to update or remove the existing ImageOverlayElement.

In its current form, it will only be called once, yes. In the (near) future, we'll need to be able to remove the overlay if (for instance) the image source changes.

>> Source/WebCore/html/shadow/ImageOverlayElement.h:39
>> +class ImageOverlayElement final : public HTMLDivElement {
> 
> Why do we need this subclass? As far as I can see it currently has no data members or unique behaviours in virtual functions. Could you just use HTMLDivElement and put the related functions somewhere else?

I was planning to stash some additional members on `ImageOverlayElement` in a future patch, but it looks like an alternative to that is perhaps keeping a map of some sort on like `Document` or `Page`.

The `ImageOverlayElement` subclass also allows me to (more easily) identify one of these image overlay elements through a type traits check…though I suppose I could just check the class list?

>> Source/WebCore/style/UserAgentStyle.cpp:193
>> +        }
> 
> Please don't add anything to default style sheet for this. Instead insert a <style> element in the shadow tree. All selectors will be scoped to that tree so nothing leaks out. See HTMLMeterElement::didAddUserAgentShadowRoot for an example.

Whoops. Will do!
Comment 16 Wenson Hsieh 2021-02-05 08:46:51 PST
Comment on attachment 419365 [details]
Fix GTK/WPE builds

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

>>> Source/WebCore/style/UserAgentStyle.cpp:193
>>> +        }
>> 
>> Please don't add anything to default style sheet for this. Instead insert a <style> element in the shadow tree. All selectors will be scoped to that tree so nothing leaks out. See HTMLMeterElement::didAddUserAgentShadowRoot for an example.
> 
> Whoops. Will do!

I suppose this also means I'll need to tie this to HTMLImageElement (at least, for the time being?)
Comment 17 Wenson Hsieh 2021-02-05 10:25:06 PST
Created attachment 419425 [details]
Patch
Comment 18 Wenson Hsieh 2021-02-05 10:27:48 PST
Created attachment 419426 [details]
Fix title
Comment 19 Ryosuke Niwa 2021-02-05 21:23:43 PST
Comment on attachment 419426 [details]
Fix title

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

> Source/WebCore/html/HTMLImageElement.cpp:827
> +bool HTMLImageElement::hasOverlay() const
> +{

Don't we need to have this on HTMLPlugInImageElement too so that this will also work on object element?

> Source/WebCore/html/HTMLImageElement.cpp:832
> +    for (auto child = makeRefPtr(shadowRoot->firstChild()); child; child = child->nextSibling()) {

Use childrenOfType<Element>?

> Source/WebCore/html/HTMLImageElement.cpp:878
> +        container->appendChild(child);

It's more efficient to build tree bottom up so do this before appending children to child.

> Source/WebCore/html/HTMLImageElement.cpp:898
> +    if (auto* frame = document().frame())

Please use makeRefPtr here.

> Source/WebCore/html/shadow/imageOverlay.css:25
> +div#image-overlay {

img element is display: inline by default. div is display: block by default.
Don't we want this to be inline-block?
Comment 20 Wenson Hsieh 2021-02-06 12:40:12 PST
Comment on attachment 419426 [details]
Fix title

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

>> Source/WebCore/html/HTMLImageElement.cpp:827
>> +{
> 
> Don't we need to have this on HTMLPlugInImageElement too so that this will also work on object element?

Good point. I will move these helper methods into `HTMLElement` as `HTMLElement::hasImageOverlay()` and `HTMLElement::updateWithImageExtractionResult()`, so it can work for both image plugin elements and regular image elements (as well as anything else we may want to support in the future, such as canvas or video elements).

>> Source/WebCore/html/HTMLImageElement.cpp:832
>> +    for (auto child = makeRefPtr(shadowRoot->firstChild()); child; child = child->nextSibling()) {
> 
> Use childrenOfType<Element>?

πŸ‘πŸ» changed to use childrenOfType<Element>

>> Source/WebCore/html/HTMLImageElement.cpp:878
>> +        container->appendChild(child);
> 
> It's more efficient to build tree bottom up so do this before appending children to child.

Fixed!

>> Source/WebCore/html/HTMLImageElement.cpp:898
>> +    if (auto* frame = document().frame())
> 
> Please use makeRefPtr here.

πŸ‘πŸ»

>> Source/WebCore/html/shadow/imageOverlay.css:25
>> +div#image-overlay {
> 
> img element is display: inline by default. div is display: block by default.
> Don't we want this to be inline-block?

Good catch β€” added `display: inline-block;` to the style.
Comment 21 Wenson Hsieh 2021-02-06 13:16:43 PST
Created attachment 419514 [details]
Move code out of HTMLImageElement
Comment 22 Ryosuke Niwa 2021-02-06 15:53:03 PST
Comment on attachment 419514 [details]
Move code out of HTMLImageElement

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

> Source/WebCore/html/HTMLElement.cpp:1210
> +    for (auto& child : childrenOfType<Element>(*shadowRoot)) {
> +        if (child.getIdAttribute() == imageOverlayElementIdentifier())

Oh, now that I'm thinking about this, we should just use TreeScope::hasElementWithId instead.
Assuming we don't use this same ID elsewhere in the shadow tree.
It would be a single HashMap lookup!

> Source/WebCore/rendering/RenderImage.cpp:817
> +    for (auto* renderer = firstChild(); renderer; renderer = renderer->nextSibling()) {
> +        if (!is<RenderBox>(renderer))

Use childrenOfType<RenderBox>?
Comment 23 Wenson Hsieh 2021-02-06 17:01:21 PST
Comment on attachment 419514 [details]
Move code out of HTMLImageElement

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

>> Source/WebCore/html/HTMLElement.cpp:1210
>> +        if (child.getIdAttribute() == imageOverlayElementIdentifier())
> 
> Oh, now that I'm thinking about this, we should just use TreeScope::hasElementWithId instead.
> Assuming we don't use this same ID elsewhere in the shadow tree.
> It would be a single HashMap lookup!

Nice β€” changed to use `hasElementWithId()`.

>> Source/WebCore/rendering/RenderImage.cpp:817
>> +        if (!is<RenderBox>(renderer))
> 
> Use childrenOfType<RenderBox>?

Fixed!
Comment 24 Wenson Hsieh 2021-02-06 17:14:18 PST
Created attachment 419516 [details]
Patch
Comment 25 EWS 2021-02-06 19:29:02 PST
Committed r272467: <https://commits.webkit.org/r272467>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 419516 [details].
Comment 26 Radar WebKit Bug Importer 2021-02-10 14:44:53 PST
<rdar://problem/74207182>