Closed Bug 1092007 Opened 7 years ago Closed 2 years ago

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

Categories

(Core :: Layout, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla71
Webcompat Priority P1
Tracking Status
firefox68 --- wontfix
firefox71 --- fixed

People

(Reporter: dholbert, Assigned: dholbert)

References

(Blocks 2 open bugs, )

Details

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

Attachments

(4 files)

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
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.
OS: Linux → All
Hardware: x86_64 → All
Testcase (taken & modified, from the chrome bug report): http://jsfiddle.net/mLkp7L1j/4/
Possibly an example of this in WebCompat bug 893
https://github.com/webcompat/web-bugs/issues/893
The conditions have been made a bit more strict now - see bug 1161771. (Might make sense to just fix both of these together.)
See Also: → 1161771
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 .
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.
Daniel, can we close this now that the spec changes have been merged?
Flags: needinfo?(dholbert)
Attachment #8663193 - Attachment description: A somewhat minimized TC for Rust Playpen issue #112 → A somewhat minimized TC for Rust Playpen issue #112 (prose inside the testcase is now inaccurate)
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.
Flags: needinfo?(dholbert)
Summary: For flex items in a vertical flex container, only treat heights as definite (for resolving % heights on children) if the flex-basis is definite → 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
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.
Duplicate of this bug: 1161771
Duplicate of this bug: 1525083
Flags: webcompat?
Whiteboard: [webcompat]

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

Webcompat Priority: --- → ?

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

Duplicate of this bug: 1279276
Blocks: 1564261

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

Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Priority: -- → P2
Blocks: 1559917

(note that LinkedIn fixed their home page)

Webcompat Priority: ? → P1
Flags: webcompat?
Attachment #9088601 - Attachment description: Bug 1092007: Treat a flex item's main-size as indefinite if the item and the container both have an indefinite size in that axis. → Bug 1092007: 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
Blocks: 1578586

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.

Attachment #9088601 - Attachment description: Bug 1092007: 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 → 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

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.

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

Pushed by dholbert@mozilla.com:
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
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/18876 for changes under testing/web-platform/tests
Whiteboard: [webcompat] → [webcompat], [wptsync upstream]
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla71
Upstream PR merged by moz-wptsync-bot
No longer blocks: 1559917
Duplicate of this bug: 1559917
Duplicate of this bug: 1564261
No longer blocks: 1564261

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

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.

Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fa3d10d81e64
followup: fix typo in code comment. rs=mats via review feedback
Blocks: 1611303
Blocks: 1654044
You need to log in before you can comment on or make changes to this bug.