[css-flexbox][css-align] The used value for 'left'/'right' is 'start' for the block-axis

RESOLVED FIXED in Firefox 52

Status

()

defect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: mats, Assigned: bradwerth)

Tracking

(Blocks 1 bug)

unspecified
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(9 attachments)

58 bytes, text/x-review-board-request
dholbert
: review+
Details
58 bytes, text/x-review-board-request
dholbert
: review+
Details
58 bytes, text/x-review-board-request
dholbert
: review+
Details
58 bytes, text/x-review-board-request
dholbert
: review+
Details
58 bytes, text/x-review-board-request
dholbert
: review+
Details
58 bytes, text/x-review-board-request
dholbert
: review+
Details
58 bytes, text/x-review-board-request
dholbert
: review+
Details
58 bytes, text/x-review-board-request
dholbert
: review+
Details
58 bytes, text/x-review-board-request
dholbert
: review+
Details
Reporter

Description

4 years ago
https://drafts.csswg.org/css-align-3/#valdef-self-position-left
"If the property’s axis is not parallel with the inline axis, this value computes to 'start'."

The Flexbox part is missing here:
http://mxr.mozilla.org/mozilla-central/source/layout/style/nsStyleStruct.cpp?rev=5ed5ee646acd#1679

Daniel, I'm assigning this to you since I figured it's faster
for you to write this piece.  I'm guessing it's just a few lines.
Reporter

Comment 1

4 years ago
FYI, I'm tweaking this code a bit to fix bug 1225376.
Reporter

Comment 2

4 years ago
FYI, this is what fantasai replied when I asked her about it:
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.

(This is why they're named so abstractly--we couldn't map them
to the inline and block axes always.)
"

The aAxis you get here is eLogicalAxisBlock for the align-* properties
and eLogicalAxisInline for the justify-* properties.  The missing code
should determine if that property applies to the block-axis and map
left/right to start in that case.
Reporter

Comment 3

4 years ago
Changing summary due to pending CSS Align spec change:
https://lists.w3.org/Archives/Public/www-style/2015Nov/0300.html

'left'/'right' should be mapped in layout instead (for the properties
that apply to the block-axis).

(Bug 1225376 will fix the style system so that left/right compute to
themselves.)
Summary: [css-flexbox][css-align] 'left'/'right' should compute to 'start' for the block-axis → [css-flexbox][css-align] The used value for 'left'/'right' is 'start' for the block-axis

Comment 5

3 years ago
mozreview-review
Comment on attachment 8798235 [details]
Bug 1221565 Part 1: Make nsFlexContainerFrame map align-self values of 'left' and 'right' to either 'start' or 'end'.

https://reviewboard.mozilla.org/r/83758/#review82328

Are you sure it's worth adding this new argument to UsedAlignSelf?  I think we already handle this gracefully in nsGridContainerFrame, for example:
 https://dxr.mozilla.org/mozilla-central/source/layout/generic/nsGridContainerFrame.cpp#2646

It doesn't really benefit from having to pass this variable every time it calls UsedAlignSelf().

Perhaps the only place where we need special treatment is in nsFlexContainerFrame? And in that case, it seems like this logic should all live there.

::: layout/generic/nsFlexContainerFrame.cpp:1724
(Diff revision 1)
>    } else {
> +    mozilla::LogicalAxis inlineAxis = eLogicalAxisInline;
> +    if (mFrame->GetType() == nsGkAtoms::flexContainerFrame) {
> +      nsFlexContainerFrame* flexFrame = static_cast<nsFlexContainerFrame*>(mFrame);
> +      if (!flexFrame->IsHorizontal()) {
> +        inlineAxis = eLogicalAxisBlock;

A few things:
 (1) we should be looking at the flex container here (containerRS->mFrame), not the flex item.  (The flex item might happen to also be a flex container -- that's what this patch seems to be currently checking for -- but that shouldn't impact its alignment in the outer flex container.)

 (2) Don't check "IsHorizontal" here.  The spec says we should condition on whether "the [align-self] property’s axis is not parallel with the [alignment container's] inline axis".  (The square-braces there are my own interpretation, which I believe is the reasonable interpretation.)  So: I think aAxisTracker.IsRowOriented() is the correct thing to condition on here. (or alternately, you could check, IsColumnOriented(), which is the exact opposite.)  That tells you whether the align-self axis (the flex container's cross axis) is parallel to the flex container's inline axis or not.  (If we're column-oriented, then the align-self axis *is* parallel to the inline axis. If we're row-oriented, it's not.)
Attachment #8798235 - Flags: review?(dholbert) → review-
(In reply to Daniel Holbert [:dholbert] from comment #5)
> A few things:
>  (1) we should be looking at the flex container here (containerRS->mFrame),
> not the flex item.

To clarify this further -- I don't think we *actually* need to bother inspecting containerRS->mFrame here, because we should have all the information we need captured in the |aAxisTracker| parameter (particularly aAxisTracker.IsRowOriented(), as noted in (2)).
Assignee

Comment 7

3 years ago
I see. I'm working on an alternative patch that moves the relevant logic into the FlexItem constructor and out of the UsedAlignSelf function, as you suggest.
Assignee

Updated

3 years ago
Assignee: dholbert → bwerth
Comment hidden (mozreview-request)
Assignee

Comment 9

3 years ago
Posted the alternate implementation requested in comment 5. While considering this approach for the next step of a similar transform for justify-self, I noticed that nsFlexContainerFrame does not appear to directly use a justify-self value. This implies to me that the implementation for flex justify-self will need to reside elsewhere. So, two questions:

1) Where is the right place to implement this change?
2) Does this difference mean that we should change the way we implement the transform for align-self such that it mirrors how justify-self works?
Flags: needinfo?(dholbert)
Fortunately "justify-self" is does not apply to flex items (i.e. it has no effect, on children of a flex container).  Spec support for this is here:
 https://drafts.csswg.org/css-align-3/#justify-flex

"align-self" & "justify-self" are axis-specific ways of saying: "Given the space that the container gave me in this axis, how should I position myself in that space?"

In a flex container, this makes sense in the cross-axis (where "align-self" applies), but it doesn't make sense in the main axis (where the various justify-* properties would apply) -- the flex container entirely controls the items' sizing/positioning in that axis, via "justify-content", and there's no per-item space in which the items could justify themselves in that axis.
Flags: needinfo?(dholbert)

Comment 11

3 years ago
mozreview-review
Comment on attachment 8798235 [details]
Bug 1221565 Part 1: Make nsFlexContainerFrame map align-self values of 'left' and 'right' to either 'start' or 'end'.

https://reviewboard.mozilla.org/r/83758/#review82608

::: layout/generic/nsFlexContainerFrame.cpp:1734
(Diff revision 2)
>  
> +  if (mAlignSelf == NS_STYLE_ALIGN_LEFT || mAlignSelf == NS_STYLE_ALIGN_RIGHT) {
> +    // Find the flex container and determine if it is row oriented; if so,
> +    // then the container's alignment axis is not parallel to the inline axis,
> +    // and so left and right values get mapped to start.
> +    FlexboxAxisTracker axisTracker(containerRS->mFrame, GetWritingMode());

We don't need a new FlexboxAxisTracker here -- we already have one that'll do: the parameter aAxisTracker.

(And your added comment above this about "Find the flex container" can be shortened a bit, too. There's no need to find anything -- the passed-in FlexboxAxisTracker is associated with the flex container, and there's no finding to be done.)

(Sorry for being unclear in comment 5. What I meant to say there was (1) the flex container we care about is containerRS->mFrame, not the child-frame, and (2) we don't actually need to bother touching that frame, because the only information we need from it is *already captured* in aAxisTracker.)

::: layout/generic/nsFlexContainerFrame.cpp:1736
(Diff revision 2)
> +    // Find the flex container and determine if it is row oriented; if so,
> +    // then the container's alignment axis is not parallel to the inline axis,
> +    // and so left and right values get mapped to start.
> +    FlexboxAxisTracker axisTracker(containerRS->mFrame, GetWritingMode());
> +    if (axisTracker.IsRowOriented()) {
> +      mAlignSelf = NS_STYLE_ALIGN_START;

This looks correct, though we might as well also handle the column-oriented case as well here, so that we won't have to bother with Left/Right special-cases at all from this point on.

I think it should be as simple as adding this "else" clause:

} else {
  const bool isLTR =
    containerRS.GetWritingMode().IsBidiLTR();
  const bool isAlignLeft = (mAlignSelf == NS_STYLE_ALIGN_LEFT);
  mAlignSelf = (isAlignLeft == isLTR) ? NS_STYLE_ALIGN_START
                                      : NS_STYLE_ALIGN_END;
}
Attachment #8798235 - Flags: review?(dholbert) → review-
(In reply to Daniel Holbert [:dholbert] from comment #11)
> This looks correct, though we might as well also handle the column-oriented
> case as well here, so that we won't have to bother with Left/Right
> special-cases at all from this point on.

(This is broadening the scope of this bug a bit -- sorry -- but I think we might as well handle these values thoroughly, since it's a logical extension & it's not too much extra work.)

ALSO: sorry for not noticing this sooner, but this align-self fixup work should probably actually happen here, so we've got the fixup code all localized to one place (and so we don't have to concern ourselves with fixing up *all* of the various FlexItem constructors):
https://dxr.mozilla.org/mozilla-central/rev/da986c9f1f723af1e0c44f4ccd4cddd5fb6084e8/layout/generic/nsFlexContainerFrame.cpp#3093

Note that your new code needs to be inserted before the "// Map 'start'/'end' to 'flex-start'/'flex-end'" chunk. (since you'll be producing START/END intermediate values, which that chunk will then map away to FLEX_START or FLEX_END as-needed)

And then please remove the LEFT/RIGHT cases from the switch statement below that point:
https://dxr.mozilla.org/mozilla-central/rev/da986c9f1f723af1e0c44f4ccd4cddd5fb6084e8/layout/generic/nsFlexContainerFrame.cpp#3117-3119
(They won't be needed anymore, since you'll have mapped those values away by then.)
RE other properties, though -- even if we don't need "justify-self", we do need to handle "left" and "right" for "align-content" and "justify-content".

Those chunks should look *extremely* similar to the new "align-self" check -- we have a similar...
>  Map 'start'/'end' to 'flex-start'/'flex-end'.
...conversion chunk for each property in nsFlexContainerFrame.cpp, and we should just convert away LEFT and RIGHT just before that chunk in each case.

Those chunks are:
align-self: (linked above already):
https://dxr.mozilla.org/mozilla-central/rev/da986c9f1f723af1e0c44f4ccd4cddd5fb6084e8/layout/generic/nsFlexContainerFrame.cpp#3100

align-content:
https://dxr.mozilla.org/mozilla-central/rev/da986c9f1f723af1e0c44f4ccd4cddd5fb6084e8/layout/generic/nsFlexContainerFrame.cpp#2808

justify-content:
https://dxr.mozilla.org/mozilla-central/rev/da986c9f1f723af1e0c44f4ccd4cddd5fb6084e8/layout/generic/nsFlexContainerFrame.cpp#2608

align-self & align-content are in the same axis, so the same logic should work for them.  justify-content is in the opposite axis, so I think you'd just want to flip the axisTracker.IsRowOriented() logic for that one -- i.e. for that one, if axisTracker.IsRowOriented() is true, you'd resolve left/right to maybe-start-or-end, else resolve it to start.
Comment hidden (mozreview-request)

Comment 15

3 years ago
mozreview-review
Comment on attachment 8798235 [details]
Bug 1221565 Part 1: Make nsFlexContainerFrame map align-self values of 'left' and 'right' to either 'start' or 'end'.

https://reviewboard.mozilla.org/r/83758/#review82646

Thanks, this looks good! Just one nit, on the commit message:
> nsFlexContainerFrame maps align-self values of 'left' and 'right' to either 'start' or 'end'.
                              ^^^^
It's not at all clear whether this sentence is describing old/bad behavior (being corrected by this patch) vs. new/good behavior (being added in this patch).

To remove ambiguity, please phrase the commit messages in an active tense, to describe the change (rather than simply stating a particular behavior).

So, for example, s/nsFlexContainerFrame maps/Make nsFlexContainerFrame map/ -- that should fix this up.
Attachment #8798235 - Flags: review?(dholbert) → review+
Comment hidden (mozreview-request)
Assignee

Comment 17

3 years ago
Urgh, I missed the removal of the left/right cases mentioned in comment 12. I'll push an updated patch containing that, then work on the part 2 material that is requested in comment 13.
Comment hidden (mozreview-request)

Comment 22

3 years ago
mozreview-review
Comment on attachment 8798664 [details]
Bug 1221565 Part 2: Make nsFlexContainerFrame map justify-content and align-content values of 'left' and 'right' to 'start' or 'end'.

https://reviewboard.mozilla.org/r/84102/#review82652

::: layout/generic/nsFlexContainerFrame.cpp:2612
(Diff revision 1)
>  
> +  // Map 'left'/'right' to 'start'/'end'
> +  if (mJustifyContent == NS_STYLE_ALIGN_LEFT ||
> +      mJustifyContent == NS_STYLE_ALIGN_RIGHT) {
> +    if (aAxisTracker.IsRowOriented()) {
> +      // Container's alignment axis is not parallel to the inline axis,

For "justify-content" (which operates in the opposite axis from align-*), the logic needs to be reversed -- we should do the more-complex start/end mapping when we *ARE* row-oriented (rather than when we're column-oriented).

So, please swap usages of "Column" & "Row" in this chunk -- particularly in the "if" check and in the comment for the "else" clause.

This should end up looking like:
 if (aAxisTracker.IsColumnOriented()) {
    ...
 } else {
   // Row-oriented, so we map [etc]
Two more general things here:
 (1) please be sure to mark off MozReview "issues" (review-feedback) as "Fixed", when you've addressed them. (They're highlighted in a yellow bar above the patch on the MozReview page, as well as in a yellow box at the left edge of each patch at the top of the page.) (MozReview won't let patches be autolanded until all issues have been explicitly marked "fixed" or "dropped".)
 (2) These left/right values will need to be exercised in reftests to verify that they actually work correctly. (We don't have any reftests for "left" & "right" alignment in flexbox right now, so this new code isn't really getting tested right now.) SO: please add "left" & "right" chunks to the following tests in this directory:
https://dxr.mozilla.org/mozilla-central/source/layout/reftests/w3c-css/submitted/flexbox/
 - flexbox-align-content-*
 - flexbox-justify-content-*
 - flexbox-align-self-horiz-001-block.xhtml (and -table since they share a reference)
 - flexbox-align-self-horiz-001-block.xhtml (and -table since they share a reference)
 - flexbox-align-self-vert-001.xhtml
 - flexbox-align-self-vert-rtl-001.xhtml

(maybe other align-self tests, too, but the above tests should cover our bases pretty well)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 39

3 years ago
mozreview-review
Comment on attachment 8798664 [details]
Bug 1221565 Part 2: Make nsFlexContainerFrame map justify-content and align-content values of 'left' and 'right' to 'start' or 'end'.

https://reviewboard.mozilla.org/r/84102/#review83216
Attachment #8798664 - Flags: review?(dholbert) → review+

Comment 40

3 years ago
mozreview-review
Comment on attachment 8799071 [details]
Bug 1221565 Part 3: flexbox-align-content-horiz-* reftests updated.

https://reviewboard.mozilla.org/r/84370/#review83222
Attachment #8799071 - Flags: review?(dholbert) → review+

Comment 41

3 years ago
mozreview-review
Comment on attachment 8799072 [details]
Bug 1221565 Part 4: flexbox-align-content-vert-* reftests updated.

https://reviewboard.mozilla.org/r/84372/#review83224
Attachment #8799072 - Flags: review?(dholbert) → review+

Comment 42

3 years ago
mozreview-review
Comment on attachment 8799073 [details]
Bug 1221565 Part 5: flexbox-justify-content-horiz-* reftests updated.

https://reviewboard.mozilla.org/r/84374/#review83226
Attachment #8799073 - Flags: review?(dholbert) → review+

Comment 43

3 years ago
mozreview-review
Comment on attachment 8799074 [details]
Bug 1221565 Part 6: flexbox-justify-content-vert-* reftests updated.

https://reviewboard.mozilla.org/r/84376/#review83228
Attachment #8799074 - Flags: review?(dholbert) → review+

Comment 44

3 years ago
mozreview-review
Comment on attachment 8799075 [details]
Bug 1221565 Part 7: flexbox-align-self-horiz-001 reftest updated.

https://reviewboard.mozilla.org/r/84378/#review83232

r=me with colors replaced throughout this patch, as noted below:

::: layout/reftests/w3c-css/submitted/flexbox/flexbox-align-self-horiz-001-block.xhtml:82
(Diff revision 2)
> +      .left {
> +        background: red;
> +        align-self: left;
> +      }
> +      .right {
> +        background: green;

Don't use red and green in reftests, except in cases where visible-red means failure and visible-green means success.

(Those colors -- particularly red -- are overloaded with extra meaning for human viewers who might be spot-checking reftest testcases.  This is somewhat documented in this [obsolete-but-still-kinda-valid] csswg test documentation: https://wiki.csswg.org/test/format#design-requirements )

So: please pick some other colors for these new elements, to match those best-practices & to avoid confusing human viewers. Perhaps "tan" and "brown" might be good choices (short, well-understood, not too similar to any other colors already in use in the test).
Attachment #8799075 - Flags: review?(dholbert) → review+
Assignee

Comment 45

3 years ago
Having trouble getting reftests to run when triggered via MozReview (though they've passed locally), so I triggered a try build manually: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7cf2c4a5de0fb5b36b62cce32a121dadba1b34ac

Comment 46

3 years ago
mozreview-review
Comment on attachment 8799076 [details]
Bug 1221565 Part 8: flexbox-align-self-vert-001 reftest updated.

https://reviewboard.mozilla.org/r/84380/#review83240

Similar to the previous patch, r=me with red/green swapped out for other colors:

::: layout/reftests/w3c-css/submitted/flexbox/flexbox-align-self-vert-001.xhtml:77
(Diff revision 2)
> +      .left {
> +        background: red;
> +        align-self: left;
> +      }
> +      .right {
> +        background: green;

(Here's the red/green in this testcase here -- please change these to other colors.)
Attachment #8799076 - Flags: review?(dholbert) → review+

Comment 47

3 years ago
mozreview-review
Comment on attachment 8799077 [details]
Bug 1221565 Part 9: flexbox-align-self-vert-rtl-001 reftest updated.

https://reviewboard.mozilla.org/r/84382/#review83242

And this one as well: r=me with red/green swapped out for other colors.

::: layout/reftests/w3c-css/submitted/flexbox/flexbox-align-self-vert-rtl-001.xhtml:80
(Diff revision 2)
> +      .left {
> +        background: red;
> +        align-self: left;
> +      }
> +      .right {
> +        background: green;

(here's the red/green in this one - please update)
Attachment #8799077 - Flags: review?(dholbert) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

3 years ago
Keywords: checkin-needed

Comment 51

3 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/121afaaeb446
Part 1: Make nsFlexContainerFrame map align-self values of 'left' and 'right' to either 'start' or 'end'. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/e697a85cfb58
Part 2: Make nsFlexContainerFrame map justify-content and align-content values of 'left' and 'right' to 'start' or 'end'. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/2c1bb3de4c5e
Part 3: flexbox-align-content-horiz-* reftests updated. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/ffca0f4dcd4e
Part 4: flexbox-align-content-vert-* reftests updated. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/a646b0385b30
Part 5: flexbox-justify-content-horiz-* reftests updated. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/f2e9bfdb611b
Part 6: flexbox-justify-content-vert-* reftests updated. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/1477ec28601b
Part 7: flexbox-align-self-horiz-001 reftest updated. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/82450e0a06fd
Part 8: flexbox-align-self-vert-001 reftest updated. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/d1f7b01c80f8
Part 9: flexbox-align-self-vert-rtl-001 reftest updated. r=dholbert
Keywords: checkin-needed
Assignee

Updated

3 years ago
See Also: → 1310015
You need to log in before you can comment on or make changes to this bug.