1056959 - In stretched flex items w/ definite cross size from container & "auto" flex-basis, flex base size should be derived from aspect ratio

Status: VERIFIED FIXED

(Core :: Layout: Flexbox, defect, P1)

Description

[Daniel Holbert [:dholbert]]

STR:
 1. Load attached testcase

ACTUAL RESULTS:
The aqua images all have the same width.

EXPECTED RESULTS:
The aqua images should have different widths (derived from their heights & the image's intrinsic 2:1 aspect ratio)


DETAILS:
In the second and third examples in the testcase, the image's heights come from the container, because these images have the default "align-self:stretch" behavior, which stretches them to fill their container. Moreover, in the second and third lines, the container has a *definite* height (specfified px-value -- doesn't depend on content).

That means the img elements' cross sizes should also be considered "definite", due to section 9.8 saying:
  # If a single-line flex container has a
  # definite cross size, the outer cross size
  # of any stretched flex items is the flex
  # container’s inner cross size (clamped to
  # the flex item’s min and max cross size)
  # and is considered definite.
http://dev.w3.org/csswg/css-flexbox/#definite-sizes

Moreover, the flex item's flex base size (which ends up becoming its main size) should be derived using that "definite" cross size & the aspect ratio, per section 9.2 step 3B:
  # If the flex item has ...
  #   an intrinsic aspect ratio,
  #   a used flex basis of auto, and
  #   a definite cross size
  # then the flex base size is calculated
  # from its inner cross size and the flex
  # item’s intrinsic aspect ratio.
http://dev.w3.org/csswg/css-flexbox/#algo-main-item

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

Comment 2

[Daniel Holbert [:dholbert]]

Actually, it looks like CrossSizeToUseWithRatio() already tries to handle this...

http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFlexContainerFrame.cpp?rev=aece7f9f944c&mark=1104-1106#1098

...but it's not getting invoked, because we don't realize that we have "a used flex basis of auto", here:
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFlexContainerFrame.cpp?rev=aece7f9f944c&mark=1104-1106#1262

...because nsHTMLReflowState resolved a non-auto computed width for our image.

We probably need to check the actual computed style for "auto" there, instead of relying on the reflow state -- at least if we've got an element with an intrinsic aspect ratio.

Comment 3

[Tab Atkins Jr.]

In CSS issue #694 <https://github.com/w3c/csswg-drafts/issues/694>, Amelia ends up hitting this bug while exploring aspect-ratio-only images. Here's her test case, in case it's helpful: <http://codepen.io/AmeliaBR/pen/bBNxbo?editors=1100>. The images *should* fall down case 3.B in the flex layout algorithm, taking their height from the flexbox and transferring that thru their aspect ratio to produce a height, so their final size matches their desired aspect ratio.  (Instead they're falling down to case 3.E, which sets their width to the flexbox width, transfers that to their height, then shrinks the width during main-size computation, and finally squashes their height to match "stretch", losing the aspect ratio.)

(Even with ideal behavior, they'll still get squashed and lose their aspect ratio if the flexbox's width isn't enough to accommodate their desired widths, as they'll still shrink horizontally somewhat during main-size determination (and then vertically, to maintain aspect ratio, during cross-size determination), then get stretched out vertically by the align-self:stretch, losing the aspect ratio.)

Comment 4

[Daniel Holbert [:dholbert]]

Note: Chrome currently agrees with us on the attached testcase, but biesi's working a patch to change this (i.e. to fix their version of this bug). So this might become more of an interop issue soon.

Comment 5

[Boris Chiou [:boris]]

This may also have impact on this test: https://searchfox.org/mozilla-central/source/testing/web-platform/tests/css/css-sizing/aspect-ratio/flex-aspect-ratio-009.tentative.html after introducing aspect-ratio property.

Comment 8

[Ting-Yu Lin [:TYLin] (PST, UTC-7)]

Attachment: Bug 1056959 Part 1 - Pass flex container's reflow input into ResolveAutoFlexBasisAndMinSize().

