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

Status: NEW

(Core :: Layout, defect)

Description

[flying sheep]

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.

Comment 1

[flying sheep]

oh, almost forgot: -moz-column-count works fine afaik.

Comment 2

[flying sheep]

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/

Comment 3

[Boris Zbarsky [:bzbarsky]]

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

Comment 4

[flying sheep]

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

Comment 5

[Boris Zbarsky [:bzbarsky]]

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

Comment 6

[flying sheep]

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.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 8

[flying sheep]

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?

Comment 9

[Boris Zbarsky [:bzbarsky]]

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.

Comment 10

[Daniel Holbert [:dholbert]]

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

Comment 11

[flying sheep]

Attachment: 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 ;)

Comment 12

[Daniel Holbert [:dholbert]]

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

Comment 13

[flying sheep]

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)

Comment 14

[Daniel Holbert [:dholbert]]

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.

Comment 15

[Daniel Holbert [:dholbert]]

Attachment: 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)

Comment 16

[Daniel Holbert [:dholbert]]

Attachment: testcase 2

Comment 17

[Daniel Holbert [:dholbert]]

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.

Comment 18

[Daniel Holbert [:dholbert]]

Attachment: 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.

Comment 19

[Daniel Holbert [:dholbert]]

Attachment: 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.

Comment 20

[Daniel Holbert [:dholbert]]

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

[Florian Bender]

Should this bug depend on Bug 811024? Is Bug 702508 also of concern here?

Comment 22

[Daniel Holbert [:dholbert]]

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

Comment 23

[Daniel Holbert [:dholbert]]

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

Comment 24

[Florian Bender]

That seems easy enough for me to do, if you don't mind, I'll do that in a few hours!

Comment 25

[Daniel Holbert [:dholbert]]

That would be great, thanks!

Comment 26

[Florian Bender]

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

Comment 27

[Daniel Holbert [: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.)

Comment 28

[Florian Bender]

Attachment: moz-column-in-flexbox.html

Comment 29

[Florian Bender]

Attachment: testcase-2.html

Comment 30

[Florian Bender]

Attachment: testcase-3.html

Comment 31

[Daniel Holbert [:dholbert]]

(Could you change the mimetype of those attachments to text/html? not sure why bugzilla didn't autodetect that for you.)

Comment 32

[Florian Bender]

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.

Comment 33

[Daniel Holbert [:dholbert]]

(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

[Florian Bender]

Sure, no problem. I'll file a bug for bugzilla, I encountered the mime issue somewhere before …