Mats Palmgren (inactive) Reporter
	Description • 8 years ago

https://drafts.csswg.org/css-align-3/#overflow-values

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

(Hope you don't mind if I steal this bug to assign it to :mihir, who's helping with CSS Flexbox improvements this summer.)

	Brad Werth [:bradwerth]
	Comment 2 • 6 years ago

That's great; thank you, and good luck with it.

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

Here's a testcase for this bug, to make things more concrete.  Ideally the grid & flex examples here should look the same -- but they don't. We center in a way that causes "unsafe" overflow (off the start side) for the flex container.

FWIW, Chrome and Edge match with our rendering here - so it seems like no one has implemented "safe" overflow for flex containers yet.

	Daniel Holbert [:dholbert]
	Comment 5 • 6 years ago
mozreview-review

Comment on attachment 8992381 [details]
Bug 1297774 - Implement safe/unsafe for flexbox 'justify-content' and 'align-{content,self,items}'

https://reviewboard.mozilla.org/r/257234/#review264200

This is close! Likely r+ after feedback is addressed, but I'll tag it r- to take one more look at that point.

::: commit-message-e951f:1
(Diff revision 1)
> +Bug 1297774 - Implement safe/unsafe for flexbox 'justify-content' and
> +              'align-content'

Two nits:
 (1) Mention align-items (and/or align-self) here, since it looks like you're fixing that as well.  If you like, you could say "align-{content,self,items}" for brevity.

 (2) Don't wrap this to two lines -- the first line of the commit message needs to contain the whole...
 Bug NNN: [summary] r=whoever
...syntax.  Any additional sentences of  (optional) explanation can go in subsequent lines if there's any more information to add.  (HG itself (in "hg log") along with several of our hg-backed web tools (e.g. treeherder) will parse out & display this first line as the "short summary" of the commit. So it needs to be complete to avoid making those views look broken.)

::: layout/generic/nsFlexContainerFrame.cpp:586
(Diff revision 1)
>    WritingMode GetWritingMode() const { return mWM; }
>    uint8_t GetAlignSelf() const     { return mAlignSelf; }
> +  uint8_t GetAlignSelfFlags() const     { return mAlignSelfFlags; }

Nit: drop the extra spaces before "{" in the new line here -- just 1 space between "const" and "{" is all you need.

(The line above it probably has extra spaces because it was nicely lined up with its surrounding code when it first landed, probably.  But alas, something changed such that it's no longer aligned. Anyway, there's no use in copypasting its arbitrary number of extra spaces into this new line of code in a non-lined-up fashion. :) Feel free to drop the extra spaces in the line above (for GetAlignSelf), too, if you like.)

::: layout/generic/nsFlexContainerFrame.cpp:2953
(Diff revision 1)
>    if (mPackingSpaceRemaining <= 0) {
>      // No available packing space to use for resolving auto margins.
>      mNumAutoMarginsInMainAxis = 0;
>    }
>  
> +  // If packing space is negative and 'overflow-position' is set to 'safe'
> +  // all justify options fall back to 'flex-start'.
> +  if (mPackingSpaceRemaining < 0 &&
> +      justifyContentFlags == NS_STYLE_JUSTIFY_SAFE) {
> +    mJustifyContent = NS_STYLE_JUSTIFY_FLEX_START;
> +  }

Three things:
 (1) There may be more flags in the future, so you should check the flags using & rather than ==.  See nsGridContainerFrame.cpp for reference.

 (2) You should merge your new code into the clause above this, to avoid checking mPackingSpaceRemaining extra times unnecessarily.  Conveniently, the mPackingSpaceRemaining==0 scenario is a special case where all alignment modes are equivalent -- so you can share the <=0 check above rather than needing to specifically check for negative values.

So really, I think you can just insert into the check above, with:
  if (mPackingSpaceRemaining <= 0) {
    ..
    // [BEGIN NEW CODE HERE]
    if (justifyContentFlags & NS_STYLE_JUSTIFY_SAFE) {
      ...
    } // [END NEW CODE HERE
  }

 (3) We should be falling back to "start" here, not to "flex-start". Please fix that in the comment & code (i.e. use NS_STYLE_JUSTIFY_START here). Right now they behave the same, but they will have subtle differences as of bug 1472843.

::: layout/generic/nsFlexContainerFrame.cpp:3175
(Diff revision 1)
> +  if (alignContentFlags == NS_STYLE_ALIGN_SAFE && mPackingSpaceRemaining < 0) {
> +    mAlignContent = NS_STYLE_ALIGN_FLEX_START;
> +  }

As noted above: use "&" rather than == (and add parens so that the "&" vs "&&" grouping is easy to visually parse).

With that, this line will end up barely too long, so please wrap after the &&. So this should end up looking like:
  if ((alignContentFlags & NS_STYLE_ALIGN_SAFE) &&
      mPackingSpaceRemaining < 0) {


And then, inside this clause, please make us fall back to _START (not _FLEX_START) and fix the comment to mention "start" (not flex-start) too.

(We could perhaps merge this with the logic below it, as well, but maybe better not to in this case because not all of the clauses below care about mPackingSpaceRemaining.)

::: layout/generic/nsFlexContainerFrame.cpp:3537
(Diff revision 1)
> +  // 'align-self' falls back to 'flex-start' if it is 'center'/'flex-end' and we
> +  // have cross axis overflow
> +  if (aLine.GetLineCrossSize() < aItem.GetOuterCrossSize(mAxis)
> +      && aItem.GetAlignSelfFlags() == NS_STYLE_ALIGN_SAFE
> +      && (alignSelf == NS_STYLE_ALIGN_CENTER
> +          || alignSelf == NS_STYLE_ALIGN_FLEX_END)) {
> +    alignSelf = NS_STYLE_ALIGN_FLEX_START;
> +  }
> +

Can we check for != STRETCH (excluding that value), rather than explicitly checking for "center"/"end" values?  That feels more future-proof, I think.

Also: unlike the other cases above, I think we really *do* need to use "flex-start" here, because our fake "start"-handling logic has already converted "start" into "flex-start" at this point (a few lines above).  So it's fine that we're using FLEX_START  in this case, but please include a comment saying "// XXX we should really be falling back to 'start' as of bug 1472843". (This will increase the chances that we'll remember to update this code after we've fixed that bug and implemented "start" for real with its nuances that differ from flex-start.)

Also, please put the "&&" and "||" operators at the ends of lines rather than at the beginnings of lines, so that consecutive logical terms are nicely vertically-aligned with each other.  (See first paragraph of "Operators" section of https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style )

::: layout/reftests/w3c-css/submitted/flexbox/flexbox-safe-overflow-position-001-ref.html:8
(Diff revision 1)
> +    Any copyright is dedicated to the Public Domain.
> +    http://creativecommons.org/publicdomain/zero/1.0
> +-->
> +<html>
> +<head>
> +  <title>Reference: Testing safe overflow-position for align-content, justify-content, and align-item(s) in flex containers</title>

Drop the parens around the "s" in "align-items" here.

(This affects both HTML files here - please fix both.)

(I think this was from you thinking the singular property "align-self" might be  called "align-item" :)  Anyway, no need to allude to that property in the title, in part because your testcase doesn't use it by name at all [and doesn't need to].)

::: layout/reftests/w3c-css/submitted/flexbox/flexbox-safe-overflow-position-001-ref.html:9
(Diff revision 1)
> +    http://creativecommons.org/publicdomain/zero/1.0
> +-->
> +<html>
> +<head>
> +  <title>Reference: Testing safe overflow-position for align-content, justify-content, and align-item(s) in flex containers</title>
> +  <link rel="author" title="Mihir Iyer" href=?mailto:miyer@mozilla.com">

The question mark here should be a double-quote character.

(This affects both HTML files here - please fix both.)

::: layout/reftests/w3c-css/submitted/flexbox/flexbox-safe-overflow-position-001-ref.html:49
(Diff revision 1)
> +    .alignContentStart {
> +      align-content: flex-start;
> +    }
> +    .justifyContentStart {
> +      justify-content: flex-start;
> +    }
> +    .alignSelfStart {
> +      align-self: flex-start;
> +    }

Let's use "start" (not "flex-start") here to be slightly more correct (per spec) about what the reference rendering should be.

