Closed Bug 921522 Opened 6 years ago Closed 6 years ago

CSS flexible boxes with min-height not vertically centered when they should be

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27
Tracking Status
firefox26 --- verified
firefox27 --- fixed

People

(Reporter: mrflix, Assigned: dholbert)

References

()

Details

(Whiteboard: [good first verify])

Attachments

(4 files, 3 obsolete files)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_8_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/29.0.1547.65 Safari/537.36

Steps to reproduce:

Tested in Firefox 24.0 and 27.0a1 (2013-09-27).

Added 100% height to <html> and <body>
Added flex with column direction and justify-content: center to an <article> with min-height




Actual results:

Child <h1> of <article> stayed on top


Expected results:

Child <h1> should be vertically centered.

Use-case: website with several pages. Some pages have more content which is exceeding the screen-size (> 100% height), some pages are smaller (< 100% height). When they are smaller, I want to center the content vertically.

Test: http://www.felixniklas.de/bug_reports/ff_flex_min-height/
Works in Chrome.
Component: Untriaged → Layout
Product: Firefox → Core
Summary: CSS flexible boxes with min-height → CSS flexible boxes with min-height not vertically centered when they should be
Daniel?
Flags: needinfo?(dholbert)
Looks like a bug. Here's a reduced testcase, comparing two flex containers side-by-side. The first gets its height from "min-height"; the second gets it from "height".
Assignee: nobody → dholbert
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(dholbert)
Yeah, the faulty code is here:

1504   mPackingSpaceRemaining = GET_MAIN_COMPONENT(aAxisTracker,
1505                                               aReflowState.ComputedWidth(),
1506                                               aReflowState.ComputedHeight());
1507   if (mPackingSpaceRemaining == NS_UNCONSTRAINEDSIZE) {
1508     mPackingSpaceRemaining = 0;
1509   } else {
    [code that divide up packing space ...]

http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFlexContainerFrame.cpp?mark=1499-1508#1499

And we're taking that "= 0" code-path in this case, because we're auto-height.

We shouldn't be going to the reflow-state here; we already know the (correctly-min/max-clamped) content-box main-size of our flex container, so we should just make that available to this code and have it use that instead of the reflow state.
Attached patch wip (obsolete) — Splinter Review
Haven't tested this; my local build is rebuilding from scratch, because I did a "hg pull -u" earlier, but I think this should basically do it.
OS: Mac OS X → All
Hardware: x86 → All
Version: 27 Branch → Trunk
Yup, that patch fixes the URL and the attached testcase, and it doesn't break any reftests, so it should be good.

I'll repost with reftests, for review.
Attached patch fix v1 (obsolete) — Splinter Review
This fixes it. This also forks two existing justify-content reftests (one vertical, one horizontal), to assert that...
 - in the vertical testcase, setting "min-height" should produce the same as "height"
 - in the horizontal testcase, setting "min-width"-combined-with-"float:left" (to keep auto-width from stretching to fill the screen) should produce the same result as "width".
Attachment #811475 - Attachment is obsolete: true
Attachment #811482 - Flags: review?(bzbarsky)
Attached patch diff -w (obsolete) — Splinter Review
Here's the output of "hg qdiff -w", for reference (since part of the code-change is removing an "if/else" check which means we're de-indenting some code.)
[FWIW: The included reftest "flexbox-justify-content-vert-1b.xhtml" is the one that's analogous to this bug's posted testcase & the URL field, and that reftest fails before the patch is applied.

The other new reftest, flexbox-justify-content-horiz-1b.xhtml, passes with or without the patch, because we can never end up with a reflowState.ComputedWidth() of NS_UNCONSTRAINEDSIZE, which means we can't hit this bug for horizontal flex containers. I'm just including this horizontal reftest for symmetry/completeness.]
That was fast - I'm impressed guys. Thanks for taking care of it, Daniel!
Attached patch fix v2Splinter Review
(Oops - init list was out of order, which triggered build warning and FAIL_ON_WARNINGS on mac & linux. Fixed here.)
Attachment #811482 - Attachment is obsolete: true
Attachment #811483 - Attachment is obsolete: true
Attachment #811482 - Flags: review?(bzbarsky)
Attachment #811538 - Flags: review?(bzbarsky)
Attached patch diff -wSplinter Review
Comment on attachment 811538 [details] [diff] [review]
fix v2

Actually, sorry - mats has looked at this code recently for some other reviews, so this will probably be easier for him to review. (but bz, feel free to steal it back if you want. :))

(Also: I think we should backport this fix to Aurora, if possible. It's a very targeted safe patch for a clearly-doing-the-wrong-thing-in-a-newish-feature sort of issue.  I'll request backport approval once this has landed on trunk.)
Attachment #811538 - Flags: review?(bzbarsky) → review?(matspal)
Here's a Try run with the new patch, showing green reftest results on the platforms that hit FAIL_ON_WARNINGS bustage in comment 8: https://tbpl.mozilla.org/?tree=Try&rev=2d31b86dcf1c
Comment on attachment 811538 [details] [diff] [review]
fix v2

Looks good to me, r=mats

>+  for (uint32_t i = 0; i < aItems.Length(); i++) {
>+    nscoord itemMarginBoxMainSize =
>+      aItems[i].GetMainSize() +
>+      aItems[i].GetMarginBorderPaddingSizeInAxis(aAxisTracker.GetMainAxis());
>+    mPackingSpaceRemaining -= itemMarginBoxMainSize;
>   }
> 
>   if (mPackingSpaceRemaining > 0) {
>     for (uint32_t i = 0; i < aItems.Length(); i++) {
>       mNumAutoMarginsInMainAxis += aItems[i].GetNumAutoMarginsInAxis(mAxis);
>     }
>   }

I'm not sure if it's common that mPackingSpaceRemaining ends up
being > 0, but I'll just note that it would be possible to 
avoid the second loop if you count the auto-margins in the
first loop instead, using a local var, and then just assign
mNumAutoMarginsInMainAxis to the local var inside the 'if'.

In that case it might also be worth putting the item in a local:
  const FlexItem& item = aItems[i];
and using that instead of doing three separate aItems[i].
Attachment #811538 - Flags: review?(matspal) → review+
(In reply to Mats Palmgren (:mats) from comment #15)
> I'm not sure if it's common that mPackingSpaceRemaining ends up
> being > 0

It's not uncommon; just depends on the flexbox use-case.

> but I'll just note that it would be possible to 
> avoid the second loop if you count the auto-margins in the
> first loop instead, using a local var, and then just assign
> mNumAutoMarginsInMainAxis to the local var inside the 'if'.

Good call! I'll spin off a followup bug to make that change.
Landed on inbound:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/f4708377f788

(Depending on merge mechanics, this should make it into Firefox Nightly in ~2 days.)
Flags: in-testsuite? → in-testsuite+
Blocks: 922410
Requesting aurora approval, since this is a safe, targeted fix for spec-compliance (and interop) in a situation where we're clearly doing the wrong thing.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 666041 (CSS3 flexbox)

User impact if declined: incorrect rendering of content that works in other browsers.[1]

Testing completed (on m-c, etc.): Local testing, Try runs, and several hours' worth of TBPL cycles on inbound. I verified that the testcases posted/linked here are fixed, and I've included reftests with the patch.

Risk to taking this patch (and alternatives if risky): extremely low; very targeted fix, making us use the already-resolved (and min/max-clamped) copy of the container height instead of a possibly-unresolved (and unclamped if "auto") version.

String or IDL/UUID changes made by this patch: none

[1]  (specifically, without this patch, we'll do the wrong thing when "min-height" is set on an auto-height flex container, and auto margins or the "justify-content" property are used to apportion packing space around flex items.
Attachment #812438 - Flags: approval-mozilla-aurora?
Attachment #812438 - Attachment description: fix, backported to aurora (just minor bitrot fixes) → fix, backported to aurora (tweaked to fix bitrot)
Depends on: css3-flexbox
https://hg.mozilla.org/mozilla-central/rev/f4708377f788
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Attachment #812438 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Keywords: verifyme
Verified as fixed on the latest Aurora 26.0a2: - the CSS flexible boxed with min-height are vertically centered (used the test cases provided in the Description and in Comment 2).

Verified on Windows 7, Ubuntu 12.04 and Mac OS X 10.8:
Mozilla/5.0 (Windows NT 6.1; rv:26.0) Gecko/20100101 Firefox/26.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:26.0) Gecko/20100101 Firefox/26.0
Mozilla/5.0 (X11; Linux i686; rv:26.0) Gecko/20100101 Firefox/26.0
Build ID: 20131018004002
Whiteboard: [good first verify]
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.