Closed Bug 1646096 Opened 4 years ago Closed 4 years ago

Implement aspect-ratio for flex

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
83 Branch
Tracking Status
firefox83 --- fixed

People

(Reporter: boris, Assigned: boris)

References

(Blocks 2 open bugs)

Details

(Keywords: dev-doc-complete)

Attachments

(8 files, 3 obsolete files)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
740 bytes, text/html
Details
47 bytes, text/x-phabricator-request
Details | Review

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

Assignee: nobody → boris.chiou

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.
See Also: → 1136312

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

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

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.

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

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

Status: NEW → ASSIGNED
Attachment #9169303 - Attachment description: Bug 1646096 - Handle min main size auto case for aspect-ratio on the flex container. → Bug 1646096 - Handle min-block-size:auto case for aspect-ratio on the flex container.

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.

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.

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

Blocks: 1660355
Attachment #9169302 - Attachment description: Bug 1646096 - Support aspect-ratio when checking the flex-basis size. → Bug 1646096 - Support aspect-ratio for flex items when computing their sizes for ReflowInput.
Attachment #9169304 - Attachment description: Bug 1646096 - Handle aspect-ratio for flex items. → Bug 1646096 - Handle aspect-ratio for Automatic Minimum Size of Flex items.
Attachment #9172014 - Attachment is obsolete: true
Blocks: 1661847
See Also: → 1658441
Attachment #9169303 - Attachment is obsolete: true

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

Attachment #9174560 - Attachment is obsolete: true
Attachment #9169303 - Attachment is obsolete: false
Attachment #9172013 - Attachment description: Bug 1646096 - Fix the calulation of the cross size with respect to aspect-ratio. → Bug 1646096 - Fix the calculation of the flex item's cross size with respect to aspect-ratio.
Attachment #9169303 - Attachment is obsolete: true

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)

Attachment #9176381 - Attachment description: testcase to avoid breaking → testcase that we should avoid breaking

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.

Pushed by bchiou@mozilla.com:
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
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/25664 for changes under testing/web-platform/tests
Upstream PR merged by moz-wptsync-bot

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

You need to log in before you can comment on or make changes to this bug.