Closed Bug 1470329 Opened 3 years ago Closed 3 years ago

Ensure "contain:size" makes elements monolithic

Categories

(Core :: Layout, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: dholbert, Assigned: morgan)

References

Details

Attachments

(1 file, 1 obsolete file)

I think in the contain:size work going on so far, we're focused on sizing/baseline information -- but we also need to make sure they are "monolithic", i.e. non-fragmentable.

https://drafts.csswg.org/css-contain/#containment-size
https://drafts.csswg.org/css-break-3/#monolithic

"Some content is not fragmentable, for example many types of replaced elements [CSS21] (such as images or video), scrollable elements, or a single line of text content. Such content is considered monolithic: it contains no possible break points. Any forced breaks within such boxes therefore cannot split the box, and must therefore also be ignored by the box’s own fragmentation context."

Testcase: https://jsfiddle.net/zo9masdk/

(Chrome doesn't seem to implement this requirement either -- I filed https://bugs.chromium.org/p/chromium/issues/detail?id=855263 on them)
I'm confused re: expected behaviour; should the fragmented element render as much as possible and have its "overflow" clipped? should it not render within the contained box at all?
Flags: needinfo?(dholbert)
I think it should overflow from the column and not be fragmented at all.  This is what Firefox does if you add e.g. "overflow:hidden" to the contained element in the attached testcase. (Chrome still splits it, because their fragmentation code is a bit special I guess.)

Basically, it should just ignore the fact that it's in a fragmented context.

Implementation-wise, one straightforward way to prevent fragmentation is to adjust the ReflowInput's own internal "AvailalbleBSize" calculation (during the ReflowInput setup) to be NS_UNCONSTRAINEDSIZE if mFrame is size-contained.

(This would be during the ReflowInput setup.)

And to test this, we can use multi-column as in the jsfiddle above, and we can use <html class="reftest-paged"> as well to emulate printing to tiny 3-inch-by-5-inch pages (e.g. with an element that has "height:10in; contain:size" which should simply overflow the printed page rather than fragmenting to additional pages).

(I don't think the upstream w3c test harness supports "reftest-paged", so we wouldn't want to put reftests with that feature into the "w3c-css/submitted" subfolder. Those would have to go in another [possibly-new] subfolder within layout/reftests.)
Flags: needinfo?(dholbert)
Oops, sorry - was in a hurry and copypaste failed -- in comment 3, I just repasted the same link as from comment 0.  Sorry! That must've been confusing. :)

I meant to paste *this* as a reference case: https://jsfiddle.net/n95b8fph/
(In Firefox and Edge, the overflow:hidden there makes the orange box monolithic, having the same effect as "contain:size" should have.)
(In reply to Daniel Holbert [:dholbert] from comment #2)
> Implementation-wise, one straightforward way to prevent fragmentation is to
> adjust the ReflowInput's own internal "AvailalbleBSize" calculation (during
> the ReflowInput setup) to be NS_UNCONSTRAINEDSIZE if mFrame is
> size-contained.

I suspect that code that handles forced breaks needs to be tweaked though.
(In reply to Mats Palmgren (:mats) from comment #5)
> (In reply to Daniel Holbert [:dholbert] from comment #2)
> > Implementation-wise, one straightforward way to prevent fragmentation is to
> > adjust the ReflowInput's own internal "AvailalbleBSize" calculation (during
> > the ReflowInput setup) to be NS_UNCONSTRAINEDSIZE if mFrame is
> > size-contained.
> 
> I suspect that code that handles forced breaks needs to be tweaked though.

Do you know where I should look for that?
Flags: needinfo?(mats)
Look for uses of nsStyleDisplay::mBreakBefore/mBreakAfter in layout.

For nsGridContainerFrame, it already checks for NS_UNCONSTRAINEDSIZE
and takes the non-fragmentation layout path, so you shouldn't need
to change anything there hopefully:
https://searchfox.org/mozilla-central/rev/93d2b9860b3d341258c7c5dcd4e278dea544432b/layout/generic/nsGridContainerFrame.cpp#4935

Hmm, now that I think about it, forced breaks in block layout is
handled by nsPageBreakFrame, which appears to do the right thing
for NS_UNCONSTRAINEDSIZE.  Flexbox layout doesn't implement
fragmentation yet so it's probably OK too.  Which leaves table
layout - I don't recall how it handles forced breaks exactly.
Flags: needinfo?(mats)
(In reply to Mats Palmgren (:mats) from comment #7)
> Which leaves table
> layout - I don't recall how it handles forced breaks exactly.

Fortunately contain:size has no effect on tables now: https://github.com/w3c/csswg-drafts/issues/2746

So maybe nothing needed there, and maybe the NS_UNCONSTRAINEDSIZE tweak will be sufficient...  Morgan's got a WIP patch that does that part already, I think.
How is this handled then?
<div style="contain:size"><table><tr><tr style="page-break-before:always">...
(In reply to Mats Palmgren (:mats) from comment #9)
> How is this handled then?
> <div style="contain:size"><table><tr><tr style="page-break-before:always">...

I added a reftest for this case and it passes with the current implementation.
Comment on attachment 8987176 [details]
Bug 1470329 - Change ReflowInput to have unconstrained BSize for size-contained elements, add reftests

https://reviewboard.mozilla.org/r/252416/#review258960

::: commit-message-ddf22:1
(Diff revision 2)
> +Bug 1470329 - Change ReflowInput to have unconstrained BSize for size-contained elements, add reftests r?dholbert

For future reference, you don't necessarily need to mention "add reftests" in commit messages; it's usually implied that tests are included. :)

(If you have a specific commit where all it does is add reftests, then of course you'd mention it there; but otherwise you don't need to mention it.)

::: layout/generic/ReflowInput.h:397
(Diff revision 2)
> +      // If we are size contained, we pretend to have
> +      // an unconstrained bSize to prevent fragmentation.
> +      if (mStyleDisplay->IsContainSize()) {
> +        return NS_UNCONSTRAINEDSIZE;
> +      }
> +      return mWritingMode.IsVertical() ? mAvailableWidth : mAvailableHeight;
> +    }

Rather than just pretending we have NS_UNCONSTRAINEDSIZE in the accessor, I think we should just make the value be the correct thing up-front -- probably here, after we make a similar adjustment for a different scenario:

https://dxr.mozilla.org/mozilla-central/rev/681eb7dfa324dd50403c382888929ea8b8b11b00/layout/generic/ReflowInput.cpp#473-486

After that ^^ linked clause, you probably want to add a new (separate) special-case like...
  if (mStyleDisplay->IsContainSize()) {
    AvailableBSize() = NS_UNCONSTRAINEDSIZE;
  }

I'm hoping that's all you need for that file?

Also: importantly, we want to be sure this new special-case clause does *not* get triggered for any of the spec's exempted boxes (tables, internal table frames, internal ruby boxes), for which "size containment has no effect".  

So we probably need to make nsStyleDisplay::IsContainSize() behavior a bit more subtle here.  For reference, see what we've got in nsStyleDisplay::IsContainPaint() (which yusuf tweaked recently) -- you probably want similar logic to that, plus an exemption for
  mozilla::StyleDisplay::InlineTable == mDisplay ||
  mozilla::StyleDisplay::Table == mDisplay 
...I think. (based on the spec language)

You *could* make that change in this patch, though it probably makes the most sense to do that over in your very first patch on bug 1467209, where you create the IsContainSize() function in the first place.

Also: you should probably test this exemption this by adding another "reftest-paged" reftest with a <table style="contain:size">, with a tall cell inside that table (or just lots of rows/cells), and then make the reference case be exactly the same except without "contain:size".  Hopefully, both files will show content that splits across multiple pages. This would effectively test that contain:size (correctly) does not turn tables into monolithic things.  (I suspect it might make them monolithic with your patch as it stands right now, though, since right now the patch doesn't have an exemption for tables / etc.)

::: layout/reftests/pagination/contain-size-break-001-ref.html:4
(Diff revision 2)
> +	<meta charset="utf-8">
> +	<title>CSS Reftest Reference</title>
> +	<link rel="author" title="Morgan Rae Reschenberg" href="mailto:mreschenberg@berkeley.edu">

Looks like these lines start with tab characters - please use spaces instead, since they render at a consistent width in every editor/viewer.

(If you can adjust your editor to make the 'tab' key insert spaces rather than tabs, that'd be worth doing, too.)

::: layout/reftests/pagination/contain-size-break-001-ref.html:9
(Diff revision 2)
> +div {
> +  color: transparent;
> +}

If possible, let's avoid the transparent-text hack in the reference case, and stick with the consistent & easy-to-reason about pattern of "testcase has transparent text inside of a contain:size element, vs. reference case has an element with zero contents."

::: layout/reftests/pagination/contain-size-break-001.html:4
(Diff revision 2)
> +  <title>CSS Test: 'contain: size' should ignore breaks and force elements to be monolithic.</title>
> +  <link rel="author" title="Morgan Rae Reschenberg" href="mailto:mreschenberg@berkeley.edu">
> +  <link rel="match" href="contain-size-break-001-ref.html">
> +<style>

Note: if you like, you don't need to bother with any of these <link> tags on the "reftest-paged" test & reference files.

The <link> tags are only for the benefit of non-Mozilla people & test-harnesses who consume our tests that we submit to the upstream shared repository. These reftest-paged tests likely won't be submitted upstream since they're using a mozilla-specific nonstandard testing feature.

::: layout/reftests/pagination/contain-size-break-001.html:37
(Diff revision 2)
> +    aaa
> +    <div class="contain basicOuter">
> +      <div class="innerPageBreak">CSS Test: 'contain:size' should ignore page-breaks and force elements to be monolithic.</div>
> +    </div>
> +    bbb

(I'm not sure this "aaa"/"bbb" text serves a purpose right now -- if it doesn't, let's remove it. Or if it does, let's make the purpose a little clearer)

::: layout/reftests/pagination/contain-size-break-001.html:38
(Diff revision 2)
> +    <div class="contain basicOuter">
> +      <div class="innerPageBreak">CSS Test: 'contain:size' should ignore page-breaks and force elements to be monolithic.</div>
> +    </div>
> +    bbb
> +    <br><br>
> +
> +    <div class="cols">
> +      <div class="contain innerObject">
> +        CSS Test: 'contain:size' should ignore wraping breaks and force elements to be monolithic.
> +      </div>
> +    </div>
> +    <br><br>
> +
> +    <div class="contain basicOuter">this is semi-inner before text
> +      <table style="background:lightblue;">
> +        <tr class="innerPageBreak">
> +          this is inner after text which may get cut off or overflow as the outer div becomes size contained
> +        </tr>
> +      </table>
> +    </div>

These should probably be split into three separate tests. 

In particular:
 - The multi-column part doesn't need to be in a "reftest-paged" test at all, so that part should be in a reftest in the shared w3c-css/submitted/contain/ directory.  (Other browsers should be able to run & benefit from that test.)

 - The reftest-paged parts (at the beginning and end of this test) are each too short to be effective right now. The elements that we're testing there need to be tall enough to have the option of triggering a pagebreak on their own. (And if they each get their own test, we can be more reliably certain that they're each getting a shot at spanning a page boundary & fragmenting or not.)  So those parts need to be split up into separate tests, and some piece of them -- probably the part that we're interested in being sure is monolithic vs. not-monolithic -- needs "height: 4in" (or larger) to be sure it's taller than the 3inch-tall-page.

::: layout/reftests/pagination/contain-size-break-001.html:46
(Diff revision 2)
> +    bbb
> +    <br><br>
> +
> +    <div class="cols">
> +      <div class="contain innerObject">
> +        CSS Test: 'contain:size' should ignore wraping breaks and force elements to be monolithic.

s/wraping/wrapping/

::: layout/reftests/pagination/reftest.list:79
(Diff revision 2)
>  # == table-caption-splitaftercaption-10.html blank.html # bug 672654
>  # == table-caption-splitaftercaption-11.html blank.html # bug 672654
>  == column-balancing-break-inside-avoid-2.html column-balancing-break-inside-avoid-2-ref.html
>  fuzzy-if(Android,1,2) == combobox-page-break-inside.html combobox-page-break-inside-ref.html
>  == table-nested-1308876-1.xhtml table-nested-1308876-1-ref.html
> +pref(layout.css.contain.enabled,true) == contain-size-break-001.html contain-size-break-001-ref.html

Use test-pref(...) here, since you only need the preference to be enabled for the testcase.

("pref(...)" enables it for both the testcase and the reference case.)
Attachment #8987176 - Flags: review?(dholbert) → review-
Priority: -- → P3
Comment on attachment 8987176 [details]
Bug 1470329 - Change ReflowInput to have unconstrained BSize for size-contained elements, add reftests

https://reviewboard.mozilla.org/r/252416/#review258960

> If possible, let's avoid the transparent-text hack in the reference case, and stick with the consistent & easy-to-reason about pattern of "testcase has transparent text inside of a contain:size element, vs. reference case has an element with zero contents."

I changed this, but these reftests won't work without first landing the patches on 1467209 because the "text should not affect the size of the container/element" functionality is implemented there ^ and assumed functional here.
Comment on attachment 8987176 [details]
Bug 1470329 - Change ReflowInput to have unconstrained BSize for size-contained elements, add reftests

https://reviewboard.mozilla.org/r/252416/#review258960

> Rather than just pretending we have NS_UNCONSTRAINEDSIZE in the accessor, I think we should just make the value be the correct thing up-front -- probably here, after we make a similar adjustment for a different scenario:
> 
> https://dxr.mozilla.org/mozilla-central/rev/681eb7dfa324dd50403c382888929ea8b8b11b00/layout/generic/ReflowInput.cpp#473-486
> 
> After that ^^ linked clause, you probably want to add a new (separate) special-case like...
>   if (mStyleDisplay->IsContainSize()) {
>     AvailableBSize() = NS_UNCONSTRAINEDSIZE;
>   }
> 
> I'm hoping that's all you need for that file?
> 
> Also: importantly, we want to be sure this new special-case clause does *not* get triggered for any of the spec's exempted boxes (tables, internal table frames, internal ruby boxes), for which "size containment has no effect".  
> 
> So we probably need to make nsStyleDisplay::IsContainSize() behavior a bit more subtle here.  For reference, see what we've got in nsStyleDisplay::IsContainPaint() (which yusuf tweaked recently) -- you probably want similar logic to that, plus an exemption for
>   mozilla::StyleDisplay::InlineTable == mDisplay ||
>   mozilla::StyleDisplay::Table == mDisplay 
> ...I think. (based on the spec language)
> 
> You *could* make that change in this patch, though it probably makes the most sense to do that over in your very first patch on bug 1467209, where you create the IsContainSize() function in the first place.
> 
> Also: you should probably test this exemption this by adding another "reftest-paged" reftest with a <table style="contain:size">, with a tall cell inside that table (or just lots of rows/cells), and then make the reference case be exactly the same except without "contain:size".  Hopefully, both files will show content that splits across multiple pages. This would effectively test that contain:size (correctly) does not turn tables into monolithic things.  (I suspect it might make them monolithic with your patch as it stands right now, though, since right now the patch doesn't have an exemption for tables / etc.)

The spec says "If the element’s principal box is a non-atomic inline-level box, size containment has no effect"; I've found bool methods for checking the other conditions (is table, is internal table element, is ruby container) mostly from [the methods Yusuf added](https://reviewboard.mozilla.org/r/251044/diff/2#index_header), but I'm wondering if I should use [IsDisplayTypeInlineOutside()](https://searchfox.org/mozilla-central/source/layout/style/nsStyleStruct.h#2358) to check this condition. Does that seem correct, or does it trigger things we shouldn't disable size-containment for (e.g. InlineGrid/Block, etc.)? The hyperlinks on the spec are a little unclear in what exactly a 'non-atomic, inline-level box' is, and I'm not sure where to look to find clarification.
Comment on attachment 8987176 [details]
Bug 1470329 - Change ReflowInput to have unconstrained BSize for size-contained elements, add reftests

https://reviewboard.mozilla.org/r/252416/#review258960

> The spec says "If the element’s principal box is a non-atomic inline-level box, size containment has no effect"; I've found bool methods for checking the other conditions (is table, is internal table element, is ruby container) mostly from [the methods Yusuf added](https://reviewboard.mozilla.org/r/251044/diff/2#index_header), but I'm wondering if I should use [IsDisplayTypeInlineOutside()](https://searchfox.org/mozilla-central/source/layout/style/nsStyleStruct.h#2358) to check this condition. Does that seem correct, or does it trigger things we shouldn't disable size-containment for (e.g. InlineGrid/Block, etc.)? The hyperlinks on the spec are a little unclear in what exactly a 'non-atomic, inline-level box' is, and I'm not sure where to look to find clarification.

You don't want to rely on IsDisplayTypeInlineOutside() here -- as you suspect, that method returns "true" for display:inline-block/grid/flex, which we want to honor containment on.

For us, the term "non-atomic inline-level box" term basically means nsInlineFrame (which is the box that gets generated for, e.g., <span> <b> and <i>, when they have "display:inline" styling [the default]).

I'd say you could cover your bases by including a comment in your IsContainSize() implementation, saying something like:
  // Note: the spec says size containment has no effect on
  // non-atomic inline-level boxes.  We don't check for those
  // here because we don't know what type of element is involved.
  // Callers are responsible for checking for whether the box in
  // question is non-atomic & inline-level, and creating an
  // exemption as-necessary.

And then, at your ReflowInput callsite where you're adjusting AvailableBSize, I don't think you need to add any special exemption (despite this ^^ comment), because I don't think nsInlineFrame cares about the Available BSize when it's reflowing itself.  (Its parent block is responsible for caring about the AvailableBSize that *it* receives and doing page-/column-breaking as-needed.)
Comment on attachment 8987176 [details]
Bug 1470329 - Change ReflowInput to have unconstrained BSize for size-contained elements, add reftests

https://reviewboard.mozilla.org/r/252416/#review259746

::: layout/generic/ReflowInput.cpp:488
(Diff revision 3)
> +  if (mStyleDisplay->IsContainSize()) {
> +    ComputedISize() = NS_UNCONSTRAINEDSIZE;
> +  }

I think this is setting the wrong thing -- this should be setting AvailableBSize(), not ComputedISize, right?

Also, this clause needs a brief explanatory code comment, saying e.g. "Size-contained boxes are monolithic, so we unset AvailableBSize() to avoid fragmenting them"

::: layout/reftests/pagination/contain-size-break-002-ref.html:2
(Diff revision 3)
> +<!DOCTYPE html>
> +<html">

nit: there's a stray quote character here.
Attachment #8987176 - Flags: review?(dholbert) → review-
Should I annotate these tests as fails == in this patch since they depend on the functionality in 1467209? If I pull both patches, they seem to all pass.
Flags: needinfo?(dholbert)
It depends on which order you intend to land them in.

(If you'd like to land these ones first and they'll be failing when they land, then do annotate them as "fails".)
Flags: needinfo?(dholbert)
(Typically when we land known-failing tests, we annotate them like so:
> fails == foo.html foo-ref.html # Bug NNN
...where Bug NNN is the bug that will fix them or which is tracking the fact that they're failing.)
Comment on attachment 8987176 [details]
Bug 1470329 - Change ReflowInput to have unconstrained BSize for size-contained elements, add reftests

https://reviewboard.mozilla.org/r/252416/#review260104

::: layout/generic/ReflowInput.cpp:490
(Diff revision 4)
> +    // that it is also monolithic. We do this by unsetting
> +    // ComputedBSize() to avoid fragmentaiton.
> +    ComputedBSize() = NS_UNCONSTRAINEDSIZE;

This still isn't quite right -- I think we should be resetting the AvailableBSize, not the ComputedBSize.

(AvailableBSize is our internal measurement of "the distance to the next fragmentation edge, e.g. page or column edge".  Whereas ComputedBSize is the resolved "height" value, if we've been able to resolve it -- e.g. if this element has height:100px, it'll be the nscoord representation of that. We definitely don't want to be resetting that one.  But I would expect that resetting the AvailableBSize to NS_UNCONSTRAINEDSIZE would prevent us from fragmenting.)
Attachment #8987176 - Flags: review?(dholbert) → review-
Comment on attachment 8987176 [details]
Bug 1470329 - Change ReflowInput to have unconstrained BSize for size-contained elements, add reftests

https://reviewboard.mozilla.org/r/252416/#review260554

::: layout/reftests/pagination/contain-size-break-001-ref.html:7
(Diff revision 5)
> +</head>
> +<style>
> +.basicOuter {
> +  height: 20px;
> +  background: orange;
> +}
> +</style>
> +  <body>

Please move the </head> close-tag down to be *after* the <style> block.

(This applies to reference cases for 001, 002, and 003 here.)

Otherwise this triggers HTML parse errors (visible as e.g. red markup in view-source) for stuff between the end of the head and the start of the body.  It probably still functions OK, but it's technically invalid. :)

::: layout/reftests/pagination/contain-size-break-001.html:2
(Diff revision 5)
> +<html class="reftest-paged">
> +<meta charset="utf-8">
> +  <title>CSS Test: 'contain: size' should ignore breaks and force elements to be monolithic.</title>
> +  <link rel="author" title="Morgan Rae Reschenberg" href="mailto:mreschenberg@berkeley.edu">
> +  <link rel="match" href="contain-size-break-001-ref.html">
> +<style>
> +div {
> +  color: transparent;
> +}
> +.contain {
> +  contain:size;
> +}
> +.innerPageBreak {
> +  page-break-before: always;
> +  page-break-after: always;
> +}
> +.basicOuter {
> +  height: 20px;
> +  background: orange;
> +}
> +</style>
> +  <body>

There should be a <head> wrapper around all this stuff between <html> and <body>.

(This applies to all 4 of the testcases here.)

::: layout/reftests/pagination/contain-size-break-001.html:8
(Diff revision 5)
> +div {
> +  color: transparent;
> +}

Let's remove this "color:transparent" rule -- it's unused, since there's no text.

::: layout/reftests/pagination/contain-size-break-001.html:25
(Diff revision 5)
> +  background: orange;
> +}
> +</style>
> +  <body>
> +    <div class="contain basicOuter">
> +      <div class="innerPageBreak"><!--CSS Test: 'contain:size' should ignore page-breaks and force elements to be monolithic.--></div>

This HTML comment just duplicates information from the title (with slight differences, e.g. "page-breaks" instead of "breaks").

To avoid that near-redundancy, let's just get rid of the comment.

(Alternately: if you'd like to put a comment here like:
<!-- This element would trigger a page-break, except that [etc] -->
...then that would be more useful.  No pressure though -- perhaps easiest to just remove it.)

::: layout/reftests/pagination/contain-size-break-002.html:3
(Diff revision 5)
> +<meta charset="utf-8">
> +  <title>CSS Test: 'contain: size' should ignore wrapping breaks and force elements to be monolithic.</title>

"wrapping breaks" is a bit vague here (and kinda sounds like it's talking about line-wrapping, but it's not).  Also, this should probably mention multi-column (multicol for short) by name in the title here.

How about:
CSS Test: 'contain: size' should force elements to be monolithic, i.e. to not fragment inside a multicol element.

Also: this testcase (the multicol one) is the one that we should probably move to the w3c-css/submitted/contain directory.

::: layout/reftests/pagination/contain-size-break-002.html:8
(Diff revision 5)
> +div {
> +  color: transparent;
> +}

Let's get rid of color:transparent here, since this test doesn't need to have any text (per below).

::: layout/reftests/pagination/contain-size-break-002.html:22
(Diff revision 5)
> +.innerObject {
> +  height: 5in;

Let's replace 5in with "200px" here.

(Tall enough to clearly get fragmented into multiple columns, within our 50px-tall multicol element, regardless of what the pixel-to-inch scale happens to be.)

(inch units make sense in reftest-paged testcases, but this isn't one of those.)

::: layout/reftests/pagination/contain-size-break-002.html:30
(Diff revision 5)
> +      <div class="contain innerObject">
> +        CSS Test: 'contain:size' should ignore wrapping breaks and force elements to be monolithic.
> +      </div>

Let's get rid of the text here -- it's not part of the core thing that you're testing here (whether or not the tall .innerObject gets fragmented into multiple columns).

innerObject has a specified height, so it doesn't need any content to push it over the column-height or anything like that.

::: layout/reftests/pagination/contain-size-break-003-ref.html:15
(Diff revision 5)
> +    <div class="basicOuter">
> +      <table style="background:lightblue; height:5in;">
> +        <tr>
> +        </tr>

Let's put a <td> inside of this <tr>, to be consistent with the testcase (so that the only difference is the containment).

(It actually looks like this is necessary for the testcase to pass, for me at least... Without this, I don't see any lightblue on my machine, because the table only paints its background via having a cell to paint something around/behind, I think.)

You might also consider giving the <table> a specified width (in the testcase & reference case), e.g. "width: 100px", so that it's easier to see.

(Otherwise it's a super skinny sliver of blue and is harder to visualize / reason about.)

::: layout/reftests/pagination/contain-size-break-003.html:8
(Diff revision 5)
> +div {
> +  color: transparent;
> +}

Remove the "color: transparent" here; it's unnecessary, since this testcase doesn't have (or need) any text.

::: layout/reftests/pagination/contain-size-break-003.html:28
(Diff revision 5)
> +  <body>
> +    <div class="contain basicOuter">
> +      <table style="background:lightblue;height:5in;">
> +        <tr class="innerPageBreak">
> +          <td>
> +          <!--CSS Test: 'contain: size' should ignore breaks and force elements to be monolithic.-->

As above, let's just get rid of this HTML comment (or replace it with something non-redundant that's more clearly documenting the HTML that it's alongside).
Attachment #8987176 - Flags: review?(dholbert) → review-
Comment on attachment 8987176 [details]
Bug 1470329 - Change ReflowInput to have unconstrained BSize for size-contained elements, add reftests

https://reviewboard.mozilla.org/r/252416/#review260738

::: layout/reftests/w3c-css/submitted/contain/contain-size-break-001.html:1
(Diff revision 6)
> +<!DOCTYPE html>
> +<html class="reftest-paged">

I think you might've moved the wrong test to the w3c-css/submitted directory, by mistake -- I think you meant to move the multicolumn test, right? 

(In this patch, we've got this reftest-paged test in the w3c-css/submitted directory, which doesn't make sense because reftest-paged is a mozilla-specific testing feature for now.)

I think the multicol test (the one that really belongs in the w3c-css/submitted directory) is called "contain-size-break-002.html" in this test -- though it perhaps should be renamed to contain-size-in-multicol-001.html so that its naming & numbering is independent of the other tests here.
Attachment #8987176 - Flags: review?(dholbert)
Comment on attachment 8987176 [details]
Bug 1470329 - Change ReflowInput to have unconstrained BSize for size-contained elements, add reftests

https://reviewboard.mozilla.org/r/252416/#review260742

::: layout/reftests/pagination/contain-size-break-002-ref.html:25
(Diff revision 6)
> +}
> +</style>
> +</head>
> +  <body>
> +    <div class="cols">
> +      <canvas class="innerObject">

Since this is the test that we'll probably be upstreaming, it'd be nice to include a brief HTML comment to explain why you're using a <canvas> tag here. (for the benefit of other browser-implementors trying to understand this test & what canvas has to do with containment)

E.g.:
    <!-- Note: using a <canvas> as a generic reference for something
         monolithic / non-fragmentable: -->

::: layout/reftests/pagination/contain-size-break-003-ref.html:6
(Diff revision 6)
> +<!DOCTYPE html>
> +<html class="reftest-paged">
> +<head>
> +  <meta charset="utf-8">
> +  <title>CSS Reftest Reference</title>
> +  <link rel="author" title="Morgan Rae Reschenberg" href="mailto:mreschenberg@berkeley.edu">

Feel free to remove these <link rel="match"> tags from these /pagination tests -- we only need these in the tests we're submitting to the w3c (since that's what the upstream w3c harness uses rather than reftest.list).

The tags are harmless & fine to leave in, too, as long as they're correct, but it might be simpler maintenance-wise to remove them (particularly given that there's still some test-shuffling-between-directories and possible renumbering to be done, per my previous comment -- and with that, it might be easier to just remove these tags rather than ensure they're updated).
Comment on attachment 8987176 [details]
Bug 1470329 - Change ReflowInput to have unconstrained BSize for size-contained elements, add reftests

https://reviewboard.mozilla.org/r/252416/#review260738

> I think you might've moved the wrong test to the w3c-css/submitted directory, by mistake -- I think you meant to move the multicolumn test, right? 
> 
> (In this patch, we've got this reftest-paged test in the w3c-css/submitted directory, which doesn't make sense because reftest-paged is a mozilla-specific testing feature for now.)
> 
> I think the multicol test (the one that really belongs in the w3c-css/submitted directory) is called "contain-size-break-002.html" in this test -- though it perhaps should be renamed to contain-size-in-multicol-001.html so that its naming & numbering is independent of the other tests here.

Ah sorry. I renamed it and then saved it in the wrong place overwriting the one I actually needed. I'll name it as above and fix it.
Comment on attachment 8987176 [details]
Bug 1470329 - Change ReflowInput to have unconstrained BSize for size-contained elements, add reftests

https://reviewboard.mozilla.org/r/252416/#review260742

> Feel free to remove these <link rel="match"> tags from these /pagination tests -- we only need these in the tests we're submitting to the w3c (since that's what the upstream w3c harness uses rather than reftest.list).
> 
> The tags are harmless & fine to leave in, too, as long as they're correct, but it might be simpler maintenance-wise to remove them (particularly given that there's still some test-shuffling-between-directories and possible renumbering to be done, per my previous comment -- and with that, it might be easier to just remove these tags rather than ensure they're updated).

Should I remove both the match and author tags, or just the match ones?
Comment on attachment 8987176 [details]
Bug 1470329 - Change ReflowInput to have unconstrained BSize for size-contained elements, add reftests

https://reviewboard.mozilla.org/r/252416/#review260742

> Should I remove both the match and author tags, or just the match ones?

Either way.  That's also a w3c-reftest-specific thing, and hypothetically if we submit these upstream someday (if w3c starts supporting some version of "reftest-paged"), we'd need to add the author tag back.

So it'd be fine to leave it -- it's harmless and has minimal maintenance requirements, as compared to <link rel="match">, since people change their names less frequently than test-files do. :)

But yeah, realistically, this is a mozilla-specific test for the time being, so it has no real need of the author tag.
Comment on attachment 8987176 [details]
Bug 1470329 - Change ReflowInput to have unconstrained BSize for size-contained elements, add reftests

https://reviewboard.mozilla.org/r/252416/#review260786

::: layout/reftests/pagination/contain-size-break-003.html:5
(Diff revision 6)
> +  <title>CSS Test: 'contain: size' should ignore breaks and force elements to be monolithic.</title>
> +  <link rel="author" title="Morgan Rae Reschenberg" href="mailto:mreschenberg@berkeley.edu">
> +  <link rel="match" href="contain-size-break-003-ref.html">
> +<style>
> +.contain {
> +  contain:size;
> +}
> +.innerPageBreak {
> +  page-break-before: always;
> +  page-break-after: always;
> +}
> +.basicOuter {
> +  height: 20px;
> +  background: orange;
> +}
> +</style>
> +</head>
> +  <body>
> +    <div class="contain basicOuter">
> +      <table style="background:lightblue;height:5in;">
> +        <tr class="innerPageBreak">
> +          <td></td>
> +        </tr>

I don't think this test is actually testing what it's intended to be testing right now.

I think it's *meant* to be testing that we don't honor page-break-before/page-break-after.  But it doesn't quite test that directly, because it's (a) tall enough that it triggers pagebreaks for other reasons, and (b) there's only one row right now so there's no content after this for "page-break-after" to deal with.  So right now, I think it's only failing in mozilla-central due to its dependence on the changes in the other containment bug, not the changes from this bug. (So it's not really testing this bug.)


I think I know how to fix it up, though -- let's fix this up like so (see suggested replacement content at the bottom, but I'm including an overview of changes here):

 (1) Use <table border> rather than just <table>. This is cosmetic but it makes it easier to visualize the parts of the table.

 (2) Give the table a smaller specified height so that page-break-* are the only things potentially triggering page-breaks (so that we're testing them more directly/thoroughly). And a specified width, while we're at it, so we can visualize it. 

 (3) Add a second identical tr and td (so that we've got a definite page-break opportunity for page-break-before/after to take action on).

So in particular, I think you should add this CSS to your <style> tag:

table {
  background: lightblue;
  height: 1in;
  width: 1in;
}

...and you should make the stuff inside the contained div look like this in the testcase:
      <table border>
        <tr class="innerPageBreak"><td></td></tr>
        <tr class="innerPageBreak"><td></td></tr>
      </table>

...vs. this in the reference case:
      <table border>
        <tr><td></td></tr>
        <tr><td></td></tr>
      </table>

With these changes, the testcase fails without this patch, in a way that makes sense -- reftest-analyzer shows that there's a page-break between the rows of the table -- and it passes with this patch (i.e. there's no pagebreak).
Comment on attachment 8987176 [details]
Bug 1470329 - Change ReflowInput to have unconstrained BSize for size-contained elements, add reftests

https://reviewboard.mozilla.org/r/252416/#review260790

::: layout/reftests/pagination/reftest.list:80
(Diff revision 6)
>  # == table-caption-splitaftercaption-11.html blank.html # bug 672654
>  == column-balancing-break-inside-avoid-2.html column-balancing-break-inside-avoid-2-ref.html
>  fuzzy-if(Android,1,2) == combobox-page-break-inside.html combobox-page-break-inside-ref.html
>  == table-nested-1308876-1.xhtml table-nested-1308876-1-ref.html
> +test-pref(layout.css.contain.enabled,true) == contain-size-break-002.html contain-size-break-002-ref.html
> +test-pref(layout.css.contain.enabled,true) fails == contain-size-break-003.html contain-size-break-003-ref.html # bug 1467209

Note: after my above comment is addressed to fix up this table-breaking test, then you shouldn't need to annotate this testcase as "fails" anymore.

So: ideally drop the "fails ... # bug 1467209" annotations, unless there's a clear reason that it still depends on that other bug (but it doesn't seem to, in my local testing, after the suggested changes are addressed).
(With these above feedback addressed, I think I'm at a spot where I understand & am happy with the tests here; so this is likely r+ with feedback addressed, modulo any nits that come up on the final changes.)
Attachment #8987176 - Attachment is obsolete: true
(Sorry, disregard comment 36; I accidentally hit "publish" rather than "discard" on a stale mozreview pending-review which I hadn't added any comments to.)
Comment on attachment 8989212 [details]
Bug 1470329 - Change ReflowInput to have unconstrained BSize for size-contained elements, add reftests

https://reviewboard.mozilla.org/r/254256/#review261380

r=me with feedback addressed:

::: layout/reftests/pagination/contain-size-break-001.html:15
(Diff revision 1)
> +    .innerPageBreak {
> +      page-break-before: always;
> +      page-break-after: always;
> +    }
> +    .basicOuter {
> +      height: 20px;

The "height: 20px" doesn't seem to be relevant here -- let's get rid of that in contain-size-break-001* and contain-size-break-002* here. 

(unless it's serving a purpose that I'm not understanding)

::: layout/reftests/pagination/contain-size-break-001.html:20
(Diff revision 1)
> +  <body>
> +    <div class="contain basicOuter">
> +      <div class="innerPageBreak">
> +      </div>

This isn't effectively testing this bug yet.  In particular, on my machine in a build without this bug's fix, this test passes (i.e. it renders on a single page, in print-preview and in the reftest harness).

To make this test effectively test the issue (and maximally trigger pagination, given its markup), you need to put some content before, inside, & after the "innerPageBreak" element.

I think you want the body to look like this:

    <div class="contain basicOuter">
      aaa
      <div class="innerPageBreak">
        bbb
      </div>
      ccc
    </div>

For me, in an unpatched build, this makes the testcase render on 3 separate pages in print-preview (with each string of letters on its own page).  So let's make that change, and then put this in the reference case:
    <div class="basicOuter">
      aaa
      <div>
        bbb
      </div>
      ccc
    </div>

::: layout/reftests/w3c-css/submitted/contain/contain-size-multicol-001.html:4
(Diff revision 1)
> +<meta charset="utf-8">
> +  <title>CSS Test: 'contain: size' should force elements to be monolithic, i.e. to not fragment inside a multicol element.</title>
> +  <link rel="author" title="Morgan Rae Reschenberg" href="mailto:mreschenberg@berkeley.edu">
> +  <link rel="match" href="contain-size-multicol-001-ref.html">
> +  <style>

This needs a <link rel="help"> tag, to ground it in the spec text.

This should work:
<link rel="help" href="https://drafts.csswg.org/css-contain/#containment-size">

(I think typically, we put this in between the "author" and "match" link tags.)
Attachment #8989212 - Flags: review?(dholbert) → review+
Comment on attachment 8989212 [details]
Bug 1470329 - Change ReflowInput to have unconstrained BSize for size-contained elements, add reftests

https://reviewboard.mozilla.org/r/254256/#review261380

> The "height: 20px" doesn't seem to be relevant here -- let's get rid of that in contain-size-break-001* and contain-size-break-002* here. 
> 
> (unless it's serving a purpose that I'm not understanding)

Oh, sorry -- I just realized that "height: 20px" is probably indeed needed, to make the orange area render at the same height, between the size-contained vs. non-size-contained case (after standard size-containment support lands for blocks etc. in the other bug)

So, never mind about that removal request. :)
Keywords: checkin-needed
I ran the tests with the pref on (all pass) and with the pref off (all fail). Seems like a good thing to do from now on re: making sure all tests are testing what they're supposed to :)
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 04ce40302fed1660f8006be656e5befc063e8a7c -d 8cd5cb7aebe2: rebasing 471031:04ce40302fed "Bug 1470329 - Change ReflowInput to have unconstrained BSize for size-contained elements, add reftests r=dholbert" (tip)
merging layout/reftests/pagination/reftest.list
merging layout/reftests/w3c-css/submitted/contain/reftest.list
warning: conflicts while merging layout/reftests/w3c-css/submitted/contain/reftest.list! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Flags: needinfo?(mreschenberg)
Keywords: checkin-needed
Flags: needinfo?(mreschenberg)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/3eed69f0be08
Change ReflowInput to have unconstrained BSize for size-contained elements, add reftests r=dholbert
Keywords: checkin-needed
(In reply to Morgan Reschenberg [:morgan] from comment #41)
> I ran the tests with the pref on (all pass) and with the pref off (all
> fail). Seems like a good thing to do from now on re: making sure all tests
> are testing what they're supposed to :)

Nice! That is indeed a good practice, to sanity-check that tests are behaving how they're supposed to. :)

(Even better, in general, is to simply run the tests without the patch's code-changes -- e.g. in this case, running reftests after doing "hg revert layout/generic/ReflowInput.cpp -r central; ./mach build binaries".  That makes sure the tests are indeed testing the specific change they're meaning to test, rather than accidentally testing some other piece of functionality that happens to also be guarded by the pref.

In this case, that's not a huge concern, since this is the first easily-visible piece of layout-related functionality to land -- so testing with the pref set & unset seems like it'd be sufficient here.)
Backed out changeset 3eed69f0be08 (bug 1470329) for web-platform failures on /css/css-contain/contain-size-breaks-001.

Backout: https://hg.mozilla.org/integration/autoland/rev/e7bd17818f9d872c42afb26ea8c9a4e5b48592db

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=3eed69f0be08c19fa9c62e4d55749501935ee6e4&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=success&selectedJob=186300754

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=186300754&repo=autoland&lineNumber=12744

[task 2018-07-03T22:26:26.642Z] 22:26:26     INFO - TEST-START | /css/css-contain/contain-size-breaks-001.html
[task 2018-07-03T22:26:26.643Z] 22:26:26     INFO - PID 5561 | 1530656786631	Marionette	DEBUG	[4294967297] flushRendering true
[task 2018-07-03T22:26:26.643Z] 22:26:26     INFO - PID 5561 | 1530656786636	Marionette	INFO	Found 108 pixels different, maximum difference per channel 255
[task 2018-07-03T22:26:26.644Z] 22:26:26     INFO - PID 5561 | 1530656786640	Marionette	INFO	Testing http://web-platform.test:8000/css/css-contain/contain-size-breaks-001.html == http://web-platform.test:8000/css/css-contain/reference/contain-size-breaks-001-ref.html
[task 2018-07-03T22:26:26.652Z] 22:26:26     INFO - PID 5561 | 1530656786645	Marionette	DEBUG	[4294967297] Waiting for page load of http://web-platform.test:8000/css/css-contain/reference/contain-size-breaks-001-ref.html
[task 2018-07-03T22:26:26.668Z] 22:26:26     INFO - PID 5561 | 1530656786658	Marionette	DEBUG	[4294967297] flushRendering true
[task 2018-07-03T22:26:26.669Z] 22:26:26     INFO - PID 5561 | 1530656786665	Marionette	DEBUG	[4294967297] Waiting for page load of http://web-platform.test:8000/css/css-contain/contain-size-breaks-001.html
[task 2018-07-03T22:26:26.686Z] 22:26:26     INFO - PID 5561 | 1530656786680	Marionette	DEBUG	[4294967297] flushRendering true
[task 2018-07-03T22:26:26.732Z] 22:26:26     INFO - TEST-UNEXPECTED-PASS | /css/css-contain/contain-size-breaks-001.html | Testing http://web-platform.test:8000/css/css-contain/contain-size-breaks-001.html == http://web-platform.test:8000/css/css-contain/reference/contain-size-breaks-001-ref.html
Flags: needinfo?(mreschenberg)
Looks like there's a web platform test that already tests this, similar to your multicol test! (and that web platform test now passes, hooray)

(That might mean the multicol test included here is semi-unnecessary, but probably no harm in testing the same thing in a few different ways.)

Anyway: I think you need to "hg rm" this file, as part of this bug's commit:
 https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/meta/css/css-contain/contain-size-breaks-001.html.ini

(you'll have to hit "reopen" at the top of this bug's mozreview page, too, in order for it to allow you to push an update)
Flags: needinfo?(mreschenberg)
Keywords: checkin-needed
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 0c4fbe28416d15ae88e224015f8227340700ded2 -d 3cb7eb89668a: rebasing 471410:0c4fbe28416d "Bug 1470329 - Change ReflowInput to have unconstrained BSize for size-contained elements, add reftests r=dholbert" (tip)
merging layout/generic/ReflowInput.cpp
merging layout/reftests/pagination/reftest.list
merging layout/reftests/w3c-css/submitted/contain/reftest.list
warning: conflicts while merging layout/reftests/w3c-css/submitted/contain/reftest.list! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Looks like this needs a rebase.
Flags: needinfo?(mreschenberg)
Keywords: checkin-needed
Flags: needinfo?(mreschenberg)
Keywords: checkin-needed
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d292eb650b0b
Change ReflowInput to have unconstrained BSize for size-contained elements, add reftests r=dholbert
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d292eb650b0b
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Depends on: 1679819
You need to log in before you can comment on or make changes to this bug.