Closed Bug 1237754 Opened 4 years ago Closed

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

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox46 --- affected
firefox47 --- fixed

People

(Reporter: mats, Assigned: mats)

Details

Attachments

(2 files, 2 obsolete files)

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
Attached patch fix (obsolete) — Splinter Review
Attachment #8705637 - Flags: review?(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");
Attachment #8705637 - Flags: review?(dholbert) → review+
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.
Flags: needinfo?(mats)
(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)
(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.
Flags: needinfo?(mats) → in-testsuite+
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).
backed this out for https://treeherder.mozilla.org/logviewer.html#?job_id=19964147&repo=mozilla-inbound that changed into frequent failures after this push
Flags: needinfo?(mats)
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)
Flags: needinfo?(mats) → needinfo?(cbook)
(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.)
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
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?
Flags: needinfo?(cbook) → needinfo?(mats)
Good catch; let's fix justify-content too before relanding.
Flags: needinfo?(mats)
Attached patch fixSplinter Review
Intra-diff for the code change:
-+    alignment = isAlign ? NS_STYLE_ALIGN_STRETCH : NS_STYLE_ALIGN_START;
++    alignment = NS_STYLE_ALIGN_STRETCH;
Attachment #8705637 - Attachment is obsolete: true
Attachment #8710203 - Flags: review?(dholbert)
Attached patch reftest changesSplinter Review
Attachment #8705638 - Attachment is obsolete: true
Comment on attachment 8710203 [details] [diff] [review]
fix

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

Thanks! r=me
Attachment #8710203 - Flags: review?(dholbert) → review+
https://hg.mozilla.org/mozilla-central/rev/f49d6b3581ed
https://hg.mozilla.org/mozilla-central/rev/e555b1530ebb
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.