1237754 - [css-grid][css-align] Make 'align-content:normal' behave as 'stretch' for Grid containers.

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Mats Palmgren (inactive)]

https://lists.w3.org/Archives/Public/www-style/2016Jan/0031.html
"
Grid
----
  - RESOLVED: Change the initial value of align-content to stretch
"

Which is kind of nonsensical, since the initial value is 'normal',
but I guess they mean 'normal' "behaves as" 'stretch', same as
for Flex containers:
https://drafts.csswg.org/css-align-3/#propdef-align-content

Comment 1

[Mats Palmgren (inactive)]

Attachment: fix

Comment 2

[Mats Palmgren (inactive)]

Attachment: reftest

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9fbf2f0a658a&selectedJob=15194856

Comment 3

[Daniel Holbert [:dholbert]]

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

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

r=me, just one nit:

::: layout/generic/nsGridContainerFrame.cpp
@@ +2916,5 @@
>    auto alignment = ::GetAlignJustifyValue(valueAndFallback, wm, isAlign,
>                                            &overflowSafe);
>    if (alignment == NS_STYLE_ALIGN_NORMAL) {
> +    alignment = isAlign ? NS_STYLE_ALIGN_STRETCH : NS_STYLE_ALIGN_START;
> +    valueAndFallback = alignment; // we may need a fallback for 'stretch' below

At first look, this assignment confused me, because it looks like we're stomping on whatever fallback value exists in the high bits of |valueAndFallback|.  (i.e. we're clearing the author's chosen fallback value)

But after checking the spec and looking at how this is used, I *think* this is actually OK, because IIUC 'normal' can't be specified with a fallback value.  To make it clearer that this is OK, could you add an assertion before this assignment? Something like:

  MOZ_ASSERT(valueAndFallback == NS_STYLE_ALIGN_NORMAL,
             "*-content:normal cannot be specified with explicit fallback");

Comment 4

[Daniel Holbert [:dholbert]]

Actually, I think this 'normal' resolution might really want to happen inside of GetAlignJustifyValue()...  That's where we resolve 'left' and 'right', at least -- based in part on whether we're "align-content" vs "justify-content" -- and this feels similar.

I think we'd just need a new clause in this 'switch' statement:
  http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsGridContainerFrame.cpp?mark=1356-1362#1349

Then the caller won't need the special-case that I quoted in comment 3 anymore, I think.

Comment 5

[Daniel Holbert [:dholbert]]

(if you agree with the change in comment 4, then I think the assertion suggested in comment 3 wouldn't be as needed, since there'd be less of an appearance that we're stomping on data)

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d935c88d79d6
https://hg.mozilla.org/integration/mozilla-inbound/rev/57a677703c20

Comment 7

[Mats Palmgren (inactive)]

(In reply to Daniel Holbert [:dholbert] from comment #3)
> To make it clearer that this is OK, could you add an assertion
> before this assignment? Something like:
> 
>   MOZ_ASSERT(valueAndFallback == NS_STYLE_ALIGN_NORMAL,
>              "*-content:normal cannot be specified with explicit fallback");

Sure, I added that assertion (for documentation purposes).

(In reply to Daniel Holbert [:dholbert] from comment #4)
> Actually, I think this 'normal' resolution might really want to happen
> inside of GetAlignJustifyValue()... 

That wouldn't work because we need to modify |valueAndFallback| for the next
call to GetAlignJustifyValue().  That's what the added comment is about,
i.e. 'normal' -> 'stretch' -> 'start', where the last step occurs if there
is overflow.

Comment 8

[Mats Palmgren (inactive)]

s/GetAlignJustifyValue/GetAlignJustifyFallbackIfAny/ in my last comment.
And yes, I could have mapped it both in GetAlignJustifyValue and
GetAlignJustifyFallbackIfAny instead I guess.  It's probably clearer
to have it in one place though (i.e. as it landed).

Comment 9

[Carsten Book [:Tomcat]]

backed this out for https://treeherder.mozilla.org/logviewer.html#?job_id=19964147&repo=mozilla-inbound that changed into frequent failures after this push

Comment 10

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1dbf56316c7
https://hg.mozilla.org/integration/mozilla-inbound/rev/b706331377c5

Comment 11

[Mats Palmgren (inactive)]

I'm only changing nsGridContainerFrame.cpp here which is only used for display:[inline-]grid.
So I'm confident that the intermittent failure wasn't caused by this bug.
Please re-land.

Looking at the failure in reftest-analyzer, I see a few pixels of anti-aliasing.
Adding some fuzz in bug 1240297 seems like the right fix (as was recently done
in bug 1186405 for inline--display-block--01.xhtml)

Comment 12

[Daniel Holbert [:dholbert]]

(This made me realize that I actually added fuzz for the wrong file in bug 1186405 - I've fixed it now, though. So, inline--display-block--01.xhtml shouldn't report these errors anymore, as of bug 1186405 comment 44.  And regardless, there's no chance its fuzzy-failures were related to this bug's patches.)

Comment 13

[Daniel Holbert [:dholbert]]

The spec was just updated to reflect this change, though it was updated in a way that suggests "justify-content:normal" changed as well: https://hg.csswg.org/drafts/rev/041ec4086fcf

I think that may have just been a mistake -- I emailed the list to clarify:
  https://lists.w3.org/Archives/Public/www-style/2016Jan/0129.html

Comment 14

[Daniel Holbert [:dholbert]]

Nope, not a mistake; just, the meeting-notes that we were reading from (quoted in comment 0 here) were incomplete:
 https://lists.w3.org/Archives/Public/www-style/2016Jan/0130.html

So, we don't need to branch on "isAlign" here anymore.  Mats, maybe we should fix that before re-landing here, since the current patches are based on what turned out to be an incorrect understanding of the CSSWG resolution?

Comment 15

[Mats Palmgren (inactive)]

Good catch; let's fix justify-content too before relanding.

Comment 16

[Mats Palmgren (inactive)]

Attachment: fix

Intra-diff for the code change:
-+    alignment = isAlign ? NS_STYLE_ALIGN_STRETCH : NS_STYLE_ALIGN_START;
++    alignment = NS_STYLE_ALIGN_STRETCH;

Comment 17

[Mats Palmgren (inactive)]

Attachment: reftest changes

Comment 18

[Mats Palmgren (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3da6c4b34000&selectedJob=15688424

Comment 19

[Daniel Holbert [:dholbert]]

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

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

Thanks! r=me

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f49d6b3581ed
https://hg.mozilla.org/integration/mozilla-inbound/rev/e555b1530ebb

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f49d6b3581ed
https://hg.mozilla.org/mozilla-central/rev/e555b1530ebb

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f49d6b3581ed
https://hg.mozilla.org/mozilla-central/rev/e555b1530ebb

Comment 23

[yisibl]

Spec: https://drafts.csswg.org/css-align-3/#align-grid

Comment 24

[yisibl]

https://drafts.csswg.org/css-align-3/#distribution-grid