Mats Palmgren (inactive) Assignee
	Description • 8 years ago

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

	Mats Palmgren (inactive) Assignee
	Comment 2 • 8 years ago

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

	Daniel Holbert [:dholbert]
	Comment 3 • 8 years ago

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

	Daniel Holbert [:dholbert]
	Comment 4 • 8 years ago

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.

	Daniel Holbert [:dholbert]
	Comment 5 • 8 years ago

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

	Pulsebot
	Comment 6 • 8 years ago

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

	Mats Palmgren (inactive) Assignee
	Comment 7 • 8 years ago

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

	Mats Palmgren (inactive) Assignee
	Comment 8 • 8 years ago

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

	Carsten Book [:Tomcat]
	Comment 9 • 8 years ago

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

	Pulsebot
	Comment 10 • 8 years ago

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

	Mats Palmgren (inactive) Assignee
	Comment 11 • 8 years ago

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)

	Daniel Holbert [:dholbert]
	Comment 12 • 8 years ago

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

	Daniel Holbert [:dholbert]
	Comment 13 • 8 years ago

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

	Daniel Holbert [:dholbert]
	Comment 14 • 8 years ago

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?

	Mats Palmgren (inactive) Assignee
	Comment 15 • 8 years ago

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

	Mats Palmgren (inactive) Assignee
	Comment 16 • 8 years ago

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

	Mats Palmgren (inactive) Assignee
	Comment 18 • 8 years ago

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

	Daniel Holbert [:dholbert]
	Comment 19 • 8 years ago

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

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

Thanks! r=me

	Pulsebot
	Comment 20 • 8 years ago

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

	Carsten Book [:Tomcat]
	Comment 21 • 8 years ago
bugherder

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

	Carsten Book [:Tomcat]
	Comment 22 • 8 years ago
bugherder

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

	yisibl
	Comment 23 • 8 years ago

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

	yisibl
	Comment 24 • 8 years ago

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