1092007 - For flex items in a vertical flex container, only treat heights as definite (for resolving % heights on children) if the flex-basis (or flex container's height) is also definite

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[Daniel Holbert [:dholbert]]

Per this www-style post[1], we should only be treating flex items' heights as definite (i.e. usable for resolving percent heights on *their* children) if the flex item has a definite flex basis.

This is specced here (note the "definite flex basis" requirement):
  # If a percentage is going to be resolved against
  # a flex item’s main size, and the flex item has
  # a definite flex basis, the main size must be
  # treated as definite for the purpose of resolving
  # the percentage, and the percentage must resolve
  # against the flexed main size of the flex item
http://dev.w3.org/csswg/css-flexbox/#definite-sizes

(I believe the reason for this is: if we have an indefinite flex basis (flex-basis:auto, effectively), then we'll derive the flex-basis from our children, and in the trivial case where the flex container & its children are all auto-sized, we'll end up being the height of the flex item's child. And then the flex item will end up resolving its final height as being a percent of its auto-height, which is kinda broken. At least, that's how I can rationalize the specced behavior.)

[1] http://lists.w3.org/Archives/Public/www-style/2014Jul/0010.html

Comment 1

[Daniel Holbert [:dholbert]]

To be clear: currently, I believe we treat flex items' heights as definite (usable for resolving percent heights) *regardless* of their flex-basis value.  That goes against the spec, as discussed in comment 0.

This came up in https://code.google.com/p/chromium/issues/detail?id=428049 , where I think Chrome is doing the right thing & we're doing the wrong thing.

Comment 2

[Daniel Holbert [:dholbert]]

Testcase (taken & modified, from the chrome bug report): http://jsfiddle.net/mLkp7L1j/4/

Comment 3

[Karl Dubost💡 :karlcow]

Possibly an example of this in WebCompat bug 893
https://github.com/webcompat/web-bugs/issues/893

Comment 4

[Daniel Holbert [:dholbert]]

The conditions have been made a bit more strict now - see bug 1161771. (Might make sense to just fix both of these together.)

Comment 5

[Leif Arne Storset]

Attachment: A somewhat minimized TC for Rust Playpen issue #112 (prose inside the testcase is now inaccurate)

I think this is the same issue I'm seeing in this attached minimized version of the Rust Playpen.

Originally we thought this was a Chromium bug (unwanted behavior started in Chromium 43), but now I think Rust Playpen should instead give the <main> flex item/flexbox a definite size. Discussion at https://github.com/rust-lang/rust-playpen/issues/112 .

Comment 6

[fantasai]

The CSSWG resolved to update the spec to (as far as we can tell) match Firefox's behavior here, since both Firefox and Edge have this behavior and it seems to be what authors expect.

Comment 7

[fantasai]

See https://github.com/w3c/csswg-drafts/issues/1679 for details.

Comment 8

[Brian Birtles (:birtles)]

Daniel, can we close this now that the spec changes have been merged?

Comment 9

[Daniel Holbert [:dholbert]]

We still don't quite match the spec (despite comment 6 :) ).  As I recall, we treat flex-item heights (in a vertical flex container) as *always* definite, but the spec still places some restrictions (though it's looser than it used to be).

Here's the spec text that is relevant here (to main sizes), in the latest spec ED:
    2. If the flex container has a definite main size,
       a flex item’s post-flexing main size is treated as definite,
       even though it can rely on the indefinite sizes of any
       flex items in the same line.
    [...]
    Note: The main size of a fully inflexible item with a definite flex basis is, by definition, definite.
https://drafts.csswg.org/css-flexbox/#definite-sizes

