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.

Expected result.

test case.

Thanks for your bug reports!

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 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;

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

> 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.

(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.

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