1311892 - Honor <overflow-position>: safe | unsafe | unspecified in abspos boxes aligned with CSS Box Alignment

Status: RESOLVED FIXED

(Core :: Layout: Positioned, defect, P3)

Description

[Daniel Holbert [:dholbert]]

In bug 1269046, I'm implementing CSS Alignment for abspos children of a CSS Flexbox (with grid coming in followup bug 1269017).

One part of CSS Alignment is optional the <overflow-position> values (safe|unsafe).  This part of the spec is unstable, and it increases the complexity of the implementation & scenarios-that-need-testing.

So, I'm punting <overflow-position> with abspos children to this bug here, rather than implementing it in bug 1269046. (In bug 1269046, I'll just ignore any specified <overflow-position>.)

(I think this is fine to punt on, since <overflow-position> wasn't present at all in the original spec text for the css-align properties in the CSS Flexbox spec [which means this feature isn't used [much] on the web], and it's also marked as at-risk in the CSS Box Alignment spec, https://drafts.csswg.org/css-align-3/ )

Comment 1

[Julien Cristau [:jcristau]]

Mass wontfix for bugs affecting firefox 52.

Comment 2

[Daniel Holbert [:dholbert]]

Mihir's working on related bug 1297774, so it probably makes sense for him to take this one as well. --> Assigning.

