Bug 223569 – [css-contain] Support contain: layout

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Rob Buis]

Support contain: layout.

[Rob Buis]

Created attachment 423869 [details]
Patch

[Rob Buis]

Created attachment 424002 [details]
Patch

[Simon Fraser (smfr)]

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.

[Rob Buis]

Created attachment 424106 [details]
Patch

[EWS Watchlist]

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

[Radar WebKit Bug Importer]

<rdar://problem/75956829>

[Rob Buis]

Created attachment 424744 [details]
Patch

[Rob Buis]

Created attachment 424765 [details]
Patch

[Simon Fraser (smfr)]

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

[Rob Buis]

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

[Rob Buis]

Created attachment 425001 [details]
Patch

[Rob Buis]

Created attachment 425004 [details]
Patch

[Rob Buis]

Created attachment 425152 [details]
Patch

[Rob Buis]

Created attachment 425265 [details]
Patch

[Rob Buis]

This is ready for review :)

[Simon Fraser (smfr)]

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

[Rob Buis]

Created attachment 425847 [details]
Patch

[Rob Buis]

Created attachment 425874 [details]
Patch

[Rob Buis]

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.

[Darin Adler]

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.

[Rob Buis]

Created attachment 426377 [details]
Patch

[Rob Buis]

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.

[Rob Buis]

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

[EWS]

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

[Darin Adler]

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.

[Rob Buis]

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

[cathiechen]

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

[zalan]

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

[zalan]

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

[Rob Buis]

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