[css-grid] Table wrapper frame breaks grid properties

RESOLVED FIXED in Firefox 52

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mats, Assigned: mats)

Tracking

({testcase})

unspecified
mozilla52
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(3 attachments)

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

3 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

3 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)
(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).
Assignee

Comment 6

3 years ago
FWIW, when I add flex-{grow,shrink,basis}:inherit I see no difference for your testcase.
Assignee

Comment 7

3 years ago
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.)
Assignee

Comment 10

3 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

3 years ago
Posted patch fixSplinter Review
Attachment #8795914 - Flags: review?(dholbert)
Attachment #8795914 - Flags: review?(dholbert) → review+
See Also: → 1306403
(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

3 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

3 years ago
Flags: in-testsuite+

Comment 15

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/860fc4fe3763
https://hg.mozilla.org/mozilla-central/rev/89f6b845f347
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.