Open
Bug 734525
Opened 13 years ago
Updated 2 years ago
multicol elements are too tall, as flex items in a vertical flex container
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
NEW
People
(Reporter: flying-sheep, Assigned: dholbert)
References
Details
Attachments
(4 files, 4 obsolete files)
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•13 years ago
|
||
oh, almost forgot: -moz-column-count works fine afaik.
Updated•13 years ago
|
Component: Untriaged → Layout
Product: Firefox → Core
QA Contact: untriaged → layout
Reporter | ||
Comment 2•13 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/
Comment 3•13 years ago
|
||
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•12 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
Comment 5•12 years ago
|
||
-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•12 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•12 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?
Comment 9•12 years ago
|
||
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.
Assignee | ||
Comment 10•12 years ago
|
||
(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•12 years ago
|
||
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 ;)
Assignee | ||
Comment 12•12 years ago
|
||
(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•12 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)
Assignee | ||
Comment 14•12 years ago
|
||
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: css3-flexbox
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)
Assignee | ||
Updated•12 years ago
|
Assignee | ||
Updated•12 years ago
|
Assignee | ||
Comment 15•12 years ago
|
||
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)
Assignee | ||
Updated•12 years ago
|
OS: Linux → All
Hardware: x86_64 → All
Version: 10 Branch → Trunk
Assignee | ||
Comment 16•12 years ago
|
||
Assignee: nobody → dholbert
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee | ||
Comment 17•12 years ago
|
||
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
Assignee | ||
Comment 18•12 years ago
|
||
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.
Assignee | ||
Comment 19•12 years ago
|
||
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.
Assignee | ||
Updated•12 years ago
|
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
Assignee | ||
Comment 20•12 years ago
|
||
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•11 years ago
|
||
Should this bug depend on Bug 811024? Is Bug 702508 also of concern here?
Assignee | ||
Comment 22•11 years ago
|
||
(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.
Assignee | ||
Comment 23•11 years ago
|
||
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•11 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)
Comment 26•11 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)
Assignee | ||
Comment 27•11 years ago
|
||
(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•11 years ago
|
||
Attachment #807958 -
Flags: feedback?(dholbert)
Comment 29•11 years ago
|
||
Attachment #807960 -
Flags: feedback?(dholbert)
Comment 30•11 years ago
|
||
Attachment #807961 -
Flags: feedback?(dholbert)
Assignee | ||
Comment 31•11 years ago
|
||
(Could you change the mimetype of those attachments to text/html? not sure why bugzilla didn't autodetect that for you.)
Assignee | ||
Updated•11 years ago
|
Attachment #807961 -
Attachment mime type: text/plain → text/html
Attachment #807961 -
Flags: feedback?(dholbert)
Assignee | ||
Updated•11 years ago
|
Attachment #807958 -
Attachment mime type: text/plain → text/html
Assignee | ||
Updated•11 years ago
|
Attachment #807960 -
Attachment mime type: text/plain → text/html
Comment 32•11 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.
Assignee | ||
Comment 33•11 years ago
|
||
(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!
Assignee | ||
Updated•11 years ago
|
Attachment #666338 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #666340 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #666356 -
Attachment is obsolete: true
Comment 34•11 years ago
|
||
Sure, no problem. I'll file a bug for bugzilla, I encountered the mime issue somewhere before …
Flags: needinfo?(fb+mozdev)
Assignee | ||
Updated•11 years ago
|
Attachment #807958 -
Flags: feedback?(dholbert)
Assignee | ||
Updated•11 years ago
|
Attachment #807960 -
Flags: feedback?(dholbert)
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•