Closed Bug 1316534 Opened 8 years ago Closed 4 years ago

[css-flexbox] A non-default "flex-basis" incorrectly prevents min-size:auto from being clamped by specified size

Categories

(Core :: Layout: Flexbox, defect)

Product:
Core
Component:
Layout: Flexbox
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
83 Branch
Project Flags:
Webcompat Priority P1
Tracking Flags:
Tracking Status
firefox52 --- wontfix
firefox68 --- wontfix
firefox83 --- fixed

People

(Reporter: dholbert, Assigned: TYLin)

References

(Blocks 1 open bug)

Details

(Whiteboard: [webcompat], [wptsync upstream])

Attachments

(9 files)

testcase 1
(deleted), text/html
Details
Bug 1316534 Part 1 - Removed unused arguments to nsIFrame::ComputeISizeValue() computing LengthPercentage ISize(). r=dholbert
(deleted), text/x-phabricator-request
Details
Bug 1316534 Part 2 - Mark image flex item's size depending on the its intrinsic size. r=dholbert
(deleted), text/x-phabricator-request
Details
Bug 1316534 Part 5 - Update flex item's specified / content size suggestion implementation to match the spec. r=dholbert
(deleted), text/x-phabricator-request
Details
Bug 1316534 Part 3 - Make ComputeSizeWithIntrinsicDimensions() respect ComputeSizeFlag::UseAutoBSize. r=dholbert
(deleted), text/x-phabricator-request
Details
Bug 1316534 Part 4 - Fix some incorrect flexbox tests. r=dholbert
(deleted), text/x-phabricator-request
Details
Bug 1316534 Part 6 - Update flex item's transferred size suggestion implementation to match the spec. r=dholbert
(deleted), text/x-phabricator-request
Details
Bug 1316534 Part 7 - Removed the unused aMinSizeFallback in CrossSizeToUseWithRatio(). r=dholbert
(deleted), text/x-phabricator-request
Details
Bug 1316534 Part 8 - Update the comment in ResolveAutoFlexBasisFromRatio(). r=dholbert
(deleted), text/x-phabricator-request
Details
Attached file testcase 1 (deleted) —
[Filing this for a Gecko bug identified in https://github.com/webcompat/web-bugs/issues/3038 ] STR: 1. Load attached testcase. EXPECTED RESULTS: The purple div and green div should be equally wide, and should exactly fill the dotted border around them. ACTUAL RESULTS: Purple bar is extra wide -- it shrinkwraps its content's size. This is from us resolving "min-width:auto" on flex items: http://dev.w3.org/csswg/css3-flexbox/#min-size-auto Quoting the spec's summary of that section: > In general, the automatic minimum size is the smaller of > its content size and its specified size. We're letting the specified size ("width: 20px") win in the lower example, but not in the higher one. We're letting the "flex-basis" mess things up for some reason.
Summary: A non-default "flex-basis" incorrectly prevents min-size:auto from being clamped by specified size → [css-flexbox] A non-default "flex-basis" incorrectly prevents min-size:auto from being clamped by specified size
Looks like we correctly implemented the spec, but the spec changed. Here's the old version of what is now called "specified size" in the min-width:auto flexbox spec text: > clamped size > is defined if the item’s flex-basis is `main-size` > and its computed main size property is definite, > and is that size (clamped by its max main size property > if it’s definite). https://www.w3.org/TR/2014/WD-css-flexbox-1-20140925/#min-size-auto The "if the item’s flex-basis is `main-size`" caveat there is what's tripping us up. (That really means to say "if the item’s flex-basis is at its initial value" -- there was briefly a keyword called "main-size" in the spec which represented the initial value, but the initial value was later renamed back to "auto" rather than "main-size".) So: we're only applying this specified-size clamping if the flex-basis is at its initial value, which is correct according to this old spec text, but incorrect according to the current spec.
Blocks: flexbox-spec-changes
Flags: needinfo?(dholbert)
Depends on: 1015474
The code that needs fixing is here (and currently basically corresponds to the old spec-text quoted in comment 1): https://dxr.mozilla.org/mozilla-central/rev/d38d06f85ef59c5dbb5d4a1a8d895957a78714de/layout/generic/nsFlexContainerFrame.cpp#1587
Too late for firefox 52, mass-wontfix.
status-firefox52: affected → wontfix
Flags: needinfo?(dholbert)
Whiteboard: [webcompat]
Flags: needinfo?(dholbert)
For CSS Grid, our automatic minimum size code was added in bug 1176775 (based on the more up-to-date spec text that we want to match here). We may want to refactor things so that we share some of that code same code for flexbox here, to be sure this works the same & stays in sync between grid & flexbox. Adding dependency.
Depends on: 1176775
Hmm, the "specified size" part is a bit more complicated for flexbox (as compared to grid). For grid items, any percent "specified size" value must necessarily be indefinite, because the percentage gets resolved against the grid area, whose size is unknown because it depends on the item. But for flex items, the percentage gets resolved against the flex container. So percent sizes on flex items may be definite, and we have to do some digging to find out. [1] "specified size" here is defined as: "If the item’s computed main size property is definite, then the specified size is that size [...]" https://drafts.csswg.org/css-flexbox-1/#min-size-auto
Blocks: 1484709
Component: Layout → Layout: Flexbox
Blocks: 1468066
Flags: needinfo?(dholbert)
See Also: → https://github.com/webcompat/web-bugs/issues/22852
Flags: needinfo?(dholbert)
Flags: webcompat?

Migrating Webcompat whiteboard priorities to project flags. See bug 1547409.

Webcompat Priority: --- → ?

See bug 1547409. Migrating whiteboard priority tags to program flags.

Blocks: 1390749
No longer blocks: 1468066
Flags: needinfo?(miket)
Flags: needinfo?(dholbert)
See Also: → https://webcompat.com/issues/40367
Webcompat Priority: ? → P1
Flags: webcompat?
Flags: needinfo?(miket)
Blocks: 1650294
Depends on: 1663822

Flex item's intrinsic size can affect the content size suggestion in the
automatic minimum size resolution, so we shouldn't optimize the reflow
away. https://drafts.csswg.org/css-flexbox-1/#min-size-auto

Add IsFlexItem() condition to the methods to avoid set
IMAGE_SIZECONSTRAINED bit in nsImageFrame::Reflow(). So after
decoding the image and updating its intrinsic size, we'll reflow the
flex item again.

This is needed to pass
testing/web-platform/tests/css/css-flexbox/flex-minimum-width-flex-items-013.html
after updating specified / content size suggestion of the automatic minimum size
resolution in the next part.

Depends on D90452

(WIP) Need to update some tests and test expectations.

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

The transferred size suggestion will be updated to match the spec in bug
1136312.

Depends on D90453

Blocks: 1665532
Assignee: nobody → aethanyc
Status: NEW → ASSIGNED

nsIFrame::ComputeSize() respects UseAutoBSize, so should
ComputeSizeWithIntrinsicDimensions(). This is essential in order to pass
layout/reftests/w3c-css/submitted/flexbox/flexbox-min-height-auto-002{a,c}.html
and flex-aspect-ratio-img-column-011.html [.flexbox 7] after updating specified
/ content size suggestion in a later part.

Depends on D90453

test_dynamic_reflow_root_disallowal.html: in the next part, we can calculate
min-size:auto as the minimum of specified size and content size correctly
regardless of the value of flex-basis. If we are not reset the inline-size
(or block-size) to auto, the flex item's min-width (or min-height) will
always be its specified size (width:10px or height:10px set in
gReflowRootCandidateStyles), and changing inner to the content size from
width:20px to width:40px will have no effect.

flex-minimum-{width,height}-flex-items-005.xht: To make the flex item
resolve min-height:auto as the specified size, the content (image) size
must be larger than it. So here we change the image size to 200x200.
Google Chrome can also pass this test after this patch.

Depends on D90618

Attachment #9176120 - Attachment description: Bug 1316534 Part 3 - Update flex item's specified / content size suggestion implementation to match the spec. → Bug 1316534 Part 5 - Update flex item's specified / content size suggestion implementation to match the spec.

This doesn't change the behavior because we remove the
aMinSizeFallback=true use case in the previous part.

Depends on D90620

Attachment #9176120 - Attachment description: Bug 1316534 Part 5 - Update flex item's specified / content size suggestion implementation to match the spec. → Bug 1316534 Part 5 - Update flex item's specified / content size suggestion implementation to match the spec. r=dholbert

CC'ing @rego who may be interested to take a glance at part 4 which is tweaking some tests that he wrote a while back:
https://phabricator.services.mozilla.com/D90619
(As noted in the phab issue, I'm convinced that the tests don't match the current spec text here; the tests predate this bug's filing & the current spec text around "specified size suggestion" etc.)

WPT.fyi links for the tests that are being changed there (showing that they don't pass anywhere, except one of them in Firefox, but that's probably a mistake that's changing as part of this bug):
https://wpt.fyi/results/css/css-flexbox/flex-minimum-height-flex-items-005.xht
https://wpt.fyi/results/css/css-flexbox/flex-minimum-width-flex-items-005.xht

Attachment #9176118 - Attachment description: Bug 1316534 Part 1 - Removed unused arguments to nsIFrame::ComputeISizeValue() computing LengthPercentage ISize(). → Bug 1316534 Part 1 - Removed unused arguments to nsIFrame::ComputeISizeValue() computing LengthPercentage ISize(). r=dholbert

(In reply to Daniel Holbert [:dholbert] from comment #24)

CC'ing @rego who may be interested to take a glance at part 4 which is tweaking some tests that he wrote a while back:
https://phabricator.services.mozilla.com/D90619
(As noted in the phab issue, I'm convinced that the tests don't match the current spec text here; the tests predate this bug's filing & the current spec text around "specified size suggestion" etc.)

WPT.fyi links for the tests that are being changed there (showing that they don't pass anywhere, except one of them in Firefox, but that's probably a mistake that's changing as part of this bug):
https://wpt.fyi/results/css/css-flexbox/flex-minimum-height-flex-items-005.xht
https://wpt.fyi/results/css/css-flexbox/flex-minimum-width-flex-items-005.xht

Thanks for the heads-up Daniel.

As you mention these tests are very old and no longer match the spec (if the ever did it), so thanks for fixing them.

Attachment #9176119 - Attachment description: Bug 1316534 Part 2 - Mark image flex item's size depending on the its intrinsic size. → Bug 1316534 Part 2 - Mark image flex item's size depending on the its intrinsic size. r=dholbert
Pushed by aethanyc@gmail.com: https://hg.mozilla.org/integration/autoland/rev/1f142e206947 Part 1 - Removed unused arguments to nsIFrame::ComputeISizeValue() computing LengthPercentage ISize(). r=dholbert https://hg.mozilla.org/integration/autoland/rev/0f703c317330 Part 2 - Mark image flex item's size depending on the its intrinsic size. r=dholbert https://hg.mozilla.org/integration/autoland/rev/d606b4e5c08b Part 3 - Make ComputeSizeWithIntrinsicDimensions() respect ComputeSizeFlag::UseAutoBSize. r=dholbert https://hg.mozilla.org/integration/autoland/rev/2de8d429eaad Part 4 - Fix some incorrect flexbox tests. r=dholbert https://hg.mozilla.org/integration/autoland/rev/110cd2ff4214 Part 5 - Update flex item's specified / content size suggestion implementation to match the spec. r=dholbert https://hg.mozilla.org/integration/autoland/rev/4a13cf490237 Part 6 - Update flex item's transferred size suggestion implementation to match the spec. r=dholbert https://hg.mozilla.org/integration/autoland/rev/34aa91a23ea0 Part 7 - Removed the unused aMinSizeFallback in CrossSizeToUseWithRatio(). r=dholbert https://hg.mozilla.org/integration/autoland/rev/fe228ad057f6 Part 8 - Update the comment in ResolveAutoFlexBasisFromRatio(). r=dholbert
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/25689 for changes under testing/web-platform/tests
Whiteboard: [webcompat] → [webcompat], [wptsync upstream]
Upstream PR merged by moz-wptsync-bot

The Webcompat team will double check if all the SeeAlso webcompat issues are solved by this fix.

Ting-Yu, thanks, thanks, thanks AND THANKS! This was a major webcompat issue being solved this week.

status-firefox68: affected → wontfix
Flags: in-testsuite+
Regressions: 1667668

After verifying the issues from the ''See Also" list we came up with the following results:

  • 18 fixed issues
  • 18 issues are still reproducible
  • 3 issues are invalid due to the changed layout or the page not being available anymore.

Thanks!

Re comment 39:

Do you have the list of sites where the issues are still reproducible? I can take a look on them.

Flags: needinfo?(ciprian.ciocan)

Ciprian, thanks for the list!

The webcompat issues are all report on mobile websites, but the lastest Firefox Android are still using geckoview 2020-09-22, which unfortunately doesn't contain the patches yet.

So I go through the list by using desktop Nightly 2020-09-20 (buggy) and Nightly 2020-09-30 (fixed by this bug), and open them in devtools'
Responsive Design Mode with Galaxy S9 screen size. All the sites except https://github.com/webcompat/web-bugs/issues/57946 look good to me on Nightly 2020-09-30, and look broken on Nightly 2020-09-20. I've left a comment on the problematic one for further investigation.

There is one new issue, https://github.com/webcompat/web-bugs/issues/58981, which is still reproducible on mobile Nightly (GV: 83.0a1-20200922), but fixed in the reference browser (GV: 83.0a1-20200929), similar to what Ting-Yu mentioned in comment 42

See Also: → https://github.com/webcompat/web-bugs/issues/58981
Regressions: 1700565
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: