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

ASSIGNED
Assigned to

Status

()

Core
Layout
ASSIGNED
8 months ago
8 months ago

People

(Reporter: dholbert, Assigned: dholbert, NeedInfo)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(firefox55 affected)

Details

Attachments

(6 attachments, 1 obsolete attachment)

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.
Flags: needinfo?(dholbert)
Flags: needinfo?(dholbert)
Comment hidden (obsolete)
Created attachment 8850667 [details]
testcase 1
Attachment #8850666 - Attachment is obsolete: true
Attachment #8850666 - Attachment description: testcase 1 → (messy version of testcase 1; ignore)
Created attachment 8850668 [details]
reference case (without "justify-self" on caption)
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.)
Flags: needinfo?(mats)
Created attachment 8850670 [details]
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).
Assignee: nobody → dholbert
Created attachment 8850675 [details]
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

8 months ago
Yeah, seems reasonable.
Flags: needinfo?(mats)
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.
Flags: needinfo?(tlin)
(D'oh, didn't mean to assign myself here -- bitten by bug 1349769, with "Take Bug" being default-checked on the add-testcase page.)
Assignee: dholbert → nobody
Created attachment 8850859 [details] [diff] [review]
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
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.
Flags: needinfo?(tlin)
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.)
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Flags: needinfo?(dholbert)
Created attachment 8852255 [details]
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.
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.
You need to log in before you can comment on or make changes to this bug.