Closed Bug 1233106 Opened 5 years ago Closed 5 years ago

implement changes to css-align property computation

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: dbaron, Assigned: mats)

References

Details

(Keywords: dev-doc-needed)

Attachments

(2 files)

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)
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 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.
Attachment #8703447 - Flags: review?(dholbert) → review+
(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.
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/6c0ccd4c3566
https://hg.mozilla.org/mozilla-central/rev/d014869cc5bf
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.