(Hypothetically, if it ends up being simpler to implement all of this in bug 1297774, we can also mark this one as a duplicate after that bug's completed.)

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: testcase 1 (grid + flex - yellow boxes are the abspos ones we're concerned with in this bug)

Comment 4

[Daniel Holbert [:dholbert]]

As you can see in testcase 1, we don't get this right (yet) for position:absolute children of a grid, even though we do get it right for normal (non-abspos) children of a grid.

Comment 5

[Daniel Holbert [:dholbert]]

(Chrome does get this right for grid, though -- they align both the teal (non-abspos) and yellow (abspos) grid child to the start of the containing block.)

Comment 6

[Daniel Holbert [:dholbert]]

Note: when you get started looking at this, nsAbsoluteContainingBlock.cpp's function "OffsetToAlignedStaticPos()" is the main entry point here. We call it twice -- once for each axis.
  https://dxr.mozilla.org/mozilla-central/search?q=OffsetToAlignedStaticPos

Comment 7

[Daniel Holbert [:dholbert]]

See https://dxr.mozilla.org/mozilla-central/rev/085cdfb90903d4985f0de1dc7786522d9fb45596/layout/generic/nsAbsoluteContainingBlock.cpp#504-510 in particular where we strip off & ignore the ALIGN_FLAG_BITS.

Comment 8

[Mihir Iyer [:mihir]]

Attachment: Bug 1311892 - Implement <overflow-position> 'safe'/'unsafe' for absolutely positioned boxes in grid and flexbox.

Review commit: https://reviewboard.mozilla.org/r/258374/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/258374/

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 8993689 [details]
Bug 1311892 - Implement <overflow-position> 'safe'/'unsafe' for absolutely positioned boxes in grid and flexbox.

https://reviewboard.mozilla.org/r/258374/#review265476

This looks great - thanks!  Not setting r+ yet because I should review the tests when those are ready. :)

Just a few nits:

::: layout/generic/nsAbsoluteContainingBlock.cpp:505
(Diff revision 1)
> -  // setting AlignJustifyFlags::eOverflowSafe in |flags|.)  For now, we behave
> -  // as if "unsafe" was the specified value (which is basically equivalent to
> +  // For nothing specified, we currently implement 'unsafe' instead of the default
> +  // behavior which has some [at-risk] extra nuance about scroll containers)

Two nits:
 (1) Please add a period after "|flags|".

 (2) These terms are a little too vague in the phrasing right now:
 "for nothing specified" (specified for what?)
 "instead of the default behavior" (not clear if this is our default behavior or the spec's default or something else)

To clarify, let's reword the last 2 lines to something like the following:
===
Note: If no <overflow-position> is specified, we behave as 'unsafe'. This doesn't quite match the css-align spec, which has an [at-risk] "smart default" behavior with some extra nuance about scroll containers.
===

::: layout/generic/nsAbsoluteContainingBlock.cpp:507
(Diff revision 1)
> -  // the default behavior, when no value is specified -- though the default
> -  // behavior also has some [at-risk] extra nuance about scroll containers...)
> -  // For now we ignore & strip off <overflow-position> bits, until bug 1311892.
> +  flags |= ((alignConst & NS_STYLE_ALIGN_SAFE)
> +             ? AlignJustifyFlags::eOverflowSafe
> +             : AlignJustifyFlags::eNoFlags);

The possible "flags |= eNoFlags" flow seems a bit odd here, since that's really a no-op.

I think this would be bit better (more direct) as:
  if (alignConst & NS_STYLE_ALIGN_SAFE) {
    flags |= AlignJustifyFlags::eOverflowSafe;
  }

Comment 10

[Daniel Holbert [:dholbert]]

Comment on attachment 8993689 [details]
Bug 1311892 - Implement <overflow-position> 'safe'/'unsafe' for absolutely positioned boxes in grid and flexbox.

https://reviewboard.mozilla.org/r/258374/#review265476

> Two nits:
>  (1) Please add a period after "|flags|".
> 
>  (2) These terms are a little too vague in the phrasing right now:
>  "for nothing specified" (specified for what?)
>  "instead of the default behavior" (not clear if this is our default behavior or the spec's default or something else)
> 
> To clarify, let's reword the last 2 lines to something like the following:
> ===
> Note: If no <overflow-position> is specified, we behave as 'unsafe'. This doesn't quite match the css-align spec, which has an [at-risk] "smart default" behavior with some extra nuance about scroll containers.
> ===

sorry -- to clarify, nit #1 here (about adding a period after |flags|) was about this first line of the code-comment, which I missed in my code-highlighting:
> // If the safe bit in alignConst is set, set the safe flag in |flags|

Comment 11

[Mihir Iyer [:mihir]]

Comment on attachment 8993689 [details]
Bug 1311892 - Implement <overflow-position> 'safe'/'unsafe' for absolutely positioned boxes in grid and flexbox.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/258374/diff/1-2/

Comment 12

[Daniel Holbert [:dholbert]]

Comment on attachment 8993689 [details]
Bug 1311892 - Implement <overflow-position> 'safe'/'unsafe' for absolutely positioned boxes in grid and flexbox.

https://reviewboard.mozilla.org/r/258374/#review265788

nearly r+, just a few nits on the tests mostly:

::: layout/generic/nsAbsoluteContainingBlock.cpp:507
(Diff revisions 1 - 2)
>    uint16_t alignConst =
>      aPlaceholderContainer->CSSAlignmentForAbsPosChild(aKidReflowInput, pcAxis);
> -  // If the safe bit in alignConst is set, set the safe flag in |flags|
> -  // For nothing specified, we currently implement 'unsafe' instead of the default
> -  // behavior which has some [at-risk] extra nuance about scroll containers)
> -  flags |= ((alignConst & NS_STYLE_ALIGN_SAFE)
> +  // If the safe bit in alignConst is set, set the safe flag in |flags|.
> +  // Note: If no <overflow-position> is specified, we behave as 'unsafe'.
> +  // This doesn't quite match the css-align spec, which has an [at-risk]
> +  // "smart default" behavior with some extra nuance about scroll conrtainers.

typo: s/conrtainers/containers/

::: layout/reftests/w3c-css/submitted/align3/grid-abspos-staticpos-safe-align-justify-self-001.html:22
(Diff revision 2)
> +      border: 1px solid black;
> +      background: yellow;
> +      margin-bottom: 5px;
> +      margin-right: 5px;
> +      float: left; /* For testing in "rows" of containers */
> +      justify-items: center; /* To exercise 'justify-self: auto' on children */

This "justify-items: center" is unused (since you specify a different justify-self on every child here), so let's get rid of this line.  (Its code-comment is kinda confusing/misleading with the test as it stands right now.)

::: layout/reftests/w3c-css/submitted/align3/grid-abspos-staticpos-safe-align-justify-self-001.html:32
(Diff revision 2)
> +    .small > .container {
> +      grid: 3px 2px 3px / 0px 2px 0px;
> +      width: 2px;
> +      height: 4px;
> +      margin-right: 10px; /* To avoid overlap between overflowing kids */
> +    }

This rule is unused here, so let's remove it.

You probably should also get merge the ".big > .container" rule into the main ".container" rule -- there's no point in separating those styles in a separate .big rule, if all we've got is this one type of .container.  (And there doesn't seem to be any need for the <div class="big"> in this test, for the same reason.)

::: layout/reftests/w3c-css/submitted/align3/grid-abspos-staticpos-safe-align-justify-self-001.html:53
(Diff revision 2)
> +    <div class="container vertRL"><div style="justify-self: safe end; align-self: safe center"></div></div>
> +    <div class="container"><div style="justify-self: safe end; align-self: safe center"></div></div>

Two things:
 (1) Since you're setting the same align-self / jusfiy-self style on each of the children here, it'd be cleaner to move those styles up into the style block, in the .container > * rule.

 (2) Please add a section here where the grid has "position:relative" (which makes its *padding area* form the containing block for the absolute positioned child, rather than its grid cell).

For that section, the child will have to be a bit bigger in order to overflow & trigger the "safe" behavior, too.

You can do this with e.g.
 .relpos { position: relative }
 .relpos > * {
   height: [larger value];
   width: [larger value];
 }
...and then adding:
  <div class="container vertRL relpos">...</div>
  <div class="container vertRL relpos">...</div>


(You need to put the new CSS *after* the existing .container > * {...} rule, so that it'll "win" at setting the size of the children.)

::: layout/reftests/w3c-css/submitted/align3/reftest.list:53
(Diff revision 2)
> +# Test <overflow-position> 'safe' in flex containers
> +== flex-abspos-staticpos-safe-align-self-001.html flex-abspos-staticpos-safe-align-self-001-ref.html

Could you name these files as "align-self-safe" rather than "safe-align-self"?

We already have a pattern of "align-self-$FLAVOR" in this file, and it'd be cleanest to follow that convention here, for sanity / predictability / consistency's sake.

(Same goes for the grid files, too.  Those ones are "safe-align-justify-self" right now -- those should probably be "align-self-safe" [or align-justify-self-safe -- but I'd suggest just dropping "justify" from the filename for simplicity & to fit in better with the naming scheme of neighboring tests, even though they're testing more than just align-self.)

"hg mv $foo $bar" will let you rename these files in a way that hg is aware of (and then you can capture the update with e.g. "hg commit --amend", I think).

(Remember to update the filenames in the <link rel="match"> tags and in reftest.list, too.)

Comment 13

[Mihir Iyer [:mihir]]

Comment on attachment 8993689 [details]
Bug 1311892 - Implement <overflow-position> 'safe'/'unsafe' for absolutely positioned boxes in grid and flexbox.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/258374/diff/2-3/

Comment 14

[Mihir Iyer [:mihir]]

Comment on attachment 8993689 [details]
Bug 1311892 - Implement <overflow-position> 'safe'/'unsafe' for absolutely positioned boxes in grid and flexbox.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/258374/diff/3-4/

Comment 15

[Daniel Holbert [:dholbert]]

Comment on attachment 8993689 [details]
Bug 1311892 - Implement <overflow-position> 'safe'/'unsafe' for absolutely positioned boxes in grid and flexbox.

https://reviewboard.mozilla.org/r/258374/#review265906

Looks good! Thanks.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/57d01bdce6b8
Implement <overflow-position> 'safe'/'unsafe' for absolutely positioned boxes in grid and flexbox. r=dholbert

Comment 17

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/57d01bdce6b8

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

test merged in https://github.com/web-platform-tests/wpt/pull/12163