Bug 221236 – Introduce image overlays and add UA shadow root support for image extraction

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Wenson Hsieh]

SSIA

[Wenson Hsieh]

Created attachment 419089 [details]
WIP

[Wenson Hsieh]

Created attachment 419090 [details]
WIP

[Devin Rousso]

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?

[Wenson Hsieh]

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

[Wenson Hsieh]

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

[Simon Fraser (smfr)]

How does all this work in the face of object-fit?

[Wenson Hsieh]

Created attachment 419342 [details]
Patch

[Wenson Hsieh]

(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).

[Ryosuke Niwa]

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.

[Wenson Hsieh]

Created attachment 419363 [details]
Patch

[Wenson Hsieh]

Created attachment 419365 [details]
Fix GTK/WPE builds

[Wenson Hsieh]

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

[Antti Koivisto]

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?

[Antti Koivisto]

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.

[Wenson Hsieh]

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!

[Wenson Hsieh]

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

[Wenson Hsieh]

Created attachment 419425 [details]
Patch

[Wenson Hsieh]

Created attachment 419426 [details]
Fix title

[Ryosuke Niwa]

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?

[Wenson Hsieh]

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.

[Wenson Hsieh]

Created attachment 419514 [details]
Move code out of HTMLImageElement

[Ryosuke Niwa]

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

[Wenson Hsieh]

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!

[Wenson Hsieh]

Created attachment 419516 [details]
Patch

[EWS]

Committed r272467: <https://commits.webkit.org/r272467>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 419516 [details].

[Radar WebKit Bug Importer]

<rdar://problem/74207182>