Closed
Bug 1306106
Opened 8 years ago
Closed 8 years ago
[css-grid] Table wrapper frame breaks grid properties
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla52
Tracking | Status | |
---|---|---|
firefox52 | --- | fixed |
People
(Reporter: MatsPalmgren_bugz, Assigned: MatsPalmgren_bugz)
References
Details
(Keywords: testcase)
Attachments
(3 files)
(deleted),
text/html
|
Details | |
(deleted),
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review |
I suspect we need something like this also for :-moz-table-wrapper
http://searchfox.org/mozilla-central/rev/5bdd2876aeb9dc97dc619ab3e067e86083dad023/layout/style/res/forms.css#29-45
Comment 1•8 years ago
|
||
We do have something like that, for "align-self"/"justify-self" at least:
> *|*::-moz-table-wrapper {
> [...]
> align-self: inherit;
> justify-self: inherit;
> order: inherit; /* needed for "order" to work on table flex/grid items */
> }
https://dxr.mozilla.org/mozilla-central/rev/66a77b9bfe5dcacd50eccf85de7c0e7e15ce0ffd/layout/style/res/ua.css#24,42-44
I think the other properties from forms.css are all applicable to an actual flex/grid *container*, rather than to an item, so they shouldn't need to inherit to :-moz-table-wrapper.
So, I think we're all fine here (and the issue in bug 1304012 comment 29 / bug 1304012 comment 30 is likely just a subtle bug in the patch there)...
Assignee | ||
Comment 2•8 years ago
|
||
Yes, I realized that too. It's the grid item properties we need, 'grid-row-start' etc.
Are there any flex item specific properties you want me to put in?
Assignee: nobody → mats
Flags: needinfo?(dholbert)
Assignee | ||
Comment 3•8 years ago
|
||
We probably should inherit the *-content and *-items properties though,
because CSS Align also applies to table containers as far as I know.
(although we don't implement it in table layout yet)
Comment 4•8 years ago
|
||
(In reply to Mats Palmgren (:mats) from comment #2)
> Are there any flex item specific properties you want me to put in?
Not sure -- there are a few flex-item-specific properties that we seem to be missing (the "flex" subproperties, flex-{grow,shrink,basis}). I'd expect we need those here, to make the item grow in this testcase:
https://jsfiddle.net/vhomvqor/
BUT, Chrome/Edge interoperably show red in that testcase, just like we do. So I'm not sure quite yet... Maybe better to just focus on grid-item-specific properties here for the moment.
(In reply to Mats Palmgren (:mats) from comment #3)
> We probably should inherit the *-content and *-items properties though,
> because CSS Align also applies to table containers as far as I know.
That would only make a difference for aligning the table-frame within its wrapper-frame, I think, right? And that's not really what someone's aiming to do when they set "align-content"/"align-items" on a table -- instead, they really want to align the rows within the table, I expect.
Flags: needinfo?(dholbert)
Comment 5•8 years ago
|
||
The CSS2 spec does say the following:
> The computed values of properties 'position', 'float', 'margin-*',
> 'top', 'right', 'bottom', and 'left' on the table element are used
> on the table wrapper box and not the table box; all other values of
> non-inheritable properties are used on the table box and not the
> table wrapper box. (Where the table element's values are not used
> on the table and table wrapper boxes, the initial values are used
> instead.)
https://www.w3.org/TR/CSS2/tables.html#model
That seems to suggest that none of these properties should be "inherited up" to the wrapper box after all (and it might explain why Firefox/Chrome/Edge interoperably ignore the "flex" property in Comment 4's jsfiddle).
Assignee | ||
Comment 6•8 years ago
|
||
FWIW, when I add flex-{grow,shrink,basis}:inherit I see no difference for your testcase.
Assignee | ||
Comment 7•8 years ago
|
||
The attached Grid testcase works in Chrome though.
Comment 8•8 years ago
|
||
*facepalm* I forgot to add "display:flex" in my jsfiddle testcase. Fixed here:
https://jsfiddle.net/vhomvqor/1/
We're still interoperably "broken", though -- that change doesn't impact anyone's rendering. Firefox, Chrome, and Edge all ignore "flex: 1" there (and honor it if I remove "display:table"). And at the same time, they all interoperably honor "align-self: flex-end" on the table, as shown here:
https://jsfiddle.net/vhomvqor/2/
Anyway -- I'll file a github issue to get clarification on this within the flexbox spec at least. I'd like to have that sorted out before we change our behavior w.r.t. flex properties here. Feel free to proceed with grid-item properties, though.
(In reply to Mats Palmgren (:mats) from comment #6)
> FWIW, when I add flex-{grow,shrink,basis}:inherit I see no difference for
> your testcase.
You're talking about adding that to ua.css, right? I think that wasn't helping just becuase I'd left out "display:flex". :) I expect it should make a difference for the /1 and /2 versions of my testcase...
Comment 9•8 years ago
|
||
(I filed https://github.com/w3c/csswg-drafts/issues/547 on the (interoperable) inconsistency between "flex" vs "align-self" here.)
Assignee | ||
Comment 10•8 years ago
|
||
> You're talking about adding that to ua.css, right?
Yes, in the *|*::-moz-table-wrapper rule:
+ flex-grow: inherit;
+ flex-shrink: inherit;
+ flex-basis: inherit;
With those added I *still* don't see a difference with the new testcase
where you added display:flex.
FWIW, I tend to think table wrappers *should* work as flex/grid items and
that CSS2 is bonkers.
Anyway, I'll leave out the flex item properties for now and let you deal
with them separately.
Summary: [css-grid][css-flexbox][css-align] table wrapper frame breaks align/grid/flex properties → [css-grid] Table wrapper frame breaks grid properties
Assignee | ||
Comment 11•8 years ago
|
||
Attachment #8795914 -
Flags: review?(dholbert)
Assignee | ||
Comment 12•8 years ago
|
||
Updated•8 years ago
|
Attachment #8795914 -
Flags: review?(dholbert) → review+
Updated•8 years ago
|
Comment 13•8 years ago
|
||
(In reply to Mats Palmgren (:mats) from comment #10)
> FWIW, I tend to think table wrappers *should* work as flex/grid items and
> that CSS2 is bonkers.
Agreed (modulo my minor interop concerns, about the flex subproperties interoperably *not* working on tables right now).
> Anyway, I'll leave out the flex item properties for now and let you deal
> with them separately.
Filed bug 1306403 on that.
Comment 14•8 years ago
|
||
Pushed by mpalmgren@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/860fc4fe3763
[css-grid] Make the table wrapper box inherit a few grid item properties. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/89f6b845f347
[css-grid] Reftest with a table wrapper box grid item.
Assignee | ||
Updated•8 years ago
|
Flags: in-testsuite+
Comment 15•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/860fc4fe3763
https://hg.mozilla.org/mozilla-central/rev/89f6b845f347
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in
before you can comment on or make changes to this bug.
Description
•