Change handling of internal table types inside of a flex container

RESOLVED FIXED in Firefox 47

Status

()

Core
Layout
RESOLVED FIXED
3 years ago
11 months ago

People

(Reporter: dholbert, Assigned: mats)

Tracking

(Blocks: 2 bugs)

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

Firefox Tracking Flags

(firefox42 affected, firefox47 fixed)

Details

Attachments

(3 attachments)

(Reporter)

Description

3 years ago
Quoting https://lists.w3.org/Archives/Public/www-style/2015Jul/0246.html :
=============
The spec currently requires that we do box fixup before determining
flex items:
   http://www.w3.org/TR/2015/WD-css-flexbox-1-20150514/#flex-items
This was discussed originally in this thread:
   [1] https://lists.w3.org/Archives/Public/www-style/2013May/0508.html
   [2] https://lists.w3.org/Archives/Public/www-style/2013Jul/0462.html
   [3] https://lists.w3.org/Archives/Public/www-style/2013Jul/0473.html

Chrome doesn't follow the spec in this case--it turns table cells into
flex items. Which, as I had pointed out [2], has the nice ability to
create a fallback rendering. The argument for not changing it was that
nobody cares. [3]

However, at least some people do care: see e.g. slide 39, which despite
the fact that it would result in layouts that only work properly in
Chrome, uses this technique:
   http://zomigi.com/downloads/Enhancing-Responsiveness-with-Flexbox_CSS-Day_150612.pdf

Should we change the spec to make internal table display types just
turn themselves into flex items, instead of triggering anonymous box
generation?

~fantasai

=============


If the CSSWG ends up resolving to change this, I *think* all we'd need to do is remove the opt-out for the various table display types here:
 http://mxr.mozilla.org/mozilla-central/source/layout/style/nsStyleContext.cpp?rev=6ec03d4b3358#687
(Reporter)

Comment 1

3 years ago
The CSSWG did indeed resolve to change this.
Quoting minutes from IRC at http://krijnhoetmer.nl/irc-logs/css/20150825#l-630 :
=============
# [11:07] <dael> fantasai: Last one, if you have a flex container and you put two table cells in it, they won't become flex items independantly. They'll wrap in an anon table and that will be flex. Chrome has impl so that each item is flex.
# [11:07] <dael> dbaron: They do anon box after?
# [11:08] <dael> fantasai: Not at all.
# [11:08] <dael> dbaron: So it turns the table cells into blocks.
[...]
# [11:17] <dael> RESOLVED: Just blockify the children of flex and grid containers. Don't do anon box fixup
=============
(Reporter)

Updated

2 years ago
Duplicate of this bug: 1252733
(Assignee)

Comment 3

2 years ago
I can take a look at this since the code is shared with Grid,
and the Grid spec (now) has the exact same note:
https://drafts.csswg.org/css-grid/#grid-items
Assignee: nobody → mats
(Reporter)

Comment 4

2 years ago
Thanks!
(Assignee)

Comment 5

2 years ago
Created attachment 8726056 [details] [diff] [review]
[css-grid][flexbox] Make grid/flex item blockification happen before creating table pseudos, per the latest specs.

This code intentionally prevents blockification of these display types.
So simply removing that should work.
Attachment #8726056 - Flags: review?(dholbert)
(Assignee)

Comment 6

2 years ago
Created attachment 8726057 [details] [diff] [review]
[css-grid] Reftests for Grid item blockification.

I wrote fairly exhaustive test for Grid for various display types and it
seems to work fine.
(Assignee)

Comment 7

2 years ago
Created attachment 8726058 [details] [diff] [review]
[flexbox] Mark a few flexbox reftest as failing for now, since they assume table pseudo creation occurs before blockification.

I'm marking a few flexbox tests as failing for now - I'll file a follow-up bug
on fixing these.  It's probably better if you take that one.
(Reporter)

Comment 9

2 years ago
Comment on attachment 8726056 [details] [diff] [review]
[css-grid][flexbox] Make grid/flex item blockification happen before creating table pseudos, per the latest specs.

Review of attachment 8726056 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks!

I expect this will cause some flexbox tests (which test for this removed behavior) to fail -- particularly these ones from
http://mxr.mozilla.org/mozilla-central/source/layout/reftests/w3c-css/submitted/flexbox/ :
 flexbox-table-fixup-001a.xhtml
 flexbox-table-fixup-001b.xhtml
 flexbox-with-pseudo-elements-003.html

Those tests will need some tweaking/simplifying as part of this patch. Unless you're already making those tweaks, I'll aim to take care of that during the next ~day, and I'll post a "part 2" patch here with those test-tweaks.
Attachment #8726056 - Flags: review?(dholbert) → review+
(Assignee)

Updated

2 years ago
Depends on: 1253354
(Assignee)

Comment 11

2 years ago
I filed a follow-up bug 1253354 to fix the Flexbox reftests.
Flags: in-testsuite+
(Assignee)

Updated

2 years ago
Blocks: 616605

Comment 13

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/40132260efda
https://hg.mozilla.org/mozilla-central/rev/f1b5b0b6759d
https://hg.mozilla.org/mozilla-central/rev/db4db8be7270
https://hg.mozilla.org/mozilla-central/rev/3b00a91ad692
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
(Reporter)

Updated

11 months ago
Duplicate of this bug: 1280313
You need to log in before you can comment on or make changes to this bug.