1178895 - Implement CSS 'contain: layout' (2015 version)

Status: RESOLVED INCOMPLETE

(Core :: CSS Parsing and Computation, defect)

Description

[Kyle Zentner]

See http://dev.w3.org/csswg/css-containment/#containment-layout for details about this portion of the css 'contain' property.

Comment 1

[Kyle Zentner]

Attachment: ContainLayout

This implements 'contain: layout' by adding a new frame type, nsContainmentFrame, which sits above any element with 'contain: layout' set.

There are probably corner cases here which are not correct, so this should be considered a WIP.

Comment 2

[Kyle Zentner]

Attachment: ContainLayoutTest

This contains a few tests for 'contain: layout'. However, it only covers a few cases where elements collapse, so it should definitely be considered a WIP.

Comment 3

[Daniel Holbert [:dholbert]]

The first chunk of bug 1170781 tweaks nsBlockFrame::Init to add NS_BLOCK_FLOAT_MGR | NS_BLOCK_MARGIN_ROOT, because "contain:paint" converts us into a formatting context.  I think we need to extend that check to also cover "contain:layout" here. (since it makes us a formatting context, too)

Make sure you include a test here with a floated child, too; that might reveal this bug. (Though maybe the wrapper-frame also gets us around that somehow.)

Comment 4

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

Why do we want a new frame type?  Isn't this supposed to work on multiple types of frames (block, flex, grid)?

I'm inclined to think this should instead build on the work in bug 1159042 that I haven't yet finished.

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to David Baron [:dbaron] ⏰UTC-7 from comment #4)
> Why do we want a new frame type?  Isn't this supposed to work on multiple
> types of frames (block, flex, grid)?

It will. The new frame type is to abstract the special sizing requirements away from those container frames.

The spec says:
  # When laying out the containing element, it must be treated as having no contents.
  # After layout of the element is complete, its contents must then be laid out into
  # the containing element’s resolved size.

Rather than teach every container frame class about this new special requirement (and patching all of their GetPrefISize etc. methods), I thought it'd be simpler & more extensible to just add a new wrapper-frame (similar to the one we use for scroll frames), to abstract this sizing behavior away from the underlying actual flex/grid/block/table-container.

> I'm inclined to think this should instead build on the work in bug 1159042
> that I haven't yet finished.

It will benefit from the fact that reflow roots work reliably, since this new frame is intended to always be a reflow root.

Is there a way to leverage bug 1159042 to get the "pretend you don't have any children for layout purposes" behavior that's required by the spec?

Comment 6

[Daniel Holbert [:dholbert]]

