1229145 - [css-grid] align/justify-self center is not applied to the margin-box

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Javier Fernandez]

Attachment: bug-align-center-1.html

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Firefox/38.0 Iceweasel/38.2.1
Build ID: 20150828090403

Steps to reproduce:

Loading the attached test case.


Actual results:

Item is not alignment correctly; margins overflow the grid container and the result is not centered at all.


Expected results:

The spec states that the alignment subject is the item's margin box, hence alignment must take margins into account when computing the final position.

The attached picture shows how it should look like.

Comment 1

[Javier Fernandez]

Attachment: bug-align-center-expected.png

Expected result.

Comment 2

[Javier Fernandez]

Attachment: bug-align-center-1.html

test case.

Comment 3

[Mats Palmgren (inactive)]

Thanks for your bug reports!

Comment 4

[Mats Palmgren (inactive)]

Attachment: [css-grid] Account for start margin when calculating border box offset for align-self/justify-self:center.

Forgot to add in the start margin here to get a frame offset.
It should have read: offset = marginStart + SpaceToFill(...) / 2.

I'm inlining here SpaceToFill though for clarity.  Now it's easy
to see that if marginStart == marginEnd those terms disappears
and a centered border box offset remains, and that increasing
marginEnd for example make the offset smaller.

Comment 5

[Mats Palmgren (inactive)]

Attachment: Test tweaks

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

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 8694168 [details] [diff] [review]
[css-grid] Account for start margin when calculating border box offset for align-self/justify-self:center.

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

r=me, ideally with the code clarified a bit as noted below:

::: layout/generic/nsGridContainerFrame.cpp
@@ +965,5 @@
>      }
> +    case NS_STYLE_ALIGN_CENTER: {
> +      nscoord size = aAxis == eLogicalAxisBlock ? aChildSize.BSize(wm)
> +                                                : aChildSize.ISize(wm);
> +      offset = (aCBSize - size + marginStart - marginEnd) / 2;

The way you phrased this in comment 4 makes intuitive sense to me, but this version in the patch does not (though I can see that they're mathematically equivalent).  In particular, it's not at all clear why "+ marginStart - marginEnd" makes sense here, for centering.

This is complex because you're really doing two things at once.  IMO it'd be clearer to separate the 2 steps:
  (1) center the margin-box
  (2) add the margin to convert to a border-box (frame-rect) offset

Something like:
  offset = SpaceToFill(...);   [the old code]
  // Now we have the offset of the centered margin-box.
  // Add marginStart to get the offset of frame (i.e. the border-box):
  offset += marginStart;

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dbed67b551aa
https://hg.mozilla.org/integration/mozilla-inbound/rev/597585905486

Comment 8

[Mats Palmgren (inactive)]

> The way you phrased this in comment 4 makes intuitive sense to me

Good, then I don't see why this needs to be clarified further
since the reader can easily work that out in their head too.

I don't see why your suggested change is any clearer.  The reader
would still have to grok why SpaceToFill(...) / 2 centers the margin-
box with the given params.  So I think I prefer the version where you
can actually see all the terms involved.

I did however add a code comment on the function itself to point out
that it aligns (or stretches) an item's margin box inside the given
size, and a comment where |offset| is declared pointing out that it
is a frame offset (border box).  As a hint to the reader to keep that
in mind while reading the code.

Comment 9

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #8)
> > The way you phrased this in comment 4 makes intuitive sense to me
> 
> Good, then I don't see why this needs to be clarified further
> since the reader can easily work that out in their head too.

I had the benefit of seeing comment 4 here. A reader reading this function does not have that benefit (unless they get sufficiently confused & check hg blame, at which point we've kind of lost at being readable).

> I don't see why your suggested change is any clearer.  The reader
> would still have to grok why SpaceToFill(...) / 2 centers the margin-
> box with the given params.

Assuming SpaceToFill does what it says & returns the packing space, I think my formulation is pretty clear... Centering a box is as simple as placing half of the extra space to its left (i.e. giving it an offset of SpaceToFill()/2).

I don't see an easy way to intuitively interpret/understand the arithmetic in the patch that landed, though.  (Maybe there is such a way and I'm just not seeing it? If so, maybe a code comment would be helpful.)

> So I think I prefer the version where you
> can actually see all the terms involved.

Both versions have all the terms; that's not an issue. The problem with the patch's version is that it's doing two things -- centering & simultaneously doing a conversion between coordinate spaces -- in a single expression.  It's nontrivial to reverse-engineer that that's what's going on & that it's correct.

> I did however add a code comment on the function itself to point out
> that it aligns (or stretches) an item's margin box inside the given
> size, and a comment where |offset| is declared pointing out that it
> is a frame offset (border box).

Thanks, that's helpful.

I do still think a comment and/or a code-tweak would help a lot here, for readability, though I'll defer to your judgement.

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/dbed67b551aa
https://hg.mozilla.org/mozilla-central/rev/597585905486