SSIA
Created attachment 419089 [details] WIP
Created attachment 419090 [details] WIP
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 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...
(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.
How does all this work in the face of object-fit?
Created attachment 419342 [details] Patch
(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 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.
Created attachment 419363 [details] Patch
Created attachment 419365 [details] Fix GTK/WPE builds
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 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 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 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 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?)
Created attachment 419425 [details] Patch
Created attachment 419426 [details] Fix title
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 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.
Created attachment 419514 [details] Move code out of HTMLImageElement
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 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!
Created attachment 419516 [details] Patch
Committed r272467: <https://commits.webkit.org/r272467> All reviewed patches have been landed. Closing bug and clearing flags on attachment 419516 [details].
<rdar://problem/74207182>