Closed Bug 1278429 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: mats, Assigned: mats)

References

Details

(Keywords: testcase)

Attachments

(2 files, 2 obsolete files)

Attached file testcase
Follow-up from bug 1221525 comment 13.
Attached patch fix (obsolete) — Splinter Review
This aligns the last grid item fragment at the row end edge.
(I'm adding reftests in bug 1151204 that covers this.)
Attachment #8761016 - Flags: review?(dholbert)
Blocks: 1151204
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.)
(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 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.
Attachment #8761016 - Flags: review?(dholbert) → review-
Attached patch fix (obsolete) — Splinter Review
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...
Attachment #8761852 - Flags: review?(dholbert)
I pasted the wrong text there (the patch is correct). 
(I changed s/offset/distance/ before submitting the patch)
Attached patch fixSplinter Review
(re-phrased the commit message)
Attachment #8761016 - Attachment is obsolete: true
Attachment #8761852 - Attachment is obsolete: true
Attachment #8761852 - Flags: review?(dholbert)
Attachment #8761859 - Flags: review?(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.)
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?
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.
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
Attachment #8761859 - Flags: review?(dholbert) → review+
OK, thanks.  I added an "(initially)" there.
Pushed by mpalmgren@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/6859e11b6a2a
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.