Bug 252245

Summary: [css-contain-intrinsic-size] auto-011.html is failing
Product: WebKit Reporter: cathiechen <cathiechen>
Component: CSSAssignee: cathiechen <cathiechen>
Status: RESOLVED FIXED    
Severity: Normal CC: obrufau, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=268874
Bug Depends on:    
Bug Blocks: 236707    
Attachments:
Description Flags
WIP-patch
none
WIP-patch none

Description cathiechen 2023-02-14 08:54:33 PST 
Per[1], contain:inline-size applies contain:size to the inline direction, so contain-intrinsic-size:auto should save last remembered size in the inline direction.

[1] https://drafts.csswg.org/css-contain-3/#containment-inline-size
Comment 1 cathiechen 2023-02-14 08:56:10 PST 
Created attachment 464983 [details]
WIP-patch
Comment 2 Radar WebKit Bug Importer 2023-02-21 08:55:20 PST 
<rdar://problem/105728798>
Comment 3 Oriol Brufau 2023-02-22 14:00:53 PST 
Comment on attachment 464983 [details]
WIP-patch

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

> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:798
> +    if (m_direction == ForColumns && (m_strategy->isComputingSizeContainment() || m_strategy->isComputingInlineSizeContainment())) {

May be good to add GridTrackSizingAlgorithmStrategy::isComputingSizeOrInlineSizeContainment()

> Source/WebCore/rendering/RenderBox.cpp:5604
> +    ASSERT(shouldApplySizeContainment() || (isHorizontalWritingMode() && shouldApplyInlineSizeContainment()));

Nit: shouldApplySizeContainment() and shouldApplyInlineSizeContainment() have some common logic that might run twice, so this seems better:

isHorizontalWritingMode() ? shouldApplySizeOrInlineSizeContainment() : shouldApplySizeContainment()

> Source/WebCore/rendering/RenderBox.cpp:5620
> +    ASSERT(shouldApplySizeContainment() || (!isHorizontalWritingMode() && shouldApplyInlineSizeContainment()));

Nit: isHorizontalWritingMode() ? shouldApplySizeContainment() : shouldApplySizeOrInlineSizeContainment()

> Source/WebCore/rendering/RenderGrid.cpp:678
> +    if (direction == ForColumns && shouldApplyInlineSizeContainment())
> +        return true;
> +
> +    return shouldApplySizeContainment();

Nit: return direction == ForColumns ? shouldApplySizeOrInlineSizeContainment() : shouldApplySizeContainment();
Comment 4 cathiechen 2023-02-23 07:48:50 PST 
Comment on attachment 464983 [details]
WIP-patch

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

Thanks for the review, Oriol!

>> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:798
>> +    if (m_direction == ForColumns && (m_strategy->isComputingSizeContainment() || m_strategy->isComputingInlineSizeContainment())) {
> 
> May be good to add GridTrackSizingAlgorithmStrategy::isComputingSizeOrInlineSizeContainment()

Done!

>> Source/WebCore/rendering/RenderBox.cpp:5604
>> +    ASSERT(shouldApplySizeContainment() || (isHorizontalWritingMode() && shouldApplyInlineSizeContainment()));
> 
> Nit: shouldApplySizeContainment() and shouldApplyInlineSizeContainment() have some common logic that might run twice, so this seems better:
> 
> isHorizontalWritingMode() ? shouldApplySizeOrInlineSizeContainment() : shouldApplySizeContainment()

Done

>> Source/WebCore/rendering/RenderBox.cpp:5620
>> +    ASSERT(shouldApplySizeContainment() || (!isHorizontalWritingMode() && shouldApplyInlineSizeContainment()));
> 
> Nit: isHorizontalWritingMode() ? shouldApplySizeContainment() : shouldApplySizeOrInlineSizeContainment()

Done

>> Source/WebCore/rendering/RenderGrid.cpp:678
>> +    return shouldApplySizeContainment();
> 
> Nit: return direction == ForColumns ? shouldApplySizeOrInlineSizeContainment() : shouldApplySizeContainment();

Done:)
Comment 5 cathiechen 2023-02-23 07:51:12 PST 
Created attachment 465135 [details]
WIP-patch
Comment 6 Oriol Brufau 2023-02-23 09:00:21 PST 
Comment on attachment 465135 [details]
WIP-patch

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

> Source/WebCore/rendering/GridTrackSizingAlgorithm.h:290
> +    bool isComputingSizeOrInlineSizeContainment() { return isComputingSizeContainment() || isComputingInlineSizeContainment(); }

Nit: probably not that important in practice, but making it virtual and then in IndefiniteSizeStrategy

bool isComputingSizeOrInlineSizeContainment() const override { return renderGrid()->shouldApplySizeOrInlineSizeContainment(); }

would ensure a single shouldApplySizeOrStyleContainment().
Comment 7 cathiechen 2023-02-28 06:55:25 PST 
Comment on attachment 465135 [details]
WIP-patch

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

>> Source/WebCore/rendering/GridTrackSizingAlgorithm.h:290
>> +    bool isComputingSizeOrInlineSizeContainment() { return isComputingSizeContainment() || isComputingInlineSizeContainment(); }
> 
> Nit: probably not that important in practice, but making it virtual and then in IndefiniteSizeStrategy
> 
> bool isComputingSizeOrInlineSizeContainment() const override { return renderGrid()->shouldApplySizeOrInlineSizeContainment(); }
> 
> would ensure a single shouldApplySizeOrStyleContainment().

Done, thanks:)
Comment 8 cathiechen 2023-02-28 07:09:17 PST 
Pull request: https://github.com/WebKit/WebKit/pull/10797
Comment 9 EWS 2023-03-01 06:31:50 PST 
Committed 261003@main (9b61d115bb39): <https://commits.webkit.org/261003@main>

Reviewed commits have been landed. Closing PR #10797 and removing active labels.