multicol elements are too tall, as flex items in a vertical flex container

NEW
Assigned to

Status

()

Core
Layout
6 years ago
5 years ago

People

(Reporter: flying sheep, Assigned: dholbert)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 4 obsolete attachments)

(Reporter)

Description

6 years ago
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:10.0.2) Gecko/20100101 Firefox/10.0.2
Build ID: 20120217110108

Steps to reproduce:

this is a cryptic effect that i can’t really put my finger on:

it seems that an element with -moz-column-width inside another element with -moz-flex-box layout fails to assume the correct minimum height and ignores the columns if there is no inline- or text-node behind it.

test case: http://jsfiddle.net/flyingsheep/6jwSE/

feel free to experiment yourself. an empty span seems sufficient to circumvent the issue.
(Reporter)

Comment 1

6 years ago
oh, almost forgot: -moz-column-count works fine afaik.
Component: Untriaged → Layout
Product: Firefox → Core
QA Contact: untriaged → layout
(Reporter)

Comment 2

6 years ago
mmh, more info: the inline node breaks the flex box. don’t know if this is intended.

however, there is no way to make column-width work inside a flex-box now.

play around here: http://jsfiddle.net/flyingsheep/gmeqm/
I don't think we really care about how columns and -moz-box interact that much, since no one should be using them together (not least because no one should be using -moz-box) and the interaction of the two is not defined anywhere....
(Reporter)

Comment 4

6 years ago
well, i think “both should continue working” is a safe guess for how they should play together.

and if nobody should build experimental html documents using vendor-prefixed css, why don’t you just remove vendor-prefixed css?

so here are two ideas what could bedone here:

1. fix the bug
2. stop investing into experimental stuff and wait until every spec is final before implementing it
-moz-box is not "experimental stuff".  It's a proprietary completely non-standard extension that's existed for over 12 years now.  It's not based on any spec, and not associated with any spec; its behavior is defined solely by its implementation.  And yes, you shouldn't be using it in your documents and we should not be exposing it to the web....
(Reporter)

Comment 6

6 years ago
well, i just used the parts that are also supported in the spec http://dev.w3.org/csswg/css3-flexbox/

and since these parts are the same in the old and new one, fixing the old one isn’t lost work (guessing that the implementation won’t be thrown overboard and totally reimplemented for the new one)

since there are assumptions in this comment, please correct me if i’m wrong.
(In reply to flying sheep from comment #6)
> and since these parts are the same in the old and new one, fixing the old
> one isn’t lost work (guessing that the implementation won’t be thrown
> overboard and totally reimplemented for the new one)

Your guess is wrong; it's being rewritten from scratch.
(Reporter)

Comment 8

6 years ago
interesting. this must be either a perfectionist working or some seriously better approach, as the most basic part of both implementations (behavior of box-flex) is the same.

i won’t complain, i just hope this bug won’t reappear in the newer flexbox implementation. but i’m still very sure that -moz-column should work together with the new -moz-flexbox in the way i think it should (the columns are populated so that the box has the least height, just like if it wasn’t a box)

i think this one is wontfix then, as the old implementation will go and never return and there is no point in fixing it?
The reason it's being rewritten from scratch is that -moz-box is defined only by its implementation, and various code depends on that implementation.  Making any changes to it will just break consumers for no good reason.  Furthermore, the existing code is ... idiosyncratic due to the things it has to support that are NOT in the spec, and in many cases the spec defines different behavior than what -moz-box has.

And again, that's because -moz-box has nothing to do with the flexbox spec.

> i just hope this bug won’t reappear in the newer flexbox implementation. 

Daniel, want to check that?  ;)

> i’m still very sure that -moz-column should work together with the new -moz-flexbox

Well, yes.

> as the old implementation will go and never return

The old implementation is not going anywhere for a while, since so much extension code depends on it.  We _may_ be able to turn it off for web content, though...  Maybe.
(In reply to Boris Zbarsky (:bz) from comment #9)
> > i just hope this bug won’t reappear in the newer flexbox implementation. 
> 
> Daniel, want to check that?  ;)