(Maybe we could just get away with forcing ComputedHeight/ComputedWidth to be 0 in all content-dependent cases in nsHTMLReflowState... is that what you had in mind?  Still, we'll leak child-sizing info via ComputeSize / GetPrefISize / GetMinISize, which is why I figured strict isolation would be simplest...)

My main goal with suggesting that Kyle use a wrapper-frame like this was to avoid needing "contain:layout" sizing boilerplate in every container-frame class.

Do you have a cleaner strategy in mind that also achieves this goal? (or, maybe the boilerplate is minimal enough to be tolerable?)

Comment 7

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

Pretending there are no children to the outside is a bit of work, yes.

But the frame still needs to behave both to its children and to its parent as though it's a frame of the relevant type (block, flexbox, grid) for all the other layout rules, no?  How do you plan to emulate that?

Comment 8

[Daniel Holbert [:dholbert]]

The new frame (nsContainmentFrame) only has one child -- the "real" frame (the grid/table/flexbox/whatever).

So this DOM:
 <div>
   <div style="contain:layout; display:flex"> 
     <kid1></kid>
     <kid2></kid>

...would produce a frame tree like this:
 nsBlockFrame
   `- nsContainmentFrame
       `- nsFlexContainerFrame
            `- kid1
            `- kid2

Comment 9

[Daniel Holbert [:dholbert]]

(We do the same thing for "overflow:hidden", basically; the idea is for this to be a lighweight version of that.)

Comment 10

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

So what happens if its parent frame does things as a function of the type of its child, or its child as a function of the type of its parent?

How would such a frame's Reflow method produce the right results for both flex and block -- or are those results actually more similar than I think they would be at first glance?

Comment 11

[Daniel Holbert [:dholbert]]

(In reply to David Baron [:dbaron] ⏰UTC-7 from comment #10)
> So what happens if its parent frame does things as a function of the type of
> its child, or its child as a function of the type of its parent?

By "type", do you mean frame-type (i.e. frame->GetType())?  Do we do that much? Any such checks would need to be generified a bit, I suppose (and I'd expect they would already would need to be generified to work with "overflow:hidden" elements).

If you mean IsFrameOfType: we may want nsContainmentFrame to just return its sole child frame's IsFrameOfType() method. (That should handle cases where a parent cares about the child's IsFrameOfType() method. Not sure how to handle children caring about IsFrameOfType() on their parent, or if that even happens.

> How would such a frame's Reflow method produce the right results for both
> flex and block -- or are those results actually more similar than I think
> they would be at first glance?

You mean, if we have "contain:layout" on a flex vs. block element?  Or if the "contain:layout" is a child of a flex vs. block container?

Regardless, this may answer your question -- the way I'm envisioning things is:
 * The nsContainmentFrame's Reflow method should mostly just call ReflowChild on its sole child frame.
 * For this reflow, nsContainmentFrame should copy its own reflow state's ComputedWidth/ComputedHeight values into its sole child's reflow state.  (This is important if the "contain:layout" element is a flex item, because then, our parent-flex-container will have put our final size in our reflowState's ComputedWidth/ComputedHeight, using setters on the reflow state.)
 * At the end of nsContainmentFrame's reflow method, it should just ignore its sole child's desired size, when finalizing its own desired size -- it needs to behave as if it doesn't have any children, which means it should be able to just trust its reflow state's computed height/width. (and treat NS_AUTOHEIGHT as 0)

Comment 12

[Kyle Zentner]

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

Comment 13

[Daniel Holbert [:dholbert]]

Comment on attachment 8627986 [details] [diff] [review]
ContainLayout

Canceling review on this, since we're going to use a different strategy here, per discussion with dbaron last week.

New strategy: teach the various container-frame classes[1] to behave as if they have no children, for sizing purposes. (but still call their normal reflow code on their children)

[1] frame classes: nsBlockFrame, nsFlexContainerFrame, nsGridContainerFrame, and most or all of the table-part frames. For <button style="contain:layout">, we could perhaps get away with making 'contain' inherit into -moz-button-content (the anonymous wrapper block around a button's contents).

Comment 14

[Daniel Holbert [:dholbert]]

Comment on attachment 8627989 [details] [diff] [review]
ContainLayoutTest

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

Two high-level comments:
 (1) As with bug 1170781, these need <title>s and <link> elements to match the w3c testsuite format, and potentially comments to explain what they're testing if it's not clear from the title.

 (2) The testcases here need to be a bit more specific. In the w3c CSS testsuite, local filenames are expected to be globally unique (for easier reporting of results, or something).  So e.g. your testcase "layout-zero.html" should really be named something like "contain-layout-auto-height-001.html" or something like that.

::: layout/reftests/css-contain/layout-inline-zero-ref.html
@@ +6,5 @@
> +  body {
> +    margin: 0;
> +  }
> +  .container-ref {
> +    display: block;

This should be "inline-block", I think, per Tab's comments about how he intends for this to work.

@@ +7,5 @@
> +    margin: 0;
> +  }
> +  .container-ref {
> +    display: block;
> +    background: red;

Since this red isn't meant to be seen (since this is the reference case), you can probably just drop this background.

@@ +9,5 @@
> +  .container-ref {
> +    display: block;
> +    background: red;
> +    border: 1em solid green;
> +    width: auto;

Don't bother setting "width:auto", since it's the default. (Right now it looks like a difference between the reference case and the testcase, but really it's not.)

@@ +14,5 @@
> +    height: 0;
> +  }
> +  .inner-ref {
> +    position: absolute;
> +    left: 1em;

Do we really need absolute positioning in the reference case here? If I drop this rule entirely, the rendering seems to effectively stay the same (because we then become inset by the 1em border on the container).  So maybe we can just drop this CSS rule?

@@ +20,5 @@
> +  </style>
> +</head>
> +<body class="reftest-paint">
> +  <span class="container-ref"><span class="inner-ref">Test span.</span></span>
> +  <!--<div class="container-ref"><span class="inner-ref">Test span.</span></div>-->

This commented-out line probably wants to be removed, I think?

::: layout/reftests/css-contain/layout-zero-ref.html
@@ +1,3 @@
> +
> +<!DOCTYPE HTML>
> +<html>

Drop blank line at the start of this test.

Comment 15

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #14)
>  (2) The testcases here need to be a bit more specific. In the w3c CSS
> testsuite, local filenames are expected to be globally unique (for easier
> reporting of results, or something).  So e.g. your testcase
> "layout-zero.html" should really be named something like
> "contain-layout-auto-height-001.html" or something like that.

(er, "contain-layout-height-auto-001.html" is probably better.)

Also: if possible, these tests should avoid relying on text at all (or very much), to avoid implicitly depending on the viewer having particular fonts (or even sane fonts). Best for the tests to be robust against false-positives and/or false-negatives, in cases where people have bizarre/stupidly-sized/missing fonts.

(I'm slightly contradicting one of my suggestions from bug 1170781 comment 17 -- my suggestion there was to make sure "contain:paint" successfully clips painted content -- not just backgrounds/borders -- since we store backgrounds/borders/content in different display items IIRC. Here, that distinction isn't as significant, so we should be able to get away with just fixed-size colorful divs instead of text.)

Comment 16

[Kyle Zentner]

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

Comment 17

[Kyle Zentner]

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

Comment 18

[Kyle Zentner]

Attachment: ContainLayoutBase

This is the first patch of several which implement 'contain: layout' by modifying the reflow logic of each relevant frame type.

This patch implements the most basic functionality for 'contain: layout' needed for all frame types.

This version of the patch is being tested in the most recent try run above.

Comment 19

[Kyle Zentner]

Attachment: ContainLayoutBlock

This patch implements 'contain: layout' on blocks and inline blocks. Tests for this functionality will follow in the next patch.

Comment 20

[Kyle Zentner]

Attachment: ContainLayoutBlockTest

This patch contains the tests for 'contain: layout' on a block. It was also used in the try run above (although I changed the author email address since then).

Comment 21

[Kyle Zentner]

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

Comment 22

[Kyle Zentner]

Attachment: ContainLayoutBlockTest

This version of the test patch contains an additional test proposed by dholbert over IRC, as well as a fix for a test so that it actually tests the behavior of this change, instead of "trivially" passing.

Comment 23

[Kyle Zentner]

Attachment: ContainLayoutTableCell

This patch causes table cells to support 'contain: layout'. Visually, this behaves mostly equivalently to wrapping the contents of the table cell in a block which itself has 'contain: layout' (and 'overflow: visible') on it, as well as most of the size styling of the table cell. Of course, it is still a table cell, so the sizing properties act like they would on a table cell (min-height and max-height are ignored, for example).

Comment 24

[Kyle Zentner]

Attachment: ContainLayoutTableCell

Testing revealed that the previous patch failed to contain the line and table row baselines (which are different!). This version does prevent contents of 'contain: layout' table cells from affecting the baseline.

Comment 25

[Kyle Zentner]

Attachment: ContainLayoutTableCellTest

This patch adds tests for 'contain: layout' when applied to the cells of tables and inline-tables.

Comment 26

[Daniel Holbert [:dholbert]]

Comment on attachment 8640105 [details] [diff] [review]
ContainLayoutBase

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

r=me on part 1 with the following addressed:

::: layout/base/nsLayoutUtils.cpp
@@ +4127,5 @@
>             IsProperAncestorFrame(rootScrolledFrame, aFrame));
>  }
>  
> +bool
> +nsLayoutUtils::IsContainLayout(const nsIFrame* aFrame) {

Put the curly-brace on its own line here.

@@ +4135,5 @@
> +  if (aFrame->StyleContext()->GetPseudo() == nsCSSAnonBoxes::scrolledContent) {
> +    nsIFrame* parent = aFrame->GetParent();
> +    MOZ_ASSERT(parent,
> +               "If we're scrolled content, we should have a parent scrollframe");
> +    return parent->StyleDisplay()->mContain & NS_STYLE_CONTAIN_LAYOUT;

I think we should technically use the style-context tree for parentage here, instead of the frame tree.

So, let's drop "nsIFrame* parent", and instead use:

  const nsStyleContext parentStyle = aFrame->StyleContext()->mParent;
  [...assertion ...]
  return parentStyle->mContain & NS_STYLE_CONTAIN_LAYOUT;

(It probably won't make a difference, but if feels a bit more direct & correct-in-edge-cases to just directly grab the parent of the scrolledContent styleContext, rather than going via the frame-which-has-that-style-context to its parent to that parent's stylecontext.)

::: layout/base/nsLayoutUtils.h
@@ +1472,5 @@
>  
> +  /*
> +   * Because layout containment should be inherited from scroll frames (but not
> +   * the other portions of containment), this function is needed. Otherwise,
> +   * there would be a nsStyleDisplay::IsContainLayout().

This comment is a bit confusing. (In particular: nothing says layout containment "should" be inherited, and it's not actually inherited, and it's unclear from this comment why other portions of containment might not be.)

Suggested replacement, with a brief summary of the function followed by a bit more thorough explanation [feel free to tweak as you like]:

 /*
  * Returns true if aFrame should be treated as having "contain:layout".
  *
  * NOTE: In order for "contain:layout" to work on scroll framse, we need it
  * to effectively inherit through to the scrolled frame. However, we can't
  * _actually_ use CSS inheritance in ua.css to do this, or else other
  * 'contain' values (e.g. "paint") would inherit through to scrolled frames
  * as well, which would break things. So, instead of using real inheritance,
  * we use this helper-function, which has a special case for scrolled frames.
  * (If it weren't for this special case, this could live on nsStyleDisplay.)
  */

::: layout/style/nsRuleNode.cpp
@@ +5729,5 @@
>      }
>  
> +    // We can use mContain here directly, since in the case where
> +    // IsContainLayout() differs from using mContain directly, the frame is a
> +    // scroll frame, and thus already a formatting context.

It's a little unclear what this comment is justifying, unless the reader has contain:layout/scrollframe details paged into their head, which they probably will not.

So, let's clarify this a bit -- please start this off with something like the following:
  // Note: We test for 'contain:layout' here by *directly* checking mContain,
  // instead of using the more-correct & scrollframe-aware function
  // nsLayoutUtils::IsContainLayout(). This is OK because in the case where
  // mContain disagrees with the nsLayoutUtils function, [...]

::: layout/style/nsStyleStruct.h
@@ +2250,5 @@
>    }
>  
> +  /*
> +   * IsContainLayout is located in nsLayoutUtils.h, since determining whether
> +   * a frame is layout contained requires looking at the parent's style.

s/IsContainLayout/IsContainLayout()/

s/layout contained/layout-contained/

s/requires/may require/,  and add a parenthetical at the end "(for scrolled frames)" to make it clearer that this is just a special case and not a normal thing.

Comment 27

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #26)
> So, let's drop "nsIFrame* parent", and instead use:
> 
>   const nsStyleContext parentStyle = aFrame->StyleContext()->mParent;

Sorry, I forgot the "*" -- that should've been "const nsStyleContext* parentStyle"

Comment 28

[Daniel Holbert [:dholbert]]

Comment on attachment 8640108 [details] [diff] [review]
ContainLayoutBlock

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

[Setting feedback+ instead of review+ on this one, since it's short & my review feedback is changing enough that I might as well take another look at it once you've addressed review comments.]

::: layout/generic/nsBlockFrame.cpp
@@ +679,5 @@
> +  if (nsLayoutUtils::IsContainLayout(this)) {
> +    if (mMinWidth != NS_INTRINSIC_WIDTH_UNKNOWN) {
> +      return mMinWidth;
> +    } else {
> +      return 0;

Several things:

 (1) else-after-return is unnecessary. But, don't bother fixing that up, because:

 (2) It looks like we have the same "if (mMinWidth != NS_INTRINSIC_WIDTH_UNKNOWN) return mMinWidth;" check-and-return directly below this clause. So rather than duplicating that check/return, your new code here should just insert *after* that return, and you'll only need the following I think:

  if (nsLayoutUtils::IsContainLayout(this)) {
    return 0;
  }

@@ +772,5 @@
> +  if (nsLayoutUtils::IsContainLayout(this)) {
> +    if (mPrefWidth != NS_INTRINSIC_WIDTH_UNKNOWN) {
> +      return mPrefWidth;
> +    } else {
> +      return 0;

Above notes on mMinWidth apply here as well. (We just need an IsContainLayout(this) return 0, *after* the already-existing  NS_INTRINSIC_WIDTH_UNKNOWN check.)

@@ +1521,5 @@
> +    // If we don't have a specified size, and we're layout contained, we should
> +    // pretend our children don't exist (have zero block size) when calculating
> +    // our own size.
> +    blockEndEdgeOfChildren = 0;
> +  }

The next ~25 lines of code after this seem to be entirely about adjusting blockEndEdgeOfChildren -- adjustments which we don't care about if we're layout-contained.

So, I think you should change this closing curly-brace to an "} else {" and indent the next ~25 lines (down to after the BRS_FLOAT_MGR clause) to be inside of this new else clause.

@@ +1539,5 @@
>          std::min(blockEndEdgeOfChildren + aState.mPrevBEndMargin.get(),
>                 aState.mReflowState.AvailableBSize());
>      }
>    }
> +  if (aState.GetFlag(BRS_FLOAT_MGR) && !nsLayoutUtils::IsContainLayout(this)) {

Per my above comment about "else", you won't need to check !IsContainLayout() here, because we'll already be inside of an "else" clause which implies !IsContainLayout.

Comment 29

[Daniel Holbert [:dholbert]]

Comment on attachment 8640750 [details] [diff] [review]
ContainLayoutTableCell

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

Just one nit on this patch (below). Setting feedback+ for now, since it's not 100% clear what 'contain:layout' will mean for table-parts yet, per my post here:
 https://lists.w3.org/Archives/Public/www-style/2015Jul/0462.html

We probably shouldn't land a version of this patch until we've heard back on that; hence, holding off on r+ for now.

::: layout/tables/nsTableCellFrame.cpp
@@ +729,5 @@
>    nsIFrame *inner = mFrames.FirstChild();
>    nscoord borderPadding = GetUsedBorderAndPadding().top;
> +  if (nsLayoutUtils::IsContainLayout(this)) {
> +    return borderPadding;
> +  }

It's not immediately clear why "return borderPadding" is the right thing to do here.

I'd prefer that we refactor this function slightly so that the contain:layout behavior more naturally falls out of the existing logic -- e.g. something like the following (with a code-comment or two explaining the flow): 

  // Cell's baseline is just the baseline of the inner frame, shifted down by
  // the cell's top border/padding.
  nscoord innerBaseline;
  if (nsLayoutUtils::IsContainLayout(this)) {
    innerBaseline = 0;
  } else if (!nsLayoutUtils::GetFirstLineBaseline(GetWritingMode(), inner, &innerBaseline)) {
    innerBaseline = inner->GetContentRectRelativeToSelf().YMost();
  }

  return innerBaseline + GetUsedBorderAndPadding().top;;

I haven't tested the above, but I think something like that should work & be a bit more self-documenting than "return borderPadding".

Comment 30

[Kyle Zentner]

Attachment: ContainLayoutBase [r=dholbert]

I made the changes you described, although I had to use const_cast to access StyleDisplay() through the parent style context.

Comment 31

[Kyle Zentner]

Attachment: ContainLayoutBlock

This version addresses the concerns above, and still passes the containment tests.

Comment 32

[Kyle Zentner]

Attachment: ContainLayoutTableCell [may not want to land, pending discussion on www-style list]

This version addresses the minor feedback above. The behavior hasn't changed, so we probably shouldn't merge this yet. The tests still pass, of course.

Comment 33

[Kyle Zentner]

Attachment: ContainLayoutFlex

This patch implements containment for flex containers and flex items. The behavior here is relatively complicated, but I think it's specified thoroughly enough.

Comment 34

[Kyle Zentner]

Attachment: ContainLayoutFlexTest

This patch tests containment on flex containers and flex items. Some of the tested behavior is strange (e.g. 'align-items: baseline' in contain-layout-flex-003.html), but I think it's all correct.

Comment 35

[Kyle Zentner]

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

Comment 36

[Daniel Holbert [:dholbert]]

Comment on attachment 8641819 [details] [diff] [review]
ContainLayoutBlock

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

::: layout/generic/nsBlockFrame.cpp
@@ +1511,5 @@
> +      nsLayoutUtils::IsContainLayout(this)) {
> +    // If we don't have a specified size, and we're layout contained, we should
> +    // pretend our children don't exist (have zero block size) when calculating
> +    // our own size.
> +    blockEndEdgeOfChildren = 0;

So, I think you may be guarding the wrong thing here, actually. From IRC discussion and code-inspection, it looks like blockEndEdgeOfChildren still needs to be correct, even if we're "contain:layout", so that we can e.g. calculate the scrollable overflow correctly.

I think what you *really* want to be guarding is the code where we set of |finalSize| (particularly in cases where it depends on our children) -- not blockEndEdgeOfChildren.

Comment 37

[Daniel Holbert [:dholbert]]

Comment on attachment 8641820 [details] [diff] [review]
ContainLayoutTableCell [may not want to land, pending discussion on www-style list]

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

This seems generally fine, modulo comment nits below & spec uncertainty here.

(Probably treat the nits on this patch as being low-priority, since we may not end up taking this patch anyway, per the spec uncertainty about table-parts.)

::: layout/tables/nsTableCellFrame.cpp
@@ +732,5 @@
>    // since we want the position as though the inner were top-aligned.
> +  nscoord innerBaseline;
> +  if (nsLayoutUtils::IsContainLayout(this)) {
> +    // If we're layout-containing, we should pretend that our children's block
> +    // size is zero.

s/children's/inner frame's/

("children" is misleading, because a nsTableCellFrame shouldn't have "children". It should only ever have a single child-frame, its anonymous inner block-frame.)

@@ +736,5 @@
> +    // size is zero.
> +    innerBaseline = 0;
> +  } else if (!nsLayoutUtils::GetFirstLineBaseline(GetWritingMode(), inner, &innerBaseline)) {
> +    // Our inner frame didn't have a baseline, so use the bottom of out inner
> +    // frame's content rect.

s/baseline/first-line baseline/

s/out/our/

@@ +1011,5 @@
>    LogicalSize cellSize(wm);
> +  if (!nsLayoutUtils::IsContainLayout(this)) {
> +    cellSize.BSize(wm) = kidSize.BSize(wm);
> +    cellSize.ISize(wm) = kidSize.ISize(wm);
> +  }

I'm a bit unclear on where we actually establish a nonzero size for the cell, if it's fixed-size & layout contained. Here, it looks like we just leave this |cellSize| at 0,0, and pass that into aDesiredSize.

Maybe this happens via our row or table-layout-strategy?

If you know, it might be worth adding a comment here to clarify that, saying e.g.:
  // Otherwise, we'll just behave as if we have no content, and trust $FOO to update
  // that based on any specified size.
(just an example, for some $FOO; I'm not at all sure that this ^^ is correct)

Comment 38

[Daniel Holbert [:dholbert]]

Comment on attachment 8641821 [details] [diff] [review]
ContainLayoutFlex

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

r=me with the following addressed:

::: layout/generic/nsFlexContainerFrame.cpp
@@ +3259,5 @@
>   */
> +nscoord
> +nsFlexContainerFrame::ResolveFlexContainerMainSize(const nsHTMLReflowState& aReflowState,
> +                                                   const FlexboxAxisTracker& aAxisTracker,
> +                                                   nscoord aTentativeMainSize,

These lines end up too long (~90 characters) with the nsFlexContainerFrame:: prefix.

To fix that, please format like so:
nscoord
nsFlexContainerFrame::ResolveFlexContainerMainSize(
  const nsHTMLReflowState& aReflowState,
  const FlexboxAxisTracker& aAxisTracker,
[...]
  nsReflowStatus& aStatus)

@@ +3297,5 @@
>      if (largestLineOuterSize <= aAvailableBSizeForContent) {
>        return aAvailableBSizeForContent;
>      }
>      return std::min(aTentativeMainSize, largestLineOuterSize);
> +  } else if (nsLayoutUtils::IsContainLayout(this)) {

Drop the else-after-return, per coding style guide:
 https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#General_C.2FC.2B.2B_Practices

Just make this an "if" after the closing curly-brace.

@@ +3362,5 @@
>      if (aSumLineCrossSizes <= aAvailableBSizeForContent) {
>        return aAvailableBSizeForContent;
>      }
>      return std::min(effectiveComputedBSize, aSumLineCrossSizes);
> +  } else if (nsLayoutUtils::IsContainLayout(this)) {

As above, drop the else-after-return.

@@ +3909,5 @@
>    desiredSizeInFlexWM.BSize(flexWM) += containerBP.BStartEnd(flexWM);
>  
> +  if (flexContainerAscent == nscoord_MIN || nsLayoutUtils::IsContainLayout(this)) {
> +    // We either haven't found a baseline, or we should pretend that we don't
> +    // have any children (and thus that we don't haven't found a baseline.)

s/don't haven't/haven't/, I think

@@ +3912,5 @@
> +    // We either haven't found a baseline, or we should pretend that we don't
> +    // have any children (and thus that we don't haven't found a baseline.)
> +    if (flexContainerAscent == nscoord_MIN) {
> +      // Still don't have our baseline set -- this happens if we have no
> +      // children (or if our children are huge enough that they have nscoord_MIN

This inner nscoord_MIN check is only guarding a NS_WARN_IF_FALSE and a comment now, after your patch.  This is a bit silly -- just fold the "if" conditoin into the warning's condition (see below for how), so you can drop this if-check.

(This puts this "Still don't have our baseline set..." comment directly adjacent to your earlier added comment. Might need some minor wordsmithing to merge them sensibly.)

@@ +3916,5 @@
> +      // children (or if our children are huge enough that they have nscoord_MIN
> +      // as their baseline... in which case, we'll use the wrong baseline, but no
> +      // big deal)
> +      NS_WARN_IF_FALSE(lines.getFirst()->IsEmpty(),
> +                      "Have flex items but didn't get an ascent - that's odd "

So I think this warning should now be (with the if-condition folded in):

 NS_WARN_IF_FALSE(flexContainerAscent != nscoord_MIN ||
                  lines.getFirst()->IsEmpty(),
                  "Have flex items [...etc...]");

Comment 39

[Kyle Zentner]

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

Comment 40

[Kyle Zentner]

Attachment: ContainLayoutBlock

I think this version of ContainLayoutBlock is a little better. Let me know if you see a reasonable way to improve it.

Comment 41

[Kyle Zentner]

Attachment: ContainLayoutBlockTest

This version of the block test patch adds a reftest checking that scrollable overflow is still created by 'contain: layout' blocks.

Comment 42

[Kyle Zentner]

Attachment: ContainLayoutTableCellTest [based on assumption that table-cells can directly be layout-contained]

This version of the table cell test patch adds a test checking that scrollable overflow is still created for 'contain: layout' table cells.

I haven't revised the ContainLayoutTable patch yet, since I'm prioritizing Bug 1172087 over it.

Comment 43

[Kyle Zentner]

Attachment: ContainLayoutFlex [r=dholbert]

This version of the ContainLayoutFlex patch has the feedback above addressed. Let me know if I missed anything.

Comment 44

[Kyle Zentner]

Attachment: ContainLayoutFlexTest

This version of the ContainLayoutFlexTest patch adds a test that scrollable overflow is still created for (inline) flex containers.

Comment 45

[Daniel Holbert [:dholbert]]

Comment on attachment 8642454 [details] [diff] [review]
ContainLayoutBlock

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

(In reply to Kyle Zentner from comment #40)
> I think this version of ContainLayoutBlock is a little better. Let me know
> if you see a reasonable way to improve it.

This is almost there, but I think the ComputeFinalSize inserted-code could be improved a bit more. See below:

::: layout/generic/nsBlockFrame.cpp
@@ +1606,5 @@
>  
> +  if (nsLayoutUtils::IsContainLayout(this) && NS_UNCONSTRAINEDSIZE == aReflowState.ComputedBSize()) {
> +    // If we're layout-containing and we don't have a specified size, then our
> +    // final size should actually be computed from only our border and padding,
> +    // as though we were empty.

Two things:
 (1) This isn't quite correct, in a few cases:
   - If we have e.g. no specified height, but we have "min-height: 50px", we don't want to be 0px tall. We need to be 50px tall (plus borderpadding). Please add a testcase for that. I think we need the "ApplyMinMaxBSize" invocation that we use up higher. (but in this case, we'll be applying it to 0).

 (2) Some of the code above this (the "else" cascade where finalSize is set up) ends up being useless (since we stomp on its result here), which is kind of unfortunate.

I'd like to address both of those by inserting a new "else" clause into the if/else cascade where finalSize already gets set up in the old code.

Specifically, I'm talking about inserting the following "NEW CODE" into this cascade:

http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsBlockFrame.cpp?rev=80ef9bb2c2e9#1528
 [OLD CODE]
   if (NS_UNCONSTRAINEDSIZE != aReflowState.ComputedBSize()
       && (GetParent()->GetType() != nsGkAtoms::columnSetFrame ||
           aReflowState.parentReflowState->AvailableBSize() == NS_UNCONSTRAINEDSIZE)) {
   [...]
   }
 [BEGIN NEW CODE]
   else if (nsLayoutUtils::IsContainLayout(this)) {
     nscoord contentBSize = 0; // size as if we had no content
     nscoord autoBSize = aReflowState.ApplyMinMaxBSize(contentBSize);
     aMetrics.mCarriedOutBEndMargin.Zero();
   }
 [END NEW CODE; OLD CODE PICKS UP AGAIN HERE]
   else if (NS_FRAME_IS_COMPLETE(aState.mReflowStatus)) {
      ...

(Note that the first "if" check here -- about unconstrained frames & columnSetFrame -- is approximately asking "Do we have a specified size which can't be influenced by any column-frame balancing shenanigans", I think.)

I haven't tested the "NEW CODE" above, but I'm pretty sure it's approximately correct. (It's just a simplified version of the code below it, but ignoring our children for sizing.) (I'm not 100% sure we need the mCarriedOutBEndMargin zeroing -- we might already get that for free, if nobody else has touched that value.)

Comment 46

[Daniel Holbert [:dholbert]]

Comment on attachment 8642455 [details] [diff] [review]
ContainLayoutBlockTest

r=me with the following addressed (assuming the tests pass on Try of course)

>+++ b/layout/reftests/w3c-css/submitted/contain/contain-layout-block-002.html
>@@ -0,0 +1,24 @@
>+<!DOCTYPE HTML>
>+<html>
>+<head>
>+  <meta charset="utf-8">
>+  <title>CSS Test: 'contain: layout' on block element should cause the element
>+    to have zero height.</title>

s/on block element/on auto-sized block-element/

>+++ b/layout/reftests/w3c-css/submitted/contain/contain-layout-block-005.html
>+  <title>CSS Test: 'contain: layout' on a block element should allow
>+    scrollable overflow.</title>
[...]
>+  .clear-fix {
>+    clear: both;
>+  }
[...]
>+    <div class="container">
>+      <div class="contained"></div>
>+      <div class="clear-fix"></div>

It's not obvious to me why this testcase needs this ".clear-fix" element (which isn't present in the reference case).

Do we not generate scrollable overflow (inside of contain:layout) unless the floats are cleared? If that's true, that seems like a bug... (which we could file as a followup, and mark this test as "fails" with a link to that bug in reftest.list.


>diff --git a/layout/reftests/w3c-css/submitted/contain/contain-layout-empty-001-ref.html b/layout/reftests/w3c-css/submitted/contain/contain-layout-empty-001-ref.html
>--- a/layout/reftests/w3c-css/submitted/contain/contain-layout-empty-001-ref.html
>+++ b/layout/reftests/w3c-css/submitted/contain/contain-layout-empty-001-ref.html
>   </style>
> </head>
>-<body class="reftest-paint">
>+<body>
>   <div class="fun">

This patch-chunk doesn't apply for me -- looks like it's just tweaking a test file which doesn't exist on my system. (Maybe you added it in another patch? Or maybe you've mostly deleted it but not quite?)

In any case, you probably should just make this tweak in whatever patch it is that adds this test.

>+++ b/layout/reftests/w3c-css/submitted/contain/contain-layout-inline-block-001.html
[...]
>+  <title>CSS Test: 'contain: layout' on inline element should cause the element
>+    to become a inline-block with zero height and width.</title>

Nit: s/a/an/ (in "a inline-block")

>+++ b/layout/reftests/w3c-css/submitted/contain/contain-layout-inline-block-003.html
>+  <title>CSS Test: hidden contents of an 'contain: layout' inline-block should not
>+    cause the baseline to move.</title>

nit: s/an/a/

>+++ b/layout/reftests/w3c-css/submitted/contain/contain-layout-inline-block-004-ref.html
>+  .line-fix {
>+    position: relative;
>+    top: -43px;
>+  }
>+  </style>
>+</head>
>+<body>
>+  <div class="line-fix">
>+    <div class="container"></div>
>+    Word
>+    <div class="container" style="top:43px">Test text contents.</div>
[...]
>+    <div class="container" style="top:40px"><div class="block"></div></div>

Where do these 43px / 40px values come from? I don't immediately see what they correspond to in the testcase, and they feel a bit magical & perhaps exact-font-dependent... (which means this reftest will be flaky/invalid on different platforms).

If my magic-detector is wrong & these values are actually totally legit & not hardcoded to make things line up with a particular font, please add a comment explaining where they come from.

>+++ b/layout/reftests/w3c-css/submitted/contain/contain-layout-inline-block-005-ref.html
>+  .container {
>+    display: inline-block;
>+    position: relative;
[...]
>+    top: -25px;
>+  }
>+  .line-fix {
>+    position: relative;
>+    top: 25px;
>+  }

Same here -- these hardcoded 25px values aren't present in the testcase & seem magical/probably-font-dependent.

Comment 47

[Daniel Holbert [:dholbert]]

Comment on attachment 8642460 [details] [diff] [review]
ContainLayoutFlexTest

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

r=me with the following addressed:

::: layout/reftests/w3c-css/submitted/contain/contain-layout-flex-001-ref.html
@@ +8,5 @@
> +  body {
> +    margin: 0;
> +  }
> +  .container {
> +    contain: layout;

I think this "contain: layout" line wants to be removed from the reference case -- you probably just left it in accidentally. (And a reference case for a feature generally shouldn't use that feature, with a few exceptions.)

(Looks like this element also has "height:0" which is what you're *really* using to set it up as a reference rendering, so I don't think you're actually depending on "contain: layout" here.)

In addition to this reference case, you also have "contain:layout" in these others:
 contain-layout-flex-002-ref.html
 contain-layout-inline-flex-001-ref.html
 contain-layout-inline-flex-006-ref.html

Please remove it from all of those (if it is just cruft, as I think it is).

::: layout/reftests/w3c-css/submitted/contain/contain-layout-flex-001.html
@@ +2,5 @@
> +<html>
> +<head>
> +  <meta charset="utf-8">
> +  <title>CSS Test: 'contain: layout' on flexbox with no intrinsic
> +    height causes it to have zero height.</title>

Nit: s/causes/should cause/

(Right now, it's unclear whether "causes" is a declarative statement-of-what-should-happen, vs. a description of a buggy rendering which this test is a regression test for. Best to use language that makes it unambiguous.)

::: layout/reftests/w3c-css/submitted/contain/contain-layout-flex-002.html
@@ +2,5 @@
> +<html>
> +<head>
> +  <meta charset="utf-8">
> +  <title>CSS Test: 'contain: layout' on a flex item should prevent the
> +    content of the item from affecting other baselines.</title>

I'm not clear on what this testcase is actually testing. RE the title -- I don't think there's actually anything to "prevent" here -- the content of the flex item here *already* won't affect other baselines (and the item doesn't participate in baseline alignment), regardless of whether the item has contain:layout or not.  (There's only one baseline-aligned item in this test, and it's not the layout-contained item.)

Also, this test already passes in current Nightly.

(Maybe this is testing a bug that you introduced in an intermediate patch version & wanted to regression-test against, though? If so, then it's probably worth keeping, though the title probably needs some finessing since there aren't actually effects that need to be prevented, as noted above.)

::: layout/reftests/w3c-css/submitted/contain/contain-layout-flex-003-ref.html
@@ +22,5 @@
> +    height: 100px;
> +    margin: 2px;
> +  }
> +  .container-inner {
> +    position: absolute;

Add a comment like...
  /* To avoid participating in baseline alignment */
...here, to explain why we're using position:absolute.

::: layout/reftests/w3c-css/submitted/contain/contain-layout-flex-003.html
@@ +2,5 @@
> +<html>
> +<head>
> +  <meta charset="utf-8">
> +  <title>CSS Test: 'contain: layout' flex items should result in the
> +    item having no baseline.</title>

s/flex items/on a flex item/

@@ +30,5 @@
> +  </style>
> +</head>
> +<body>
> +  <div class="flex-box">
> +    <div class="flex-item sibling">

You don't have any ".sibling" CSS rules defined in this testcase, so I think you want to drop the "sibling" class here. (It's probably copypaste cruft from the previous test.)

::: layout/reftests/w3c-css/submitted/contain/contain-layout-inline-flex-001.html
@@ +1,5 @@
> +<!DOCTYPE HTML>
> +<html>
> +<head>
> +  <meta charset="utf-8">
> +  <title>CSS Test: 'contain: layout' on inline-flex should cause the element

s/inline-flex/auto-sized inline-flex/

::: layout/reftests/w3c-css/submitted/contain/contain-layout-inline-flex-002.html
@@ +61,5 @@
> +<body>
> +  <!--
> +    Since the contents of a contained element do not move the baseline, all
> +    of the following green boxes should be on the same line, and the same size.
> +    All of the words should be on the same line as each other as well.

Drop this last line of the comment, I think (about "All of the words should be on the same line...").

This isn't actually true if this test is viewed on e.g. a mobile device with a 400px-wide screen -- the words & divs will wrap to several lines. (I think the test still works just fine in that scenario; just this comment-statement won't hold up.)

::: layout/reftests/w3c-css/submitted/contain/contain-layout-inline-flex-003-ref.html
@@ +11,5 @@
> +  .container {
> +    display: inline-flex;
> +    width: 0;
> +    height: 0;
> +    vertical-align: top;

Add a comment like "To prevent contents from influencing baseline-alignment".  (I think that's what this is for?)

@@ +13,5 @@
> +    width: 0;
> +    height: 0;
> +    vertical-align: top;
> +    position: relative;
> +    top: 5px;

This "top: 5px" feels a bit magical... I suspect this is just an alignment hack that happens to work with this particular font? Or does this come from anywhere?

If it's actually legit, it needs a comment explaining where it comes from & why it's not just magic. (But even then it'd probably be best to avoid it if possible...)

Two possible ways to avoid it:
 (1) I noticed that you use "vertical-align:bottom" instead of "top" in the next reference case test -- maybe that would work here & remove the need for relative positioning? (I'm not 100% sure.)

...OR:
 (2) A slightly heavy-handed (but foolproof) way to avoid the need for magic here is:
 - In the testcase, style the inline-flex contents with "color: transparent" so that the text is invisible (but still participates in layout).
 - In the reference case, just have a bunch of empty display:inline-flex elements with height:0; width:0.
    This removes the need to set vertical-align / relative positioning / magic numbers entirely. (This strategy may help with getting rid of the magic numbers in comment 46, too.)

::: layout/reftests/w3c-css/submitted/contain/contain-layout-inline-flex-003.html
@@ +2,5 @@
> +<html>
> +<head>
> +  <meta charset="utf-8">
> +  <title>CSS Test: 'contain: layout' on an inline flexbox with visible
> +    contents should not move its line's baseline</title>

This title sort of says the opposite of what you want it to say.  (It says "contain:layout...should not move its baseline", but really it does move the baseline.)

maybe tweak  to say:
  <title>CSS Test: 'contain: layout' on an inline flexbox with visible
    contents should prevent contents from influencing baseline</title>

@@ +60,5 @@
> +<body>
> +  <!--
> +    Since the contents of a contained element do not move the baseline, all
> +    of the following green boxes should be on the same line, and the same size.
> +    All of the words should be on the same line as each other as well.

As with previous test, please drop this final line, since it's not really true if we end up needing to do line-wrapping. (which is entirely possible given that this line is a bit wide)

::: layout/reftests/w3c-css/submitted/contain/contain-layout-inline-flex-004.html
@@ +2,5 @@
> +<html>
> +<head>
> +  <meta charset="utf-8">
> +  <title>CSS Test: 'contain: layout' on an inline row flexbox
> +    should not affect its line's baseline</title>

This isn't true -- "contain:layout" absolutely affects the baseline (in the sense that the baseline changes when you add "contain:layout").

I think you mean to say that it prevents the *contents* from affecting the baseline.

So - tweak language to something like:
  <title>CSS Test: 'contain: layout' on an inline row flexbox
    should prevent contents from influencing baseline</title>

::: layout/reftests/w3c-css/submitted/contain/contain-layout-inline-flex-005.html
@@ +2,5 @@
> +<html>
> +<head>
> +  <meta charset="utf-8">
> +  <title>CSS Test: 'contain: layout' on an inline column flexbox
> +    should not affect its line's baseline</title>

(My above comment on contain-layout-inline-flex-004.html applies here too.)

Comment 48

[Daniel Holbert [:dholbert]]

FWIW: I filed bug 1191000 to cover some additional cases w/ flex items that we should test. (No worries if you don't get to it, though; it shouldn't block this bug from landing, and I or someone else can add those tests later on if you don't get to them.)

Comment 49

[Daniel Holbert [:dholbert]]

Comment on attachment 8642456 [details] [diff] [review]
ContainLayoutTableCellTest [based on assumption that table-cells can directly be layout-contained]

I'm canceling review on the table-cell tests, because from recent www-style discussion, it seems that table parts will likely just be promoted to "display:block":
  https://lists.w3.org/Archives/Public/www-style/2015Aug/0016.html

So given that, the right way to test "contain" on table cells (and "display:table-$FOO") will probably just be to test that it actually modifies the computed "display".

(It might also be worth testing that layout-contained stuff *inside of a table-cell* doesn't influence table / table-cell's sizing, and some of these tests might be able to be tweaked to test that. But for now, it probably wouldn't be correct to take these tests.)

Sorry that the spec is a bit unstable on this point. :-/  Thanks for taking the time to write these & the table-cell patch, in any case!

Comment 50

[Kyle Zentner]

Attachment: ContainLayoutBlock

This version of the ContainLayoutBlock patch uses the method suggested above for nsBlockFrame::ComputeFinalSize.

Comment 51

[Kyle Zentner]

Attachment: ContainLayoutBlockTest [r=dholbert]

This version of the ContainLayoutBlockTest patch uses the "heavy handed" method suggested above to test contained inline-block baseline behavior.

I think I also addressed all the other feedback.

Comment 52

[Kyle Zentner]

Attachment: ContainLayoutFlex [r=dholbert]

The only change in this version of the ContainLayoutFlex patch is that I've updated the patch number in the commit message.

Comment 53

[Kyle Zentner]

Attachment: ContainLayoutFlexTest [r=dholbert]

This version of the ContainLayoutFlexTest patch uses the same tricks as the ContainLayoutBlockTest to simplify testing contained baseline behavior.

I think I've also addressed all the other feedback.

Comment 54

[Kyle Zentner]

Comment on attachment 8643373 [details] [diff] [review]
ContainLayoutBlockTest [r=dholbert]

Changing this to r+ since dholbert said r=me with feedback addressed.

Comment 55

[Daniel Holbert [:dholbert]]

Comment on attachment 8643372 [details] [diff] [review]
ContainLayoutBlock

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

r=me, just one last nit:

::: layout/generic/nsBlockFrame.cpp
@@ +1568,5 @@
> +    // Hence this case is a simplified version of the case below.
> +    nscoord contentBSize = 0;
> +    nscoord autoBSize = aReflowState.ApplyMinMaxBSize(contentBSize);
> +    aMetrics.mCarriedOutBEndMargin.Zero();
> +    autoBSize += borderPadding.BStart(wm) + borderPadding.BEnd(wm);

I just noticed that the "bstart + bend" here should really just be condensed to:
 borderPadding.BStartEnd(wm)

Please make that tweak here, and also in the clause below this (so that this new code remains a simplified-but-similar version of the code below it, as mentioned in your comment).

Thanks!

Comment 56

[Daniel Holbert [:dholbert]]

(In reply to Kyle Zentner from comment #50)
> Created attachment 8643372 [details] [diff] [review]
> ContainLayoutBlock

This patch seems to be deleting the file "contain-layout-empty-001.html" now. (and that file doesn't exist in my tree).

I think you meant to just revert changes to this file...? (I don't know where it's being added, but if it's not sticking around, you should just remove it from the patch that actually adds it -- not in this patch.)

Comment 57

[Daniel Holbert [:dholbert]]

Comment on attachment 8643373 [details] [diff] [review]
ContainLayoutBlockTest [r=dholbert]

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

3 more nits on ContainLayoutBlockTest (along with the contain-layout-empty thing noted above):

::: layout/reftests/w3c-css/submitted/contain/contain-layout-block-004.html
@@ +31,5 @@
> +</head>
> +<body>
> +  <div class="container">
> +    <div class="contained"></div>
> +    <div class="clear-fix"></div>

I just noticed the clear-fix element is here, too -- please add the same clear-fix explanatory comment that you added to contain-layout-block-005.html. (thanks for that)

::: layout/reftests/w3c-css/submitted/contain/contain-layout-inline-block-004.html
@@ +1,5 @@
> +<!DOCTYPE HTML>
> +<html>
> +<head>
> +  <meta charset="utf-8">
> +  <title>CSS Test: invisible contents of an 'contain: layout' inline-block

s/an/a/

@@ +5,5 @@
> +  <title>CSS Test: invisible contents of an 'contain: layout' inline-block
> +    should not cause the baseline to move.</title>
> +  <link rel="author" title="Kyle Zentner" href="mailto:[e-mail]">
> +  <link rel="help" href="http://www.w3.org/TR/css-containment-1/#containment-layout">
> +  <link rel="match" href="contain-layout-inline-block-003-ref.html">

It's confusing for this test to be using -003-ref.html as its reference case.

Please rename the existing -003.html testcase to -003a.html, and rename this one to -003b.html, so that it doesn't have a different number from its reference case.

Comment 58

[Kyle Zentner]

Attachment: ContainLayoutBlock [r=dholbert]

I applied the feedback above, so this patch should be done.

Comment 59

[Kyle Zentner]

Attachment: ContainLayoutBlockTest [r=dholbert]

I renamed the tests as mentioned above, and made the other small title / comment modifications.

Comment 60

[Kyle Zentner]

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

Comment 61

[Daniel Holbert [:dholbert]]

(In reply to Kyle Zentner from comment #60)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=cc5a320496da

This Try run looks mostly-good -- most orange looks likely-unrelated to contain:layout, possibly from being based on top of a underlying cset that with issues.

However, the Mac & Android R / R1 / R2 failures are in tests added here,though: contain-layout-inline-flex-001.html, 004, 005, and 006.  Those may need the "make text visibility:hidden in the testcase and completely remove it in the reference case" treatment that we used on other tests for no-baseline-alignment-impact.

(At least in the case of contain-layout-inline-flex-001.html, it looks like the reference case is assuming that an inline-flex element with height:0px;width:0px;" will ignore its contents when determining its position, but that's not quite the case -- its contents influence its baseline, and if this gives it a fractional baseline, I think it might snap upwards or downwards a little. That's likely what's happening on Android for that test. Tests 4 & 5 use "vertical-align: bottom;" in the reference case -- I'll bet something similar is happening there, with a fractional baseline throwing off alignment somehow due to a rounding difference.)

We need to address this (probably with a 'visibility:hidden' hack as noted above) before landing the patches here. I'll plan on doing that [hence setting ni=me] since Kyle's internship is over and I don't want to eat into his vacation -- but Kyle, if you're up for making this tweak & you get to it before I do, feel free!

Comment 62

[Daniel Holbert [:dholbert]]

(Transferring comment 61 needinfo to Kyle, since I think he's working on getting this stuff landed)

Comment 63

[Kyle Zentner]

Attachment: ContainLayoutBase [r=dholbert]

This patch is essentially the same as the previous version, updated due to bitrot.

dholbert, do you want to review these again?

This and the following patches have been tested at https://treeherder.mozilla.org/#/jobs?repo=try&revision=136c39e6ff5d.

Comment 64

[Kyle Zentner]

Attachment: ContainLayoutBlock [r=dholbert]

Comment 65

[Kyle Zentner]

Attachment: ContainLayoutBlockTest [r=dholbert]

Comment 66

[Kyle Zentner]

Attachment: ContainLayoutFlex [r=dholbert]

Comment 67

[Kyle Zentner]

Attachment: ContainLayoutFlexTest [r=dholbert]

Comment 68

[Daniel Holbert [:dholbert]]

(In reply to Kyle Zentner from comment #63)
> dholbert, do you want to review these again?

No, with one exception -- I just noticed comment 30, about the "const_cast" -- I don't think you actually need to const_cast there. The StyleContext() accessor on a const nsIFrame returns a non-const nsStyleContext; and nsStyleContext's GetParent accessor also returns a non-const nsStyleContext. So there should be no reason you need a const_cast here.

Please drop that const_cast. r=me stands with that

(I noticed you updated the tests since comment 61, too; I didn't dig into the changes there. I'll trust that they're sane & hopefully address comment 61's test-failures.)

Comment 69

[Daniel Holbert [:dholbert]]

Comment on attachment 8685095 [details] [diff] [review]
ContainLayoutBase [r=dholbert]

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

::: layout/base/nsLayoutUtils.cpp
@@ +4911,5 @@
> +  if (aFrame->StyleContext()->GetPseudo() == nsCSSAnonBoxes::scrolledContent) {
> +    // We need a non-const nsStyleContext* so that we can call StyleDisplay()
> +    // on it.
> +    nsStyleContext* parentStyle =
> +      const_cast<nsStyleContext*>(aFrame->StyleContext()->GetParent());

To be extra-explicit, this ^^^ is the const_cast that has no reason to exist.  Drop it and the comment about it, which should just leave:
  nsStyleContext* parentStyle = aFrame->StyleContext()->GetParent();
which should compile just fine.

Comment 70

[Daniel Holbert [:dholbert]]

(Also: if your patches aren't in order on this bug [as they might not be, after you re-post to address comment 69], it'd be helpful for whoever lands them if you had "part 1", "part 2", etc. in the attachment-names. If you want to add that for any existing attachments here, you can do so by clicking the "details" link for the attachment, and then "edit details" (a link towards the top of that page), and then editing the "Description" field.

And as noted in bug 1170781 comment 53, ideally it's nice to tree-sheriffs if you have "r=dholbert" already in the commit message when you're prepping for checkin-needed, since that needs to be there before the patch is landed.  Though sheriffs will often be kind enough to look for that & add it for you.)

Comment 71

[Ben Kelly [:bkelly, not reviewing]]

Kyle, Daniel,

Do you know the status of this effort?  It seems like it stalled last year.  Chrome is shipping their impl in 52.

Since there is re-newed interest, it would be great if we could get this going again.

Thanks!

Comment 72

[Daniel Holbert [:dholbert]]

Comment 71 is probably best directed / thought of in terms of bug 1150081, the metabug for the "contain" property in general.  (It has various values, all of which probably need to be implemented before we could responsibly ship it, I think).  This bug here is only about one value for the property.

This bug here probably doesn't have too much work left -- it was close to ready to land (modulo some failures in the included tests).  But, parts of the spec have been rewritten since the patch-stack here was written, so this may need some rewriting.

More importantly, we need to finish off bug 1172087 (contain:style) before the broader feature would be able to ship, and that bug had some more-substantial review feedback / questions that need sorting out.

There is a layout/rendering-team prioritization meeting later today during the London all-hands, and hopefully we can get these bugs on someone's roadmap.

Comment 73

[Sebastian Zartner [:sebo]]

As Kyle was inactive for more than a year now, I think it's safe to unassign him. Would be great to see this reassigned to someone else soon.

Sebastian

Comment 74

[Daniel Holbert [:dholbert]]

"contain:layout" has been reworked in the spec (and the patches here likely don't apply anymore).  In the interests of sanity, I'm going to close this bug as "incomplete" and file a new bug for the updated implementation.  (The patches/discussion here will definitely inform the new bug, though!)