Closed Bug 1056959 Opened 7 years ago Closed 1 year ago

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

Categories

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

defect

Tracking

()

VERIFIED FIXED
83 Branch
Tracking Status
firefox83 --- verified

People

(Reporter: dholbert, Assigned: TYLin)

References

(Blocks 1 open bug, Regressed 1 open bug)

Details

Attachments

(6 files, 1 obsolete file)

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
Attached file testcase 1
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
OS: Linux → All
Hardware: x86_64 → All
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.
Flags: in-testsuite?
Flags: needinfo?(dholbert)
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.)

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.

Blocks: 1658441
Duplicate of this bug: 1667466
Duplicate of this bug: 1667517

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

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

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

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

Attachment #9180011 - Attachment description: Bug 1056959 Part 2 - Move IsCrossSizeDefinite() into FlexItem, and have it consider flex item's stretched cross size. → Bug 1056959 Part 1 - Move IsCrossSizeDefinite() into FlexItem, and have it consider flex item's stretched cross size.
Attachment #9180012 - Attachment description: Bug 1056959 Part 3 - Revise FlexItem::IsCrossSizeDefinite() by using flex item's writing mode. → Bug 1056959 Part 2 - Revise FlexItem::IsCrossSizeDefinite() by using flex item's writing mode.
Attachment #9180013 - Attachment description: Bug 1056959 Part 4 - Resolve flex item's stretched cross size before attempting to freeze the item. → Bug 1056959 Part 3 - Resolve flex item's stretched cross size before attempting to freeze the item.
Attachment #9180014 - Attachment description: Bug 1056959 Part 5 - Resolve flex item's base size from cross-size and aspect ratio. → Bug 1056959 Part 4 - Resolve flex item's base size from cross-size and aspect ratio.
Attachment #9180015 - Attachment description: Bug 1056959 Part 6 - Remove dead code that computes flex base size from aspect ratio in ComputeSize(). → Bug 1056959 Part 5 - Remove dead code that computes flex base size from aspect ratio in ComputeSize().
Attachment #9180010 - Attachment is obsolete: true
Flags: needinfo?(dholbert)
Assignee: dholbert → aethanyc
Severity: normal → S3
Component: Layout → Layout: Flexbox
Priority: -- → P1
Attachment #9180015 - Attachment description: Bug 1056959 Part 5 - Remove dead code that computes flex base size from aspect ratio in ComputeSize(). → Bug 1056959 Part 5 - Remove newly-unnecessary code that computes flex base size from aspect ratio in ComputeSize().
Pushed by aethanyc@gmail.com:
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
Duplicate of this bug: 1658441
QA Whiteboard: [qa-83b-p2]

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.

Status: RESOLVED → VERIFIED
QA Whiteboard: [qa-83b-p2]
Regressions: 1679572
Regressions: 1711823
You need to log in before you can comment on or make changes to this bug.