Bug 228283 – Fix aspect-ratio-intrinsic-size-004.html

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

[Rob Buis]

Fix aspect-ratio-intrinsic-size-00?.html.

[Rob Buis]

Created attachment 434211 [details]
Patch

[Rob Buis]

Rough fix for now, I hope this bug could also fix aspect-ratio-intrinsic-size-005.html.

[Rob Buis]

Created attachment 434275 [details]
Patch

[Rob Buis]

Created attachment 434281 [details]
Patch

[Rob Buis]

Created attachment 434282 [details]
Patch

[Rob Buis]

Created attachment 434283 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/81415269>

[Rob Buis]

Created attachment 435550 [details]
Patch

[Rob Buis]

Created attachment 435557 [details]
Patch

[Rob Buis]

Created attachment 436615 [details]
Patch

[Rob Buis]

Created attachment 436617 [details]
Patch

[Rob Buis]

Created attachment 436618 [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

[Rob Buis]

Created attachment 436633 [details]
Patch

[Rob Buis]

Created attachment 436759 [details]
Patch

[Sergio Villar Senin]

Comment on attachment 436759 [details]
Patch

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

I've serious doubts about two call sites added by this patch  and thus the r-.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:353
> +    dirtyForLayoutFromPercentageHeightDescendants();

This looks pretty interesting indeed. I haven't realized before that it's only called from RenderBlockFlow and thus never for flexboxes. I wonder whether we should call it from RenderFlexibleBox::layoutAndPlaceChildren() to kind of mimic the location where it's called in RenderBlockFlow...

> Source/WebCore/rendering/RenderFlexibleBox.cpp:966
> +            return percentageLogicalHeightIsResolvable();

Hmm I am not sure we could call percentageLogicalHeightIsResolvable() here. This method could start the following sequence:

percentageLogicalHeightIsResolvable()->computePercentageLogicalHeight()->availableLogicalHeightForPercentageComputation()->useChildOverridingLogicalHeightForPercentageResolution()->useChildOverridingMainSizeForPercentageResolution()->canComputePercentageFlexBasis()->computePercentageLogicalHeight()->availableLogicalHeightForPercentageComputation() ...

entering a loop of calls. Do you have a test for this case?

> Source/WebCore/rendering/RenderFlexibleBox.cpp:1056
> +            addPercentHeightDescendant(*child);

Doesn't look 100% correct. A content dependent size (like min-content, max-content etc...) isn't also definite but it isn't a percentage.

Also it shouldn't be really called from RenderFlexibleBox, looks something we're missing in RenderBox to handle the case of items with aspect ratio.

[Rob Buis]

Created attachment 436867 [details]
Patch

[Rob Buis]

Created attachment 436879 [details]
Patch

[Rob Buis]

Created attachment 437004 [details]
Patch

[Rob Buis]

Created attachment 437136 [details]
Patch

[Rob Buis]

Created attachment 437165 [details]
Patch

[Rob Buis]

Created attachment 437297 [details]
Patch

[Rob Buis]

Created attachment 437470 [details]
Patch

[Rob Buis]

Created attachment 437471 [details]
Patch

[Rob Buis]

Created attachment 437479 [details]
Patch

[Rob Buis]

Created attachment 437492 [details]
Patch

[Sergio Villar Senin]

Comment on attachment 437492 [details]
Patch

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

We're getting there, thanks for your patience providing alternatives :)

> Source/WebCore/rendering/RenderFlexibleBox.cpp:1789
> +

I'd better not use this name as is too similar to RenderBlock's needsPreferredWidthsRecalculation(). I'd go for a more specific name like childHasAspectRatioAndStretchedLogicalHeight() or the like.

However this method is so specific and used just once that I think we can get rid of it (or at the most convert it into a lambda in the place you use it)

> Source/WebCore/rendering/RenderFlexibleBox.cpp:1874
> +            child.setPreferredLogicalWidthsDirty(true, MarkOnlyThis);

The setPreferredLogicalWidthsDirty() is done by updateBlockChildDirtyBitsBeforeLayout(), you just need to set forceChildRelayout to true as in the block above for percentage children. So something like this:

if (!forceChildRelayout && (childHasPercentHeightDescendants(child) || (child.style().hasAspectRatio() && child.hasStretchedLogicalHeight()))) {
   [...]
   forceChildRelayout = true;
}

should do the job.

[Rob Buis]

Created attachment 437849 [details]
Patch

[Sergio Villar Senin]

Comment on attachment 437849 [details]
Patch

Awesome thanks!

[EWS]

Committed r282264 (241542@main): <https://commits.webkit.org/241542@main>

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