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

RESOLVED FIXED in Firefox 45

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jfernandez, Assigned: mats)

Tracking

({testcase})

Trunk
mozilla45
testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox45 fixed)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
Created attachment 8693762 [details]
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.
(Reporter)

Comment 1

3 years ago
Created attachment 8693768 [details]
bug-align-center-expected.png

Expected result.
(Reporter)

Comment 2

3 years ago
Created attachment 8693770 [details]
bug-align-center-1.html

test case.
Attachment #8693762 - Attachment is obsolete: true
(Assignee)

Comment 3

3 years ago
Thanks for your bug reports!
Assignee: nobody → mats
Blocks: 1151213
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Keywords: testcase
(Assignee)

Comment 4

3 years ago
Created attachment 8694168 [details] [diff] [review]
[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.
Attachment #8694168 - Flags: review?(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;
Attachment #8694168 - Flags: review?(dholbert) → review+
(Assignee)

Comment 8

3 years ago
> 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.
Flags: in-testsuite+
(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

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/dbed67b551aa
https://hg.mozilla.org/mozilla-central/rev/597585905486
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.