Closed
Bug 921522
Opened 11 years ago
Closed 11 years ago
CSS flexible boxes with min-height not vertically centered when they should be
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla27
People
(Reporter: mrflix, Assigned: dholbert)
References
()
Details
(Whiteboard: [good first verify])
Attachments
(4 files, 3 obsolete files)
712 bytes,
text/html
|
Details | |
13.19 KB,
patch
|
MatsPalmgren_bugz
:
review+
|
Details | Diff | Splinter Review |
12.76 KB,
patch
|
Details | Diff | Splinter Review | |
13.25 KB,
patch
|
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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•11 years ago
|
Updated•11 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
Assignee | ||
Comment 2•11 years ago
|
||
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•11 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•11 years ago
|
||
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•11 years ago
|
OS: Mac OS X → All
Hardware: x86 → All
Version: 27 Branch → Trunk
Assignee | ||
Comment 5•11 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•11 years ago
|
||
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•11 years ago
|
||
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•11 years ago
|
||
Flags: in-testsuite?
Assignee | ||
Comment 9•11 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•11 years ago
|
||
That was fast - I'm impressed guys. Thanks for taking care of it, Daniel!
Assignee | ||
Comment 11•11 years ago
|
||
(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•11 years ago
|
||
Assignee | ||
Comment 13•11 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•11 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 15•11 years ago
|
||
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•11 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•11 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 | ||
Comment 18•11 years ago
|
||
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•11 years ago
|
Attachment #812438 -
Attachment description: fix, backported to aurora (just minor bitrot fixes) → fix, backported to aurora (tweaked to fix bitrot)
Assignee | ||
Updated•11 years ago
|
Depends on: css3-flexbox
Comment 19•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Updated•11 years ago
|
Attachment #812438 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 20•11 years ago
|
||
status-firefox26:
--- → fixed
status-firefox27:
--- → fixed
Comment 21•11 years ago
|
||
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
Updated•11 years ago
|
Whiteboard: [good first verify]
You need to log in
before you can comment on or make changes to this bug.
Description
•