Closed Bug 1306106 Opened 8 years ago Closed 8 years ago

[css-grid] Table wrapper frame breaks grid properties

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: MatsPalmgren_bugz, Assigned: MatsPalmgren_bugz)

References

Details

(Keywords: testcase)

Attachments

(3 files)

Attached file grid testcase (deleted) —
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)...
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)
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)
(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)
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).
FWIW, when I add flex-{grow,shrink,basis}:inherit I see no difference for your testcase.
The attached Grid testcase works in Chrome though.
*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...
(I filed https://github.com/w3c/csswg-drafts/issues/547 on the (interoperable) inconsistency between "flex" vs "align-self" here.)
> 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
Attached patch fix (deleted) — Splinter Review
Attachment #8795914 - Flags: review?(dholbert)
Attachment #8795914 - Flags: review?(dholbert) → review+
(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.
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.
Flags: in-testsuite+
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: