Mats Palmgren (inactive) Reporter
	Description • 9 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.

	Mats Palmgren (inactive) Reporter
	Comment 1 • 9 years ago

FYI, I'm tweaking this code a bit to fix bug 1225376.

	Mats Palmgren (inactive) Reporter
	Comment 2 • 9 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.

	Mats Palmgren (inactive) Reporter
	Comment 3 • 9 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.)

	Daniel Holbert [:dholbert]
	Comment 5 • 8 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.)

	Daniel Holbert [:dholbert]
	Comment 6 • 8 years ago

(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)).

	Brad Werth [:bradwerth] Assignee
	Comment 7 • 8 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.

	Brad Werth [:bradwerth] Assignee
	Comment 9 • 8 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?

	Daniel Holbert [:dholbert]
	Comment 10 • 8 years ago

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.

	Daniel Holbert [:dholbert]
	Comment 11 • 8 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;
}

	Daniel Holbert [:dholbert]
	Comment 12 • 8 years ago

(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.)

	Daniel Holbert [:dholbert]
	Comment 13 • 8 years ago

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.

	Daniel Holbert [:dholbert]
	Comment 15 • 8 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.

	Brad Werth [:bradwerth] Assignee
	Comment 17 • 8 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.

	Brad Werth [:bradwerth] Assignee
	Comment 19 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=033a02bad8c3

	Brad Werth [:bradwerth] Assignee
	Comment 20 • 8 years ago

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

	Daniel Holbert [:dholbert]
	Comment 22 • 8 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]

	Daniel Holbert [:dholbert]
	Comment 23 • 8 years ago

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)

	Daniel Holbert [:dholbert]
	Comment 39 • 8 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

	Daniel Holbert [:dholbert]
	Comment 40 • 8 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

	Daniel Holbert [:dholbert]
	Comment 41 • 8 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

	Daniel Holbert [:dholbert]
	Comment 42 • 8 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

	Daniel Holbert [:dholbert]
	Comment 43 • 8 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

	Daniel Holbert [:dholbert]
	Comment 44 • 8 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).

	Brad Werth [:bradwerth] Assignee
	Comment 45 • 8 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

	Daniel Holbert [:dholbert]
	Comment 46 • 8 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.)

	Daniel Holbert [:dholbert]
	Comment 47 • 8 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)

	Pulsebot
	Comment 51 • 8 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

	Carsten Book [:Tomcat]
	Comment 52 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/121afaaeb446
https://hg.mozilla.org/mozilla-central/rev/e697a85cfb58
https://hg.mozilla.org/mozilla-central/rev/2c1bb3de4c5e
https://hg.mozilla.org/mozilla-central/rev/ffca0f4dcd4e
https://hg.mozilla.org/mozilla-central/rev/a646b0385b30
https://hg.mozilla.org/mozilla-central/rev/f2e9bfdb611b
https://hg.mozilla.org/mozilla-central/rev/1477ec28601b
https://hg.mozilla.org/mozilla-central/rev/82450e0a06fd
https://hg.mozilla.org/mozilla-central/rev/d1f7b01c80f8