I looked into this briefly -- the testcases here seem to require a vertical flexbox, & my implementation so far is horizontal-only.  (I don't yet have support for "flex-direction: column", which is how you say "box-orient: vertical" in new-flexbox-speak.)

@flyingsheep: If you could post a testcase that demonstrates the undesirable -moz-box behavior w/ a horizontal flexbox, I'd be happy to give that a try with the new implementation, though.
(Reporter)

Comment 11

6 years ago
Created attachment 607319 [details]
tests the interaction of the new -moz-flexbox and -moz-column-width (doesn't work because it uses keywords from obsolete spec-version)

is this acceptable as testcase? note that

1. i don’t know if -moz-flexbox can be applied to the body. if not, i’ll update the testcase.

2. it *does* use -moz-flex-flow: column (it only makes sense with vertical boxes)

3. i haven’t tested the test case, so please check it for bugs ;)
(In reply to flying sheep from comment #11)
> 2. it *does* use -moz-flex-flow: column (it only makes sense with vertical
> boxes)

OK -- I can't really test this meaningfully yet, then, because I don't yet have vertical support.

I expect it should work, though. There's no strong reason to expect that ancient -moz-box quirks should persist in the new implementation. :) It'll likely have a few bugs of its own, but it'll be more tightly integrated with our other general layout code, rather than being a XUL transplant, so I'd expect it work correctly for things like this.

In any case, I'll circle back around to test this once I've got vertical support.
(Reporter)

Comment 13

6 years ago
nice, thanks. how about setting the bug for missing vertical mode as blocking for this one (so you won’t have to think of this, as you see it in the blocking list of the vertical-mode-bug)
I don't have a separate bug filed on vertical flexbox yet, but I'll set the main flexbox as blocking this, to help me remember to circle back to this.
Depends on: 666041
Summary: -moz-column-width inside -moz-flex-box fails without inline node behind it → -moz-column-width inside -moz-flex-box fails without inline node behind it (or: be sure columns work inside vertical css flexbox)
Created attachment 666338 [details]
testcase w/ updated keyword-names

Here's this bug's original testcase, with property names/values updated to their final prefixed forms (and including webkit prefixes for comparability), and with "margin: 0" on the body element so that the "height: 100%" doesn't make it always have a scrollbar from a few px of overflow.

With the patch to enable flexbox in bug 783409, this testcase almost renders correctly, but not quite -- the cyan region is taller than it should be.  (It looks like it's 3em tall instead of 1em tall)
OS: Linux → All
Hardware: x86_64 → All
Version: 10 Branch → Trunk
Created attachment 666340 [details]
testcase 2
Assignee: nobody → dholbert
Status: UNCONFIRMED → NEW
Ever confirmed: true
I'm narrowing the focus of this bug to be specifically about the remaining issue at the end of comment 15 -- the vertical-sizing of the multicol flex-item being too tall.
Summary: -moz-column-width inside -moz-flex-box fails without inline node behind it (or: be sure columns work inside vertical css flexbox) → multicol elements are too tall, as flex items in a vertical flex container
Created attachment 666356 [details]
testcase 3: with "align-self" set

This testcase (where both webkit & gecko both make the multicol element a single tall column) helps in showing what's going on.

We need to establish an initial size for the multicol element, and we shrinkwrap it to do so.  For the multicol element in this case, "shrinkwrap" means that it takes on its preferred size, which is the width of a single column.  And given that width, its desired height is ~10em, so that's what we give it.  This testcase has "flex-align: center", and it shows the centered flex item that's got a single column, ~10em tall.

The earlier testcases here are special, though, because they have (default) "align-self: stretch", which means we probably shouldn't be shrinkwrapping them, since we know they're likely to take on a size that's wider than their shrinkwrapped size. (or at least as wide)

For vertical flexboxes, I think we probably shouldn't shrinkwrap, if we've got "align-self: stretch".  I'll attach a WIP patch in a few min to make that change.  It passes existing flexbox reftests, and I'll include some new tests when requesting review.

BTW, I don't think the flexbox spec really covers this situation.  I believe our current behavior actually matches the spec, but I don't think that behavior will be what authors expect or want.
Created attachment 666357 [details] [diff] [review]
patch v1

This patch makes us render "testcase w/ updated keyword-names" and "testcase 2" as the author would probably expect, with the multicol element only being ~1em tall.
Attachment #607319 - Attachment description: tests the interaction of the new -moz-flexbox and -moz-column-width → tests the interaction of the new -moz-flexbox and -moz-column-width (doesn't work because it uses keywords from obsolete spec-version)
Attachment #607319 - Attachment is obsolete: true
I started a www-style thread to get clarification on what the correct behavior should be for this: http://lists.w3.org/Archives/Public/www-style/2012Oct/0017.html

Comment 21

5 years ago
Should this bug depend on Bug 811024? Is Bug 702508 also of concern here?
(In reply to Florian Bender from comment #21)
> Should this bug depend on Bug 811024?

No - that one is about flexbox as a child of a multicol element. This bug here is about the reverse. (multicol element as a child of flexbox.

> Is Bug 702508 also of concern here?

No - it's orthogonal.
Also, the testcases here need updating; they use prefixed keywords, which we no longer support. (so they aren't actually exercising flexbox code anymore). needinfo=me to fix that later today
Flags: needinfo?(dholbert)

Comment 24

5 years ago
That seems easy enough for me to do, if you don't mind, I'll do that in a few hours!
Flags: needinfo?(dholbert)
That would be great, thanks!
Flags: needinfo?(fb+mozdev)

Comment 26

5 years ago
I'm working on the testcases now, meanwhile comparing spec for changes in syntax. 

Should I keep the -webkit- prefixes? There's still a lot of implementations out there using the prefix (according to caniuse). Chrome 29 unprefixed this.

(Thanks BTW for your answers on other bugs; I'd rather not bugspam them ;) )
Flags: needinfo?(dholbert)
(In reply to Florian Bender from comment #26)
> I'm working on the testcases now, meanwhile comparing spec for changes in
> syntax.

I think just replacing "-moz-flex" with "flex" and "-moz-flex-direction" with "flex-direction" should be sufficient.

> Should I keep the -webkit- prefixes? There's still a lot of implementations
> out there using the prefix (according to caniuse).

Sure, but don't worry about it too much.  (It's somewhat handy for being able to compare to a larger number of other browsers' rendering.)
Flags: needinfo?(dholbert)

Comment 28

5 years ago
Created attachment 807958 [details]
moz-column-in-flexbox.html
Attachment #807958 - Flags: feedback?(dholbert)

Comment 29

5 years ago
Created attachment 807960 [details]
testcase-2.html
Attachment #807960 - Flags: feedback?(dholbert)

Comment 30

5 years ago
Created attachment 807961 [details]
testcase-3.html
Attachment #807961 - Flags: feedback?(dholbert)
(Could you change the mimetype of those attachments to text/html? not sure why bugzilla didn't autodetect that for you.)
Attachment #807961 - Attachment mime type: text/plain → text/html
Attachment #807961 - Flags: feedback?(dholbert)
Attachment #807958 - Attachment mime type: text/plain → text/html
Attachment #807960 - Attachment mime type: text/plain → text/html

Comment 32

5 years ago
So here we go: 

– unprefixed flexbox properties (keeping -webkit-)
– added unprefixed column-* properties (future proofing)
– fixed encoding issue (at least on my machine, all files UTF-8 now)
– "fixed" whitespace

I hope that's ok. If I should change anything else or revert some changes, just let me know!

(I could not mark the old testcases as obsolete, I guess I need editbugs privs for that.)

I actually set the attachments as text/html when uploading, not sure why Bugzilla ignores me here. I'll fix that in a second.
(In reply to Daniel Holbert [:dholbert] from comment #31)
> (Could you change the mimetype of those attachments to text/html? not sure
> why bugzilla didn't autodetect that for you.)

actually it's easy enough, so I just went ahead and did it. :)

And it looks like (with the new keywords) our behavior hasn't changed since comment 17. (so, the bug still remains)

Thanks for fixing the testcases!

Comment 34

5 years ago
Sure, no problem. I'll file a bug for bugzilla, I encountered the mime issue somewhere before …
Flags: needinfo?(fb+mozdev)
You need to log in before you can comment on or make changes to this bug.