1646096 - Implement aspect-ratio for flex

Status: RESOLVED FIXED

(Core :: Layout: Flexbox, enhancement, P3)

Description

[Boris Chiou [:boris]]

Implement aspect-ratio on display: flex.
At least we have to pass the wpt: testing/web-platform/tests/css/css-sizing/aspect-ratio/flex-aspect-ratio-00x.tentative.html

Comment 1

[Boris Chiou [:boris]]

It seems we have to focus on

1. flex basis size algorithm.
2. transferred size suggestion. Looks like we don't handle this well for non-replaced-element flex items.

Comment 2

[Boris Chiou [:boris]]

One of the current tentative wpts depends on Bug 1056959. I'd like to keep it expected failure for now.

Comment 3

[Boris Chiou [:boris]]

Attachment: Bug 1646096 - Support aspect-ratio for flex items when computing their sizes for ReflowInput.

This patch focus on applying aspect-ratio property to flex item (basic blocks).
For replaced elements, we should rely on its IntrinsicRatio() function.

Comment 4

[Boris Chiou [:boris]]

Attachment: Bug 1646096 - Handle min-block-size:auto case for aspect-ratio on the flex container.

This depends on github.com/w3c/csswg-drafts/issues/5406 acutally.
I tried this tentative way to take aspect-ratio into account for
handling min main size auto case on the flex container.
(https://drafts.csswg.org/css-sizing-4/#aspect-ratio-minimum)

Basically, we treat the main size as auto if it is ratio-dependent axis.
So we can put as many as flex items in this line. And then we compare the
largest line length of this to the main size calculated by aspect-ratio, and
use the larger one as the input for computing the final main size.

Comment 5

[Boris Chiou [:boris]]

Attachment: Bug 1646096 - Handle aspect-ratio for Automatic Minimum Size of Flex items.

This tweaks the retrieval of aspect-ratio for Automatic Minimum Size of
Flex Items.

https://drafts.csswg.org/css-flexbox/#min-size-auto

Comment 6

[Boris Chiou [:boris]]

Attachment: Bug 1646096 - Mark bug number for flex-aspect-ratio-009.tentative.html.

Comment 7

[Boris Chiou [:boris]]

I notice there are some new wpts (Bug 1660056) added yesterday we don't pass based on my current patches. I'm looking at them. May add new extra patches or file a different bug, depends on where the root cause happens.

Comment 8

[Boris Chiou [:boris]]

For the cross size determination of each item we

determine the hypothetical cross size of each item by performing layout as if it were an in-flow block-level box with the used main size and the given available space, treating auto as fit-content.

So perhaps this is the part I didn't address in the patch series. If we need to do this as in-flow block-level box, then it makes sense to use the used main size and the aspect-ratio to get the hypothetical cross size for the flex items.

Comment 9

[Boris Chiou [:boris]]

(In reply to Boris Chiou [:boris] from comment #8)

For the cross size determination of each item we

determine the hypothetical cross size of each item by performing layout as if it were an in-flow block-level box with the used main size and the given available space, treating auto as fit-content.

So perhaps this is the part I didn't address in the patch series. If we need to do this as in-flow block-level box, then it makes sense to use the used main size and the aspect-ratio to get the hypothetical cross size for the flex items.

OK. So it seems the reasonable way to fix this is to do the similar things as the imposed main size for both replaced elements and non-replaced elements (i.e. also in nsIFrame::ComputeSize) if we have the non-auto aspect-ratio for this flex item and have set the frame property, nsIFrame::FlexItemMainSizeOverride().

Comment 11

[Boris Chiou [:boris]]

Attachment: Bug 1646096 - Take aspect-ratio into account in FlexItem::mIntrinsicRatio.

Comment 12

[Boris Chiou [:boris]]

Attachment: Bug 1646096 - Fix the calculation of the flex item's cross size with respect to aspect-ratio.

Address aspect-ratio cases for non-replaced elements, for
https://drafts.csswg.org/css-flexbox-1/#cross-sizing.

Comment 13

[Boris Chiou [:boris]]

Attachment: Bug 1646096 - The cross-size of the flex container should take the imposed size into account.

Comment 14

[Boris Chiou [:boris]]

Attachment: Bug 1646096 - Mark bug number for the incorrect calculation of the cross size for column-oriented flex container.

Comment 15

[Daniel Holbert [:dholbert]]

Attachment: Bug 1646096: Add back assertion to ensure we don't create printed sheets with no pages on them (softened for the first sheet). r?hiro

Per the comment in the patch: if we're given a bogus range, we can technically
end up with a single PrintedSheetFrame that contains no displayable
pages. Luckily, that's a situation that the frontend should detect & recover
from quickly & gracefully, so it's not really a problem. So: in that scenario,
we'll now spam a non-fatal warning; and we'll fatally assert if any sheet
beyond the first page has zero displayable pages. (That still shouldn't be
possible.)

Comment 16

[Phabricator Automation]

Comment on attachment 9174560 [details]
Bug 1646096: Add back assertion to ensure we don't create printed sheets with no pages on them (softened for the first sheet). r?hiro

Revision D89537 was moved to bug 1662940. Setting attachment 9174560 [details] to obsolete.

Comment 17

[Daniel Holbert [:dholbert]]

Attachment: testcase that we should avoid breaking

Here's a testcase to illustrate why we can't just leave mainAxisCoord untouched as noted in my inline comment in phabricator.

This is a variant of the existing web-platform test https://searchfox.org/mozilla-central/source/testing/web-platform/tests/css/css-sizing/aspect-ratio/flex-aspect-ratio-001.tentative.html -- I've moved the test's styles to a <style> block, and added my own tweaks in inline styles.

With your current first patch in the patch stack here, the third chunk in this testcase renders as a skinny green bar, rather than a green square. But really, all three chunks should render the same. (Chrome "agrees" with us right now simply because they don't support the explicit content keyword for flex-basis, so they're not a trustworthy source on the rendering of this third example)

Comment 18

[Boris Chiou [:boris]]

Attachment: Bug 1646096 - Add tests for replaced-element flex items with aspect-ratio.

So we have the better test coverage for apsect-ratio on flex layout.

Those wpts should be passed already because we handle aspect-ratio
property well in GetIntrinisicRatio() for all the replaced elements.

Comment 19

[Boris Chiou [:boris]]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=48bb933db3a99811d134bba8d590459c196e2f82

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5ae44731baab
Add tests for replaced-element flex items with aspect-ratio. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/6683051983ae
Support aspect-ratio for flex items when computing their sizes for ReflowInput. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/bbc133587c17
Handle aspect-ratio for Automatic Minimum Size of Flex items. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/1f41a35021e9
Mark bug number for flex-aspect-ratio-009.tentative.html. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/1f474090b6e8
Take aspect-ratio into account in FlexItem::mIntrinsicRatio. r=dholbert,TYLin
https://hg.mozilla.org/integration/autoland/rev/e3eb1e60ee1f
Fix the calculation of the flex item's cross size with respect to aspect-ratio. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/fd47099e9a65
Mark bug number for the incorrect calculation of the cross size for column-oriented flex container. r=dholbert

Comment 21

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/25664 for changes under testing/web-platform/tests

Comment 22

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 23

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/5ae44731baab
https://hg.mozilla.org/mozilla-central/rev/6683051983ae
https://hg.mozilla.org/mozilla-central/rev/bbc133587c17
https://hg.mozilla.org/mozilla-central/rev/1f41a35021e9
https://hg.mozilla.org/mozilla-central/rev/1f474090b6e8
https://hg.mozilla.org/mozilla-central/rev/e3eb1e60ee1f
https://hg.mozilla.org/mozilla-central/rev/fd47099e9a65

Comment 24

[Rachel Andrew]

I've updated BCD for this partial implementation of the feature.