Assertion failure: "Grid's 'align-self' axis is never parallel to the container's inline axis"

RESOLVED FIXED in Firefox 45

Status

()

Core
Layout
--
critical
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: Jesse Ruderman, Assigned: mats)

Tracking

(Blocks: 2 bugs, {assertion, testcase})

Trunk
mozilla45
assertion, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox45 fixed)

Details

Attachments

(5 attachments, 4 obsolete attachments)

(Reporter)

Description

3 years ago
Created attachment 8688237 [details]
testcase

Assertion failure: alignSelf != 6 && alignSelf != 7 (Grid's 'align-self' axis is never parallel to the container's inline axis, so these should've computed to 'start' already), at layout/generic/nsGridContainerFrame.cpp:1019
(Reporter)

Comment 1

3 years ago
Created attachment 8688238 [details]
stack
(Assignee)

Updated

3 years ago
Assignee: nobody → mats
(Assignee)

Comment 2

3 years ago
Created attachment 8689501 [details] [diff] [review]
part 1 - [css-align] Map align/justify-self:left/right to start per spec (not as if they were align/justify-content values)

I think we need to map *-self left/right differently than for *-content
because for the former the style is for an alignment subject (e.g.
grid item, with its parent as the alignment container), whereas for
*-content the style is for an alignment container (e.g. a grid
container with its track as the alignment *subjects*).
Attachment #8689501 - Flags: review?(dholbert)
(Assignee)

Comment 3

3 years ago
Created attachment 8689502 [details] [diff] [review]
part 2 - [css-align] Make align/justify-items:left/right compute to themselves.

I've also changed my mind regarding mapping left/right for
[align/justify]-items.  I don't think we should do that at all
because it's only *-content and *-self values that are actually used
for aligning the alignment subjects.

As the spec says:
https://drafts.csswg.org/css-align-3/#justify-items-property
"This property specifies the default justify-self for all of the boxes..."

and:
https://drafts.csswg.org/css-align-3/#valdef-self-position-left
"Aligns the alignment subject to be flush with the alignment container’s line-left edge. If the property’s axis is not parallel with the inline axis, this value computes to start."

I think it's implicit that "property" in "If the property’s axis" can
only be *-self or *-content because those are the only ones used
for "Aligns the alignment subject...".  So that excludes *-items.

Also, it's unintuitive to map the value twice and it would be somewhat
unpredictable.
Attachment #8689502 - Flags: review?(dholbert)
(Assignee)

Comment 4

3 years ago
Created attachment 8689503 [details] [diff] [review]
part 3 - [css-align] Add mochitests for align/justify-items/self/content computed values
(Assignee)

Comment 5

3 years ago
Created attachment 8689505 [details] [diff] [review]
part 4 - [css-grid] Crashtest
Comment on attachment 8689501 [details] [diff] [review]
part 1 - [css-align] Map align/justify-self:left/right to start per spec (not as if they were align/justify-content values)

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

r=me; two questions below, address as you see fit.

::: layout/style/nsStyleStruct.cpp
@@ +1716,5 @@
> +         aParentDisplay->mDisplay == NS_STYLE_DISPLAY_INLINE_FLEX) &&
> +        !aDisplay->IsAbsolutelyPositionedStyle()) {
> +      // XXX TODO
> +      // NOTE: make sure to strip off 'legacy' bit when mapping to 'start'
> +    } else {

I'm not clear on why this flex-item special-case needs to exist, but I won't hold up this review on it, since this is copypasted from existing code (MapLeftRightToStart) which I also have the same question about.

Could you post a bit more about why we need this special-case? (either here, or in bug 1207698 if that's where this special-case should be sorted out, or in a new bug)

(I'm guessing you're wanting to avoid caring about LogicalAxis aAxis in a flex container here, and that supeficially makes sense, because flex containers really should care about {main|cross}-axis, not {inline|block}-axis.  *However* -- in truth, aAxis here **isn't inherently a signal for block vs. inline**, despite the fact that it holds those values. The way you're using it in the callers, it's just a proxy for whether we're handling justify-* vs. align-*.  And from that perspective, I'd expect flexbox should behave the same as any other container here. [and we should maybe consider reworking aAxis to be a different sort of enum, maybe explicitly having values {eAlign, eJustify}.)

(also: if the above parenthetical happens to convince you that we don't really need this special case, then feel free to drop the special-case from both Map...ToStart functions as part of this bug.)

::: layout/style/nsStyleStruct.h
@@ +1466,4 @@
>    uint8_t MapLeftRightToStart(uint8_t aAlign, mozilla::LogicalAxis aAxis,
>                                const nsStyleDisplay* aDisplay) const;
> +  // Helper for the ComputedAlign/JustifySelf* methods.
> +  uint8_t MapSelfLeftRightToStart(uint8_t aAlign, mozilla::LogicalAxis aAxis,

Should we rename the other function here ("MapLeftRightToStart") to have "Content" in the name, for symmetry with this new one (named "MapSelfLeftRightToStart")?

(since one is for *-content, and the other is for *-self)
Attachment #8689501 - Flags: review?(dholbert) → review+
(Assignee)

Comment 8

3 years ago
> Could you post a bit more about why we need this special-case?

fantasai said:
https://lists.w3.org/Archives/Public/www-style/2015Sep/0209.html
"This is true except in flex layout, because in flex layout
the align/justify mapping depends on the flex direction, not
the writing mode."

So we need to compare the passed in aAxis and some Flexbox
properties to determine when to map these values to 'start', IIUC.

> Should we rename the other function here ("MapLeftRightToStart") to have
> "Content" in the name

Yeah, I'll do that if you agree with "part 2" here.
I don't think I agree with "part 2".

(In reply to Mats Palmgren (:mats) from comment #3)
> Created attachment 8689502 [details] [diff] [review]
> part 2 - [css-align] Make align/justify-items:left/right compute to
> themselves.
> 
> I've also changed my mind regarding mapping left/right for
> [align/justify]-items.  I don't think we should do that at all
> because it's only *-content and *-self values that are actually used
> for aligning the alignment subjects.

But the computed value of *-items is also exposed via getComputedStyle (and via traditional inheritance). It seems weird that *-self would have this special "left|right unilaterally compute to start sometimes" behavior, while *-items would never do that.  (Sure, *-items is only _used_ via *-self.  But its computed value is inspectable separately from that.  And if "left" is defined as computing a particular way in these properties [as it is], then we need to make it behave that way when it's inspected.)
 
> https://drafts.csswg.org/css-align-3/#valdef-self-position-left
> "Aligns the alignment subject to be flush with the alignment container’s
> line-left edge. If the property’s axis is not parallel with the inline axis,
> this value computes to start."
> 
> I think it's implicit that "property" in "If the property’s axis" can
> only be *-self or *-content because those are the only ones used
> for "Aligns the alignment subject...".  So that excludes *-items.

I disagree that this is implicit.  Computed-value-resolution is completely separate from when/where the value is actually used, so I don't think your logic here follows.

If what you're saying is *really* the intent of the spec authors (and I'm dubious about that at this point), it absolutely needs to be made explicit in the spec.  If I haven't changed your mind, could you email www-style to sanity-check your interpretation & get this [align|justify]-items exemption to be mentioned explicitly here in the spec section for "left" that you quoted?

> Also, it's unintuitive to map the value twice and it would be somewhat
> unpredictable.

I agree with you here, from an internal-usage-in-layout perspective. But I don't think that means we should change what getComputedStyle returns. (or what's inherited)
(In reply to Mats Palmgren (:mats) from comment #8)
> > Could you post a bit more about why we need this special-case?
> 
> fantasai said:

(Ah, I see this is bug 1221565 & you've just posted a bit more detail there. Thanks!)
(Assignee)

Comment 11

3 years ago
> could you email www-style to sanity-check your interpretation

I'll do so, but unfortunately it usually takes months to get an answer there... :-(
For now, I'd say let's punt on part 2 (and consider it a separate bug, to the extent that it is a bug). It's something that we can easily fix after enabling grid I'd think, without much likelihood of breaking anything.

(And for now, we'll be implementing the letter of the spec [which is IMO also the intent of the spec in this case, though I think you disagree].)
(point of comment 12 being: that www-style post/response doesn't need to hold up the fixing of this bug here.)
Comment on attachment 8689502 [details] [diff] [review]
part 2 - [css-align] Make align/justify-items:left/right compute to themselves.

(Marking part 2 r- for now, per comment 9 & comment 12)
Attachment #8689502 - Flags: review?(dholbert) → review-
(Assignee)

Comment 15

3 years ago
OK, that sounds reasonable, let's handle the *-items part in a separate bug after
we get clarification from www-style.
(Assignee)

Comment 16

2 years ago
On 11/23/2015 02:25 PM, Mats Palmgren wrote:
> On 11/21/2015 06:30 AM, fantasai wrote:
>> I think the best solution here would be to not compute left/right
>> to start, but to treat it as start when we're operating in the
>> wrong axis.
>
> Seems reasonable to me, please update the CSS Align spec.
>
> Just to confirm I understand you correctly: you're saying that all
> specified values containing 'left'/'right' should compute to the
> specified value, and that the used value is the computed value
> except when the property applies to the block axis in which case
> the used value is the computed value with the 'left'/'right'
> keyword replaced by 'start'.  And this applies to all align/
> justify properties specified in the CSS Align spec.
> Correct?

Correct.

~fantasai
(Assignee)

Comment 17

2 years ago
So, based on the above I will instead remove the mapping of left/right in
the style system and add code for that in layout instead.
Sounds good. Thanks!
(Assignee)

Comment 19

2 years ago
Created attachment 8691398 [details] [diff] [review]
part 1 - [css-align] Don't compute left/right to start in the style system anymore (due to pending spec change).  Map the used value instead (in layout).
Attachment #8689501 - Attachment is obsolete: true
Attachment #8689502 - Attachment is obsolete: true
Attachment #8691398 - Flags: review?(dholbert)
(Assignee)

Comment 20

2 years ago
Created attachment 8691399 [details] [diff] [review]
part 2 - [css-align] Add mochitests for align/justify-items/self/content computed values.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb0e30748dc2
Attachment #8689503 - Attachment is obsolete: true
(Assignee)

Comment 21

2 years ago
Created attachment 8691401 [details] [diff] [review]
part 3 - [css-grid] Crashtest.
Attachment #8689505 - Attachment is obsolete: true
Comment on attachment 8691398 [details] [diff] [review]
part 1 - [css-align] Don't compute left/right to start in the style system anymore (due to pending spec change).  Map the used value instead (in layout).

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

r=me, with non-<TANGENT> comments addressed.

::: layout/generic/nsGridContainerFrame.cpp
@@ +1021,5 @@
>    bool overflowSafe = alignSelf & NS_STYLE_ALIGN_SAFE;
>    alignSelf &= ~NS_STYLE_ALIGN_FLAG_BITS;
> +  if (alignSelf == NS_STYLE_ALIGN_LEFT || alignSelf == NS_STYLE_ALIGN_RIGHT) {
> +    alignSelf = NS_STYLE_ALIGN_START;
> +  }

Could you preserve a version of the assertion message here as a code-comment?

e.g.:
    // Grid's 'align-self' axis is never parallel to the container's
    // inline axis, so 'left' & 'right' (which are inline-specific)
    // produce a used value of "start".

@@ +1097,5 @@
>      case NS_STYLE_ALIGN_LEFT:
>      case NS_STYLE_ALIGN_RIGHT: {
> +      if (aIsAlign) {
> +        return NS_STYLE_ALIGN_START;
> +      }

Same here.

::: layout/style/nsRuleNode.cpp
@@ +7865,5 @@
>    // align-content: enum, inherit, initial
>    const auto& alignContentValue = *aRuleData->ValueForAlignContent();
>    if (MOZ_UNLIKELY(alignContentValue.GetUnit() == eCSSUnit_Inherit)) {
>      if (MOZ_LIKELY(parentContext)) {
> +      pos->mAlignContent = parentPos->ComputedAlignContent();

Do we need this special case at all here anymore? I think we can just defer to the normal inheritance codepath, via SetDiscrete() (which we call a few lines further down).

::: layout/style/nsStyleStruct.cpp
@@ +1725,5 @@
>        // XXX maybe map 'auto' too? (ISSUE 8 in the spec)
>        // https://drafts.csswg.org/css-align-3/#content-distribution
>        if ((mJustifyContent & NS_STYLE_ALIGN_ALL_BITS) ==
>            NS_STYLE_JUSTIFY_STRETCH) {
>          return NS_STYLE_JUSTIFY_FLEX_START;

<TANGENT>
Note: I suspect we should do away with this computed-value special-case as well (mapping 'stretch' to 'flex-start') and just handle this in layout. Emailed www-style to confirm:
  https://lists.w3.org/Archives/Public/www-style/2015Nov/0327.html

We can handle this in a separate bug, in any case -- but at that point, we may want to get rid of the ComputedAlignContent() & ComputedJustifyContent() methods entirely, because they'll both just be simple accessors for already-public member vars. Unlike the ComputedXYZ{Self|Items} methods, we'll have no special logic in these Content-flavored methods.
</TANGENT>
Attachment #8691398 - Flags: review?(dholbert) → review+
(I filed bug 1227751 on my <TANGENT>, RE killing the "justify-content:stretch" special case, btw.)
(Assignee)

Comment 25

2 years ago
> Do we need this special case at all here anymore?

Nope, good catch.  I removed the special inheritance code there and just use
SetDiscrete() as you suggested.
Flags: in-testsuite+

Comment 26

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d15205791d9b
https://hg.mozilla.org/mozilla-central/rev/ff70bbc58b6c
https://hg.mozilla.org/mozilla-central/rev/07f9701e551d
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox45: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.