1233106 - implement changes to css-align property computation

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

The CSS WG resolved on the changes to computation of various css-align properties, most of which were issues that we raised.  This was as described in:
https://lists.w3.org/Archives/Member/w3c-css-wg/2015OctDec/0143.html
(there's probably a public link, but don't have time tofind it mid-telecon)

Comment 1

[Mats Palmgren (inactive)]

https://lists.w3.org/Archives/Public/www-style/2015Dec/0233.html

Comment 2

[Mats Palmgren (inactive)]

Attachment: Update align-/justify-* properties to the current CSS Align spec (adding 'normal' keyword, dropping 'auto' in some cases etc).

Comment 3

[Mats Palmgren (inactive)]

Attachment: Updated tests due to changes to align-/justify-* properties.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=affb1f2a8950
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed84a3976f2e

Comment 4

[Mats Palmgren (inactive)]

BTW, I posted about a possible issue with align/justify-self:auto here:
https://lists.w3.org/Archives/Public/www-style/2016Jan/0004.html
(the patch above implements what the spec currently says)

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 8703447 [details] [diff] [review]
Update align-/justify-* properties to the current CSS Align spec (adding 'normal' keyword, dropping 'auto' in some cases etc).

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

r=me, with nits below addressed as you see fit.

::: layout/style/nsRuleNode.cpp
@@ +8111,2 @@
>      } else {
>        pos->mAlignSelf = NS_STYLE_ALIGN_START;

Why is this _START? Should this be _NORMAL now?

(I think this is a case where we have "align-self: inherit", and we're the root node [parentContext is null], and I think we're reacting by trying to resolve to the initial value.  The initial value is 'auto', which now computes to 'normal' on the root node -- hence, I think this should be 'normal', not 'start'.)

Regardless, consider adding a comment justifying why we use the value that we do use in this case, since this seems like a pretty obscure case where "inherit" & "auto" are both being bypassed due to the lack-of-a-parent.

@@ +8137,1 @@
>      }

I don't think you need this "if/else" around parentContext -- your nsStylePosition::ComputedJustifyItems() impl already handles a null arg. (though it doesn't yet have the right return value for this case, as noted towards the bottom of this comment)

@@ +8157,2 @@
>      } else {
>        pos->mJustifySelf = NS_STYLE_JUSTIFY_START;

(similar to note above for mAlignSelf, but now for mJustifySelf):

Should we be using _NORMAL (not _START) here, in the "if (parentContext)" else-clause? And as above, consider adding a comment here (or a reference pointing to the requested code-comment in the analogous mAlignSelf section).

::: layout/style/nsStyleStruct.cpp
@@ +1655,5 @@
>      MOZ_ASSERT(!(parentAlignItems & NS_STYLE_ALIGN_LEGACY),
>                 "align-items can't have 'legacy'");
>      return parentAlignItems;
>    }
>    return NS_STYLE_ALIGN_START;

Per Tab's response to your question, this should now be _NORMAL, not _START.
References:
 https://lists.w3.org/Archives/Public/www-style/2016Jan/0015.html
 https://hg.csswg.org/drafts/rev/3505b7fd4adb

@@ +1671,2 @@
>      if (inheritedJustifyItems & NS_STYLE_JUSTIFY_LEGACY) {
>        return inheritedJustifyItems;

This is now the most special-casey "ComputedXYZ" function here.

Because this is special-casey and because "legacy" is weird, it might be worth quoting the spec-text for this in a code-comment, to get the high-level idea across in English.

In particular, this spec-quote might help somewhere here:
  // "If the inherited value of justify-items includes the 'legacy' keyword,
  // 'auto' computes to the inherited value.
  // Otherwise, 'auto' computes to 'normal'.
https://drafts.csswg.org/css-align/#propdef-justify-items

@@ +1687,3 @@
>      return inheritedJustifyItems & ~NS_STYLE_JUSTIFY_LEGACY;
>    }
>    return NS_STYLE_JUSTIFY_START;

As above, this should now be _NORMAL, not _START.

Comment 6

[Mats Palmgren (inactive)]

(In reply to Daniel Holbert [:dholbert] from comment #5)
> >        pos->mAlignSelf = NS_STYLE_ALIGN_START;
> 
> Why is this _START? Should this be _NORMAL now?

Yes, this is the issue I posted to www-style@ about.
Tab agreed 'normal' would be better and has already updated
the spec accordingly.  (You saw the updated spec judging
from your comment.)

I fixed this before landing.

> I don't think you need this "if/else" around parentContext -- your
> nsStylePosition::ComputedJustifyItems() impl already handles a null arg.

It appeared to be unnecessary, but that was just a bug.  Fixed like so:

     if (MOZ_LIKELY(parentContext)) {
-      pos->mJustifyItems = parentPos->ComputedJustifyItems(parentContext);
+      pos->mJustifyItems =
+        parentPos->ComputedJustifyItems(parentContext->GetParent());

(I think the old code does the right thing - the first recursion
in ComputedJustifyItems() would just be redundant)

> Should we be using _NORMAL (not _START) here, in the "if (parentContext)"
> else-clause?

Yes, per the updated spec.


> ::: layout/style/nsStyleStruct.cpp
> In particular, this spec-quote might help somewhere here:
>   // "If the inherited value of justify-items includes the 'legacy' keyword,
>   // 'auto' computes to the inherited value.
>   // Otherwise, 'auto' computes to 'normal'.

OK, sure.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6c0ccd4c3566
https://hg.mozilla.org/integration/mozilla-inbound/rev/d014869cc5bf

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6c0ccd4c3566
https://hg.mozilla.org/mozilla-central/rev/d014869cc5bf