<caption> mistakenly honors "justify-self", inside of table as grid item
Categories
(Core :: Layout, enhancement)
Tracking
()
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.
Reporter | ||
Updated•7 years ago
|
Reporter | ||
Updated•7 years ago
|
Comment hidden (obsolete) |
Reporter | ||
Comment 2•7 years ago
|
||
Reporter | ||
Updated•7 years ago
|
Reporter | ||
Comment 3•7 years ago
|
||
Reporter | ||
Comment 4•7 years ago
|
||
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.)
Reporter | ||
Comment 5•7 years ago
|
||
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).
Reporter | ||
Comment 6•7 years ago
|
||
...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).
Reporter | ||
Comment 8•7 years ago
|
||
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.
Reporter | ||
Comment 9•7 years ago
|
||
(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 | ||
Comment 10•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c77f9239149dfd5c2b54d590917a688f14a28624
Assignee | ||
Comment 11•7 years ago
|
||
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.
Reporter | ||
Comment 12•7 years ago
|
||
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.)
Reporter | ||
Updated•7 years ago
|
Reporter | ||
Comment 13•7 years ago
|
||
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.
Reporter | ||
Comment 14•7 years ago
|
||
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.
Updated•2 years ago
|
Assignee | ||
Comment 15•3 months ago
|
||
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
.
Assignee | ||
Comment 16•3 months ago
|
||
The helper will be used in the next part, too.
This doesn't change behavior.
Depends on D196703
Assignee | ||
Comment 17•3 months ago
|
||
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.
Depends on D196704
Assignee | ||
Comment 18•3 months ago
|
||
I'll finish what I left 7 years ago :)
Comment 19•3 months ago
|
||
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
Comment 20•3 months ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3fc8eb817020
https://hg.mozilla.org/mozilla-central/rev/af239677dad3
https://hg.mozilla.org/mozilla-central/rev/6e8085865f74
Updated•1 month ago
|
Description
•