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

RESOLVED FIXED in Firefox 26

Status

()

Core
Layout
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Felix Niklas, Assigned: dholbert)

Tracking

Trunk
mozilla27
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox26 verified, firefox27 fixed)

Details

(Whiteboard: [good first verify], URL)

Attachments

(4 attachments, 3 obsolete attachments)

(Reporter)

Description

4 years ago
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.
(Reporter)

Updated

4 years ago

Updated

4 years ago
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)
(Assignee)

Comment 2

4 years ago
Created attachment 811474 [details]
testcase (comparing "min-height" to "height" on container)

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

Comment 3

4 years ago
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.
(Assignee)

Comment 4

4 years ago
Created attachment 811475 [details] [diff] [review]
wip

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

Updated

4 years ago
OS: Mac OS X → All
Hardware: x86 → All
Version: 27 Branch → Trunk
(Assignee)

Comment 5

4 years ago
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.
(Assignee)

Comment 6

4 years ago
Created attachment 811482 [details] [diff] [review]
fix v1

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

Comment 7

4 years ago
Created attachment 811483 [details] [diff] [review]
diff -w

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

Comment 8

4 years ago
Try run: https://tbpl.mozilla.org/?tree=Try&rev=669e5b1eaa0f
Flags: in-testsuite?
(Assignee)

Comment 9

4 years ago
[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.]
(Reporter)

Comment 10

4 years ago
That was fast - I'm impressed guys. Thanks for taking care of it, Daniel!
(Assignee)

Comment 11

4 years ago
Created attachment 811538 [details] [diff] [review]
fix v2

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

Comment 12

4 years ago
Created attachment 811540 [details] [diff] [review]
diff -w
(Assignee)

Comment 13

4 years ago
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)
(Assignee)

Comment 14

4 years ago
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+
(Assignee)

Comment 16

4 years ago
(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.
(Assignee)

Comment 17

4 years ago
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+
(Assignee)

Updated

4 years ago
Blocks: 922410
(Assignee)

Comment 18

4 years ago
Created attachment 812438 [details] [diff] [review]
fix, backported to aurora (tweaked to fix bitrot)

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?
(Assignee)

Updated

4 years ago
Attachment #812438 - Attachment description: fix, backported to aurora (just minor bitrot fixes) → fix, backported to aurora (tweaked to fix bitrot)
(Assignee)

Updated

4 years ago
Depends on: 666041
https://hg.mozilla.org/mozilla-central/rev/f4708377f788
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Attachment #812438 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/5532bfeb1a17
status-firefox26: --- → fixed
status-firefox27: --- → fixed

Updated

4 years ago
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
status-firefox26: fixed → verified

Updated

4 years ago
Whiteboard: [good first verify]

Updated

4 years ago
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.