This change shouldn't change behavior.

Comment 9

[Ting-Yu Lin [:TYLin] (PST, UTC-7)]

Attachment: Bug 1056959 Part 1 - Move IsCrossSizeDefinite() into FlexItem, and have it consider flex item's stretched cross size.

This change shouldn't change behavior.

SpecifiedCrossSizeIfDefinite() is not needed because if
IsCrossSizeDefinite() is true, we can just use flex item's CrossSize()
directly.

Note: IsCrossSizeDefinite() should probably use the item's writing mode
instead of container's because the item's writing mode should be used in
"sizing phase" per
https://drafts.csswg.org/css-writing-modes-4/#orthogonal-flows. We'll
fix this in the next patch.

Depends on D92670

Comment 10

[Ting-Yu Lin [:TYLin] (PST, UTC-7)]

Attachment: Bug 1056959 Part 2 - Revise FlexItem::IsCrossSizeDefinite() by using flex item's writing mode.

Depends on D92671

Comment 11

[Ting-Yu Lin [:TYLin] (PST, UTC-7)]

Attachment: Bug 1056959 Part 3 - Resolve flex item's stretched cross size before attempting to freeze the item.

In the next part, we are going to use the stretched cross size and the
aspect ratio to resolve flex base size if the pre-requisites are met.

This change shouldn't change behavior (yet).

Depends on D92672

Comment 12

[Ting-Yu Lin [:TYLin] (PST, UTC-7)]

Attachment: Bug 1056959 Part 4 - Resolve flex item's base size from cross-size and aspect ratio.

This is the main patch for this bug. The idea is to always resolve the
flex base size if the pre-requisites of Flex Layout Algorithm Step 3B
are met.

We need to resolve the flex base size before we freeze the flex item.
Otherwise, loading flex-aspect-ratio-img-row-006.html can trigger the
assertion in SetFlexBaseSizeAndMainSize(), because mFlexBaseSize is
always computes to a definite value by ReflowInput if inline axis is the
main axis.

The testcases marked as fails (flexbox-intrinsic-ratio-*) are going to
be removed in bug 1664938 since they are obsolete and incorrect.

Depends on D92673

Comment 13

[Ting-Yu Lin [:TYLin] (PST, UTC-7)]

Attachment: Bug 1056959 Part 5 - Remove newly-unnecessary code that computes flex base size from aspect ratio in ComputeSize().

In the previous part, we resolve flex base size from the cross size and
the aspect ratio, so we don't need to consider the same thing in
nsIFrame::ComputeSize().

It's effectively reverting
https://hg.mozilla.org/mozilla-central/rev/6683051983ae

Depends on D92674

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1f0e2063f733
Part 1 - Move IsCrossSizeDefinite() into FlexItem, and have it consider flex item's stretched cross size. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/2d9b40846671
Part 2 - Revise FlexItem::IsCrossSizeDefinite() by using flex item's writing mode. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/ecc3fb708804
Part 3 - Resolve flex item's stretched cross size before attempting to freeze the item. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/15346eded59c
Part 4 - Resolve flex item's base size from cross-size and aspect ratio. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/e4fdf273d586
Part 5 - Remove newly-unnecessary code that computes flex base size from aspect ratio in ComputeSize(). r=dholbert

Comment 15

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/1f0e2063f733
https://hg.mozilla.org/mozilla-central/rev/2d9b40846671
https://hg.mozilla.org/mozilla-central/rev/ecc3fb708804
https://hg.mozilla.org/mozilla-central/rev/15346eded59c
https://hg.mozilla.org/mozilla-central/rev/e4fdf273d586

Comment 17

[Bogdan Maris, Desktop Test Engineering]

Using the testcase attached here I couldn't reproduce using old Nightly. Reproduced the issue from the dupe bug 1667466 using the testcase attached in the bug on old Nightly before the fix. Also verified that using the testcase from bug 1667466 the issue does not reproduce on Firefox 83.0b10.