1278429 - [css-grid] Fragmented grid item with align-self:last-baseline has wrong block-axis position

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Mats Palmgren (inactive)]

Attachment: testcase

Follow-up from bug 1221525 comment 13.

Comment 1

[Mats Palmgren (inactive)]

Attachment: fix

This aligns the last grid item fragment at the row end edge.
(I'm adding reftests in bug 1151204 that covers this.)

Comment 2

[Mats Palmgren (inactive)]

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

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 8761016 [details] [diff] [review]
fix

Review of attachment 8761016 [details] [diff] [review]:
-----------------------------------------------------------------

> Bug 1278429 - [css-grid] Fragmented grid item with align-self:last-baseline has wrong block-axis position.  r=dholbert

Nit: commit message should describe the change, rather than describing the problem.

::: layout/generic/nsGridContainerFrame.cpp
@@ +4558,5 @@
>      cb = aState.ContainingBlockFor(area);
>      isConstrainedBSize = aFragmentainer && !wm.IsOrthogonalTo(childWM);
>      if (isConstrainedBSize) {
> +      nscoord gridAreaBOffset = cb.BStart(wm) - aState.mFragBStart;
> +      consumedGridAreaBSize = std::max(0, -gridAreaBOffset);

I don't immediately understand the calculation behind "consumedGridAreaBSize" here.

Intuitively (based on naming), it sounds like consumedGridAreaBSize might be describing the same quantity -- but clearly they're not, because they're not equal here.  Can you add a comment here explaining the calculation/the difference between them?

@@ +4657,5 @@
>        }
> +      nscoord cbsz = cb.BSize(wm);
> +      if (isConstrainedBSize) {
> +        cbsz -= consumedGridAreaBSize;
> +      }

(If you like, looks like we could drop the isConstrainedBSize check here, and just collapse the above 4 lines to:
  nscoord cbsz = cb.BSize(wm) - consumedGridAreaBSize;
...because consumedGridAreaBSize will just be 0 when isConstrainedBSize is false.)

Comment 4

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #3)
> Comment on attachment 8761016 [details] [diff] [review]
> fix
> 
> Review of attachment 8761016 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> > Bug 1278429 - [css-grid] Fragmented grid item with align-self:last-baseline has wrong block-axis position.  r=dholbert
> 
> Nit: commit message should describe the change, rather than describing the problem.

(Also, RE the "last-baseline" mention in the commit message in particular -- from looking at the patch and playing around with the testcase a bit, I think this affects grid items with *any* non-"start"-equivalent align-self value -- e.g. "align-self:center" and "align-self:end" seem to be broken as well.  The issue & the fix are broader than "last-baseline", IIUC.)

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 8761016 [details] [diff] [review]
fix

Review of attachment 8761016 [details] [diff] [review]:
-----------------------------------------------------------------

r- since I can't say I understand the consumedGridAreaBSize computation at this point, but this is likely r+ once that's clarified in a code-comment.

Comment 6

[Mats Palmgren (inactive)]

Attachment: fix

Does this help?

+  // The part of the child's grid area that's in previous container fragments.
+  nscoord consumedGridAreaBSize = 0;

+      // |gridAreaBOffset| is the offset of the child's grid area in this
+      // container fragment (if negative, that offset is the child CB size
+      // consumed in previous container fragments).  Note that cb.BStart
+      // and aState.mFragBStart are in "global" grid coordinates (like all
+      // track positions).
+      nscoord gridAreaBOffset = cb.BStart(wm) - aState.mFragBStart;
+      consumedGridAreaBSize = std::max(0, -gridAreaBOffset);

> I think this affects grid items with *any* non-"start"-equivalent

Indeed.  We should discuss what we want to do about these cases in London,
because there really are no good solutions.  It's *impossible* to do
end-alignment that attaches to the end of the last track in general
because even if you could somehow predict upfront exactly where the child
can break, it might not break exactly at a point where it will fill
the last track(s).  So it might actually be better to align items that
has a fragmented grid area at the start.

Anyway, this is a stop-gap fix so that we at least do something that
isn't completely off the rails...

Comment 7

[Mats Palmgren (inactive)]

I pasted the wrong text there (the patch is correct). 
(I changed s/offset/distance/ before submitting the patch)

Comment 8

[Mats Palmgren (inactive)]

Attachment: fix

(re-phrased the commit message)

Comment 9

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #6)
> Created attachment 8761852 [details] [diff] [review]
> fix
> 
> Does this help?

I think so, with one exception -- I don't understand this line (line 4569 in the patch)
>      cb.BStart(wm) = std::max(0, gridAreaBOffset);

Your new comment says:
 - cb.BStart is (initially at least) in "global" coordinates
 - gridAreaBOffset is an *offset within the current container fragment* (not in "global" coordinates)
So when we set the former to the latter in this line that I quoted above, are we intentionally *changing* the coordinate space of |cb.BStart|?

(I think maybe we are intentionally changing coordinate space, so that "0" ends up representing the upper-left corner of the child's grid area *in the container's first continuation*, and cb.BStart represents where the current child-fragment's "grid area fragment" is, with respect to that grid-area upper-left corner... Is that approximately right? In any case, I think this needs a further clarification, particularly if BStart's coordinate space is indeed changing.)

Comment 10

[Daniel Holbert [:dholbert]]

Actually, in the scenario I was imagining, gridAreaBOffset was negative, so the assignment quoted above would end up doing
  cb.BStart(wm) = 0;

My question still remains, though -- is that still in the "global coordinate space" (as your comment says cb.BStart is)? Or are we intentionally changing cb's coordinate space that point?

Comment 11

[Mats Palmgren (inactive)]

Well, it's not really changing coordinate space, it's just that it starts out
as a "global" offset because that's what ContainingBlockFor gives us.
Yes, the intent of 'cb' is to be the "local" CB for this child fragment,
it just takes a few calculations to get there.

Comment 12

[Daniel Holbert [:dholbert]]

Gotcha, thanks. Could you clarify the "cb.BStart and ... are in global grid coordinates" comment to make it clearer that that statement is only briefly true? (Maybe add "initially", or add a final parenthetical saying that it changes, or something)

r=me with that

Comment 13

[Mats Palmgren (inactive)]

OK, thanks.  I added an "(initially)" there.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6859e11b6a2a
[css-grid] Align a fragmented grid item with 'align-self' that is anchored at the end ('last-baseline', 'end', etc) at the end of its last track.  r=dholbert

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6859e11b6a2a