Bug 223569

Created attachment 423869 [details] Patch

Created attachment 424002 [details] Patch

Since all the 'contain' implementations are likely to be somewhat involved, I think you should write up a page on the wiki describing your plans, and to solicit pre-patch feedback.

Created attachment 424106 [details] Patch

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

<rdar://problem/75956829>

Created attachment 424744 [details] Patch

Created attachment 424765 [details] Patch

(In reply to Simon Fraser (smfr) from comment #3) > Since all the 'contain' implementations are likely to be somewhat involved, > I think you should write up a page on the wiki describing your plans, and to > solicit pre-patch feedback. Did this happen?

(In reply to Simon Fraser (smfr) from comment #9) > (In reply to Simon Fraser (smfr) from comment #3) > > Since all the 'contain' implementations are likely to be somewhat involved, > > I think you should write up a page on the wiki describing your plans, and to > > solicit pre-patch feedback. > > Did this happen? Yes, sorry, here it is https://trac.webkit.org/wiki/CSSContainment. We plan to add more for the remaining features (like contain: paint) once we start prototyping.

Created attachment 425001 [details] Patch

Created attachment 425004 [details] Patch

Created attachment 425152 [details] Patch

Created attachment 425265 [details] Patch

This is ready for review :)

Comment on attachment 425265 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425265&action=review > Source/WebCore/rendering/RenderLayer.cpp:584 > - return !renderer().style().hasAutoUsedZIndex() || isRenderViewLayer(); > + return renderer().shouldApplyLayoutContainment() || !renderer().style().hasAutoUsedZIndex() || isRenderViewLayer(); This isn't the right way to do this. You should d this in Style::Adjuster::adjust()

Created attachment 425847 [details] Patch

Created attachment 425874 [details] Patch

Comment on attachment 425265 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425265&action=review >> Source/WebCore/rendering/RenderLayer.cpp:584 >> + return renderer().shouldApplyLayoutContainment() || !renderer().style().hasAutoUsedZIndex() || isRenderViewLayer(); > > This isn't the right way to do this. You should d this in Style::Adjuster::adjust() I see, I moved the logic there in the latest patch.

Comment on attachment 425874 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425874&action=review No need for a feature flag? > Source/WebCore/rendering/RenderBlock.cpp:2578 > + return Optional<LayoutUnit>(); WTF::nullopt is the way we normally write this; not sure why we’re using something different here > Source/WebCore/rendering/RenderBlock.cpp:2596 > + return Optional<LayoutUnit>(synthesizedBaselineFromBorderBox(*this, lineDirection) + (lineDirection == HorizontalLine ? marginBottom(): marginLeft())); We normally put a space before the ":". Also, is the Optional<LayoutUnit> really needed? Can we just write LayoutUnit instead? Or nothing at all? > Source/WebCore/rendering/RenderObject.h:189 > + bool isAtomicInlineLevelBox() const > + { > + return style().isDisplayInlineType() && !(style().display() == DisplayType::Inline && !isReplaced()); > + } In a large class like this, it’s best to keep multi-line function bodies (even if only one line has code) out of the class definition so we can see an overview of what’s in the class rather than have it interspersed with code. Instead we can just put an inline implementation after the class. See RenderObject::isBeforeContent for an example. > Source/WebCore/rendering/RenderObject.h:191 > + inline bool shouldApplyLayoutContainment() const The inline here is unneeded and redundant if we are putting the function body inside the class. Or we can move this out. I’m a bit uncomfortable adding more operations to RenderObject itself. Seems like the class is getting out of hand. Other options include putting this in some derived class, or writing it as a free function that takes a const RenderObject& argument, since nothing in these two functions requires access to anything private.

Created attachment 426377 [details] Patch

Comment on attachment 425874 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425874&action=review >> Source/WebCore/rendering/RenderBlock.cpp:2578 >> + return Optional<LayoutUnit>(); > > WTF::nullopt is the way we normally write this; not sure why we’re using something different here Done. >> Source/WebCore/rendering/RenderBlock.cpp:2596 >> + return Optional<LayoutUnit>(synthesizedBaselineFromBorderBox(*this, lineDirection) + (lineDirection == HorizontalLine ? marginBottom(): marginLeft())); > > We normally put a space before the ":". > > Also, is the Optional<LayoutUnit> really needed? Can we just write LayoutUnit instead? Or nothing at all? Right that was a typo. I agree it is nice to not explicitly state the return type, fixed. >> Source/WebCore/rendering/RenderObject.h:189 >> + } > > In a large class like this, it’s best to keep multi-line function bodies (even if only one line has code) out of the class definition so we can see an overview of what’s in the class rather than have it interspersed with code. Instead we can just put an inline implementation after the class. See RenderObject::isBeforeContent for an example. Fixed. >> Source/WebCore/rendering/RenderObject.h:191 >> + inline bool shouldApplyLayoutContainment() const > > The inline here is unneeded and redundant if we are putting the function body inside the class. > > Or we can move this out. > > I’m a bit uncomfortable adding more operations to RenderObject itself. Seems like the class is getting out of hand. Other options include putting this in some derived class, or writing it as a free function that takes a const RenderObject& argument, since nothing in these two functions requires access to anything private. Understood, I moved it out.

(In reply to Darin Adler from comment #20) > Comment on attachment 425874 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=425874&action=review > > No need for a feature flag? It is implied by RenderStyle::containsLayout().

Committed r276235 (236717@main): <https://commits.webkit.org/236717@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 426377 [details].

Comment on attachment 426377 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426377&action=review > Source/WebCore/rendering/RenderObject.h:206 > + inline bool isAtomicInlineLevelBox() const; This isn’t quite right. We should leave out the inline keyword here. > Source/WebCore/rendering/RenderObject.h:1180 > +bool RenderObject::isAtomicInlineLevelBox() const We need to put the inline keyword here.

Comment on attachment 426377 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426377&action=review >> Source/WebCore/rendering/RenderObject.h:206 >> + inline bool isAtomicInlineLevelBox() const; > > This isn’t quite right. We should leave out the inline keyword here. Right, I was a bit hasty there. >> Source/WebCore/rendering/RenderObject.h:1180 >> +bool RenderObject::isAtomicInlineLevelBox() const > > We need to put the inline keyword here. Cathie will fix both in this patch which also relies on isAtomicInlineLevelBox (https://bugs.webkit.org/show_bug.cgi?id=223570).

(In reply to Rob Buis from comment #26) > Comment on attachment 426377 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=426377&action=review > > >> Source/WebCore/rendering/RenderObject.h:206 > >> + inline bool isAtomicInlineLevelBox() const; > > > > This isn’t quite right. We should leave out the inline keyword here. > > Right, I was a bit hasty there. > > >> Source/WebCore/rendering/RenderObject.h:1180 > >> +bool RenderObject::isAtomicInlineLevelBox() const > > > > We need to put the inline keyword here. > > Cathie will fix both in this patch which also relies on > isAtomicInlineLevelBox (https://bugs.webkit.org/show_bug.cgi?id=223570). Done:)

Unfortunately this won't work with inline level boxes where the containment render is a type of RenderInline. The rendering code assumes the containing block always being a RenderBlock type. (there's a workaround for similar problem where the relatively positioned inline box (e.g. <span style="position: relative">) has an absolutely positioned block level box descendant. In such cases, we propagate the containing block functionality to the nearest non-anonymous containing block on the ancestor chain). However this is not a problem in LFC codebase where the layout nodes (renderer) are not typed heavily (no RenderInline vs RenderBlock) so the containingBlock() just returns a ContainerBox&.

(In reply to zalan from comment #28) > Unfortunately this won't work with inline level boxes where the containment > render is a type of RenderInline. The rendering code assumes the containing > block always being a RenderBlock type. (there's a workaround for similar > problem where the relatively positioned inline box (e.g. <span > style="position: relative">) has an absolutely positioned block level box > descendant. In such cases, we propagate the containing block functionality > to the nearest non-anonymous containing block on the ancestor chain). > However this is not a problem in LFC codebase where the layout nodes > (renderer) are not typed heavily (no RenderInline vs RenderBlock) so the > containingBlock() just returns a ContainerBox&. I need to think about this a bit.

(In reply to zalan from comment #28) > Unfortunately this won't work with inline level boxes where the containment > render is a type of RenderInline. The rendering code assumes the containing > block always being a RenderBlock type. (there's a workaround for similar > problem where the relatively positioned inline box (e.g. <span > style="position: relative">) has an absolutely positioned block level box > descendant. In such cases, we propagate the containing block functionality > to the nearest non-anonymous containing block on the ancestor chain). > However this is not a problem in LFC codebase where the layout nodes > (renderer) are not typed heavily (no RenderInline vs RenderBlock) so the > containingBlock() just returns a ContainerBox&. Do you mean the landed patch will try to apply layout containment to those inline level boxes you described and there will be no effect? Or mistakenly not even bother trying? A (WPT) test may be a good idea if we currently do the wrong thing (and then we can compare to the other implementations).