Closed Bug 1350037 Opened 7 years ago Closed 3 months ago

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

Categories

(Core :: Layout, enhancement)

enhancement

Tracking

()

RESOLVED FIXED
123 Branch
Tracking Status
firefox55 --- wontfix
firefox123 --- fixed

People

(Reporter: dholbert, Assigned: TYLin)

References

Details

Attachments

(9 files, 1 obsolete file)

470 bytes, text/html
Details
372 bytes, text/html
Details
451 bytes, text/html
Details
440 bytes, text/html
Details
1.84 KB, patch
Details | Diff | Splinter Review
665 bytes, text/html
Details
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
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)
Attached file testcase 1
Attachment #8850666 - Attachment is obsolete: true
Attachment #8850666 - Attachment description: testcase 1 → (messy version of testcase 1; ignore)
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)
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
...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).
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
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)
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.
Severity: normal → S3

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.

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

This doesn't change behavior.

Depends on D196703

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

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

Assignee: dholbert → aethanyc
Blocks: 1833486
Flags: needinfo?(dholbert)
Blocks: 1841296
Pushed by aethanyc@gmail.com:
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
Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → 123 Branch
Regressions: 1881495
Regressions: 1881671
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: