1350037 - <caption> mistakenly honors "justify-self", inside of table as grid item

Status: RESOLVED FIXED

(Core :: Layout, enhancement)

Description

[Daniel Holbert [:dholbert]]

I believe the issue in bug 1322570 comment 61 is happening because we're mistakenly honoring "justify-self" on <caption> elements inside of tables inside of CSS grids.

More details & a testcase when I get back from lunch; just filing this as a placeholder bug for now.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: (messy version of testcase 1; ignore)

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: reference case (without "justify-self" on caption)

Comment 4

[Daniel Holbert [:dholbert]]

I think this is just from us being a little too overzealous in this chunk of ReflowInput.cpp:
https://dxr.mozilla.org/mozilla-central/rev/e03e0c60462c775c7558a1dc9d5cf2076c3cd1f9/layout/generic/ReflowInput.cpp#2388-2402

Right now, we're applying that special case for *any frame whose parent is a nsGkAtoms::tableWrapperFrame (and whose grandparent is a grid)*.  But really, I think we only intend to apply that special case for table frames.

(We need it for table frames because they've got the table-wrapper-frame as their extra parent in the frame tree, despite having the grid as their parent in the author's HTML.  But for caption, it's not the same -- the <caption> is a child of the table in the author's html, so it doesn't make sense that we'd apply grid layout alignment stuff to the caption frame.)

So perhaps we should rip out the current check...
 "is mFrame's parent is a nsGkAtoms::tableWrapperFrame"
...and instead check:
 "is mFrame a nsGkAtoms::tableFrame"
...to make this more specific to table frames (which is what we're really wanting to target here, IIUC)?

(We could still assert that the parent we skip over is a tableWrapperFrame as we walk up to look for a grid, too, but I don't think we need to explicitly check for that.)

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 ("justify-items" on <table>) -- broken in builds before bug 1322570

Here's a variant of the testcase that sets "justify-items:end" on the <caption>. It should match the reference case that I attached previously.

This testcase is broken (i.e. the yellow area is too small) in current mozilla-central, though I think if you apply the first two patches from bug 1322570, it'll change to match the reference case (because the clause I linked to in comment 4 -- which we're misapplying for caption -- will change to more directly use the grid for "justify-items" resolution, instead of the table in this case).

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: testcase 3 ("justify-items" on grid) -- broken in builds with bug 1322570

...and here's a testcase with "justify-items" on the grid. This doesn't affect the rendering in current Nightly, but it does if you apply the first 2 patches from bug 1322570 (because those patches change to more explicitly make us use the grid's "justify-items" for justify-self:auto resolution in this "we think we're a grid item" special-case that I linked in comment 4).

Comment 7

[Mats Palmgren (inactive)]

Yeah, seems reasonable.

Comment 8

[Daniel Holbert [:dholbert]]

Thanks! TYLin, perhaps you'd be up for taking this as a helper or followup for bug 1322570?

If not, mats or I can probably take it soon, too.

Comment 9

[Daniel Holbert [:dholbert]]

(D'oh, didn't mean to assign myself here -- bitten by bug 1349769, with "Take Bug" being default-checked on the add-testcase page.)

Comment 10

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: I sketched a WIP on top of my patches in bug 1322570 with the idea in comment 4 and the caption hack removed, but another type of failure occurs. Perhaps I missed something :/

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c77f9239149dfd5c2b54d590917a688f14a28624

Comment 11

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Daniel, given comment 10 showing the failure I don't fully understand, and we probably want to add test 2 in Comment 5 as a complete reftest, I would be grateful if you would help take this bug.

Comment 12

[Daniel Holbert [:dholbert]]

OK, I'll take a look tomorrow.  (Your patch does indeed look like what I had in mind; sorry it wasn't as straightforward as I imagined.)

Comment 13

[Daniel Holbert [:dholbert]]

Attachment: testcase 4 (breaks with initial WIP)

Here's a reduced testcase for the bustage that the initial WIP hit on Try.  Its caption is intentionally wider than the table body.

With the patch applied, the table body is snapped to the right edge of the grid area, and the caption overflows off of the right edge.

Without the patch applied, the caption (the wider thing) is snapped to the right edge of the grid area, and nothing overflows.

Comment 14

[Daniel Holbert [:dholbert]]

OK, so the problem is actually further down, here:
https://dxr.mozilla.org/mozilla-central/rev/272ce6c2572164f5f6a9fba2a980ba9ccf50770c/layout/generic/ReflowInput.cpp#2465-2472

* WITHOUT TYLin's wip patch, we *skip* that clause when setting up the caption's ReflowInput (because we fail the "alignCBType != nsGkAtoms::gridContainerFrame" condition)

* WITH TYLin's wip patch, we *enter* that clause, and we call CalculateBlockSideMargins.

And CalculateBlockSideMargins causes the problem here with testcase 4. In particular:
 - It calculates availMarginSpace by subtracting our margin-box ISize from the available ISize (which in this case is the width of our grid area, I think).
 - That calculation produces availMarginSpace = -1920
 - Because this is negative, it *updates the ReflowInput::ComputedLogicalMargin* to take that negative value as its margin-end*:
https://dxr.mozilla.org/mozilla-central/rev/272ce6c2572164f5f6a9fba2a980ba9ccf50770c/layout/generic/ReflowInput.cpp#2686-2690

 - So then later on when we size the table-wrapper frame, we think that its caption's margin-box is smaller than it actually is, due to that negative margin. So the caption overflows, because we didn't actually account for its full size.

Comment 15

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Bug 1350037 Part 1 - Rename two variables storing grid item's axes. r?dholbert

Rename to make them clearer that they stores child's axes in grid container's
writing-mode. Without the "InWM" suffix, it's a bit confusing why not just use
eLogicalAxisInline.

Comment 16

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Bug 1350037 Part 2 - Add a helper to compute stretch for grid items. r?dholbert

The helper will be used in the next part, too.

This doesn't change behavior.

Depends on D196703

Comment 17

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Bug 1350037 Part 3 - Prevent table caption from honoring justify-*/align-* when table is a grid item. r?dholbert

The old code used to apply ShrinkWrap flag to both the inner table and the
caption if the table wrapper is a stretched grid item. However, per analysis in
bug 1350037 comment 4, when we reflow table captions, we'd like it not to
shrink-wrap, but to stretch its inline-size to at least as wide as the inner
table frame [1].

This patch moves the logic that computes whether a table is a stretched grid
item from ReflowInput to nsTableWrapperFrame, and apply the ShrinkWrap
flag to inner table if needed.

[1] https://searchfox.org/mozilla-central/rev/54d6d78c3b33c92fd9eef9c0555a153bf8e4f2b4/layout/tables/nsTableWrapperFrame.cpp#760-763

Depends on D196704

Comment 18

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

I'll finish what I left 7 years ago :)

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3fc8eb817020
Part 1 - Rename two variables storing grid item's axes. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/af239677dad3
Part 2 - Add a helper to compute stretch for grid items. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/6e8085865f74
Part 3 - Prevent table caption from honoring justify-*/align-* when table is a grid item. r=dholbert

Comment 20

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/3fc8eb817020
https://hg.mozilla.org/mozilla-central/rev/af239677dad3
https://hg.mozilla.org/mozilla-central/rev/6e8085865f74