Our behavior *is* correct on the attached testcase, though, FWIW (at least as far as the height of the blue area -- I didn't look further than that).

I've updated the title of this bug and the testcase to reflect the new state of things.

Comment 10

[Sam Sneddon [:gsnedders]]

wpt/css/css-flexbox/percentage-heights-003.html now has a test for this, and fails in Firefox due to everything being treated as definite.

Comment 13

[Daniel Holbert [:dholbert]]

wpt.fyi link for the test mentioned in comment 10: https://wpt.fyi/results/css/css-flexbox/percentage-heights-003.html

Comment 14

[Firefox Bug Husbandry Bot]

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

Comment 15

[Firefox Bug Husbandry Bot]

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

Comment 17

[Daniel Holbert [:dholbert]]

We should try to fix this soonish, given the webcompat issues involved here. Taking.

Comment 18

[Mike Taylor [:miketaylr]]

(note that LinkedIn fixed their home page)

Comment 19

[Daniel Holbert [:dholbert]]

Attachment: Bug 1092007 part 2: Treat a flex item's main-size as indefinite if the item and the container both have an indefinite size in that axis. r?mats

Comment 20

[Daniel Holbert [:dholbert]]

Attachment: Bug 1092007 part 1: Adjust some in-content CSS to account for flex definite-sizing change. r?johannh

As of this bug, flex items in a vertical flex container will sometimes be
considered to have "indefinite" sizes, i.e. percent sizes in them will no
longer resolve. To work around this, they need to provide a definite flex-basis
(e.g. as part of the "flex" shorthand property) if we want percent sizes to
resolve (instead of being treated as "auto") inside of them.

Comment 21

[Daniel Holbert [:dholbert]]

The "part 1" patch here is needed in order to avoid failing this test:
https://searchfox.org/mozilla-central/source/browser/components/sessionstore/test/browser_590563.js

(johannh, I hope you don't mind reviewing that change, as someone who's worked on the file in question, info-pages.inc.css.)

Without part 1's tweak to info-pages.inc.css, the "main" patch (part 2) makes us fail some sizing checks in browser_590563.js, because about:sessionrestore ends up rendering with a 0-height "list of tabs to restore" area. That area is actually a height:100% element inside of a flex item in a vertical flex container, and the flex item has the default flex-basis:auto and the flex container is also auto-sized, which means the flex item's height is now considered indefinite (as of this bug), which means the height:100% ends up behaving as height:auto and collapsing away to nothing because it's got no content. (This is the new way that this markup is supposed to render.)

My patch in part 1 changes to give that flex item a definite flex basis (0px), which makes its final size be considered definite as well, which means the height:100% can resolve against the final size. It won't actually end up being 0px in height, because (a) it's got a min-height:12em, and (b) it's still got a flex-grow of 1 so it'll absorb any excess space that its flex container has.

Comment 22

[Daniel Holbert [:dholbert]]

(AFAICT from some local testing, part 1 doesn't change the current rendering of about:sessionrestore. It simply prevents it from breaking when I apply part 2.)

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6063bd9bccb1
part 1: Adjust some in-content CSS to account for flex definite-sizing change. r=johannh
https://hg.mozilla.org/integration/autoland/rev/55e700b5f42e
part 2: Treat a flex item's main-size as indefinite if the item and the container both have an indefinite size in that axis. r=mats

Comment 24

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

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

Comment 25

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

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 26

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/6063bd9bccb1
https://hg.mozilla.org/mozilla-central/rev/55e700b5f42e

Comment 27

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

Upstream PR merged by moz-wptsync-bot

Comment 30

[Daniel Holbert [:dholbert]]

Good news! I've gone through the list of associated webcompat issues and dependencies/dupes, and I've verified that they are all indeed fixed in latest Nightly, version 71.0a1 (2019-09-06) (64-bit)

Also worth noting: for some of the testcases that were posted here early-on, the testcases' expectations are now incorrect, due to subsequent spec changes (e.g. comment 6 and comment 9).

In particular, the linked jsfiddle (http://jsfiddle.net/mLkp7L1j/4/ ) and the "somewhat minimized TC" attachment (https://bug1092007.bmoattachments.org/attachment.cgi?id=8663193 ) didn't actually change their rendering as part of this bug, since they predated additional spec changes. But we do match Chrome's rendering on both of those testcases, so our (unchanged) rendering on both of those should be OK from a compat & spec-compliance perspective.

Here's a better "real" testcase for the final incarnation of the spec change that this bug covered: attachment 8601759 [details] (from duplicate bug 1161771). That testcase did change its rendering when this patch landed (such that the red area is now only as tall as the text, instead of being as tall as its container).

Comment 31

[Daniel Holbert [:dholbert]]

Attachment: Bug 1092007 followup: fix typo in code comment. rs=mats via review feedback

DONTBUILD because this is comment-only.

This patch is just addressing a typo in a code comment that was left as a
review nit, which I forgot to address before landing.

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fa3d10d81e64
followup: fix typo in code comment. rs=mats via review feedback

Comment 33

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/fa3d10d81e64