(I believe it doesn't actually make a difference here, rendering-wise.)

::: layout/reftests/w3c-css/submitted/flexbox/flexbox-safe-overflow-position-001-ref.html:61
(Diff revision 1)
> +  <div class="initialBlock"></div>
> +  <div class="flex rowNoWrap justifyContentStart">

I think this "initialBlock" is only here to create blank space, right? (so that overflow is visible & doesn't overlap other stuff)

If that's the case, let's get rid of this block and instead just put a "margin-top" value in your .flex rule (to allocate some margin before/between the flex containers).

::: layout/reftests/w3c-css/submitted/flexbox/reftest.list:217
(Diff revision 1)
>  
>  # Tests for "display:flex" on root node
>  == flexbox-root-node-001a.html flexbox-root-node-001-ref.html
>  == flexbox-root-node-001b.html flexbox-root-node-001-ref.html
>  
> +# Tests for 'overflow-position' safe

This sounds like "overflow-position" is a CSS property. Let's clarify to:

# Tests for <overflow-position> "safe" keyword in CSS Alignment properties

	Mihir Iyer [:mihir] Assignee
	Comment 6 • 6 years ago
mozreview-review

Comment on attachment 8992381 [details]
Bug 1297774 - Implement safe/unsafe for flexbox 'justify-content' and 'align-{content,self,items}'

https://reviewboard.mozilla.org/r/257234/#review264384

::: layout/reftests/w3c-css/submitted/flexbox/flexbox-safe-overflow-position-001-ref.html:61
(Diff revision 1)
> +  <div class="initialBlock"></div>
> +  <div class="flex rowNoWrap justifyContentStart">

Fixed in both files.

	Daniel Holbert [:dholbert]
	Comment 9 • 6 years ago
mozreview-review

Comment on attachment 8992381 [details]
Bug 1297774 - Implement safe/unsafe for flexbox 'justify-content' and 'align-{content,self,items}'

https://reviewboard.mozilla.org/r/257234/#review264432

::: layout/generic/nsFlexContainerFrame.cpp:2956
(Diff revisions 1 - 3)
>    mPackingSpaceRemaining -= aLine->GetSumOfGaps();
>  
>    if (mPackingSpaceRemaining <= 0) {
>      // No available packing space to use for resolving auto margins.
>      mNumAutoMarginsInMainAxis = 0;
> +    // If packing space is negative and 'overflow-position' is set to 'safe

Two things:

 (1) Add single-quote at the end of the line here (after 'safe)

 (2) Throughout your changes in this .cpp file, let's replace the single-quoted term 'overflow-position' with alligator-brackets <overflow-position>, to more clearly match the CSS spec* and to make it look less like this is a named CSS property or keyword.

(In CSS spec-writing, quoted terms like 'foo' are usually a property or a keyword, whereas alligator-bracketed <foo> represents a type.  overflow-position is the latter -- it's a type.)

* https://drafts.csswg.org/css-align/#typedef-overflow-position

::: layout/generic/nsFlexContainerFrame.cpp:3538
(Diff revisions 1 - 3)
> -  if (aLine.GetLineCrossSize() < aItem.GetOuterCrossSize(mAxis)
> -      && aItem.GetAlignSelfFlags() == NS_STYLE_ALIGN_SAFE
> -      && (alignSelf == NS_STYLE_ALIGN_CENTER
> -          || alignSelf == NS_STYLE_ALIGN_FLEX_END)) {
> +  // XXX we should really be falling back to 'start' as of bug 1472843
> +  if (aLine.GetLineCrossSize() < aItem.GetOuterCrossSize(mAxis) &&
> +      (aItem.GetAlignSelfFlags() & NS_STYLE_ALIGN_SAFE) &&
> +      !(alignSelf & NS_STYLE_ALIGN_STRETCH)) {

Do we actually need to care about the align-self value here?  Previously you were checking for == flex-end or center, which I'd suggested simplifying to !=stretch, but now I'm wondering whether we should even be doing that...

I don't see any spec support for this, offhand, so maybe just get rid of this last condition?

Or: if we do need it for some reason, please make it clear why in a code-comment here -- and in that case, please also check for STRETCH using == rather than &. This last comparison (checking alignSelf for STRETCH) should still use equality (or rather, inequality !=), rather than bitmasking with &.  (The alignSelf variable here is not a bitfield (once we've removed the flag bits, as we have by this point) -- it's an enumerated value, and some of its enumerated possibilities may be confused with others if we treat them as masks (e.g. the value 0x1 would get matched by any enum value that has the final bit set, if we were checking using &).

	Pulsebot
	Comment 11 • 6 years ago

Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/836cb5c95549
Implement safe/unsafe for flexbox 'justify-content' and 'align-{content,self,items}' r=dholbert

	Eliza Balazs [:ebalazs_]
	Comment 12 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/836cb5c95549

	Rachel Andrew
	Comment 13 • 6 years ago

I have been working on the documentation for this update.

Updated (to edit the descriptions of safe and unsafe):
https://developer.mozilla.org/en-US/docs/Web/CSS/align-self
https://developer.mozilla.org/en-US/docs/Web/CSS/align-content
https://developer.mozilla.org/en-US/docs/Web/CSS/justify-content

Compat data also updated and has been pushed to MDN so is appearing on those pages.

I would appreciate a review of those changes, thanks!

	Oana Botisan, Desktop QA
	Comment 14 • 6 years ago

I managed to reproduce the issue using the test case attached to comment 3 on an older version of Nightly (2018-06-15).
I retested everything using beta 63.0b7 and latest Nightly 64.a1 on Windows 10 x64, Ubuntu 16.04 and macOS 10.13. The bug is not reproducing anymore.