Closed Bug 1463599 Opened 6 years ago Closed 6 years ago

Make "contain:paint" trigger a stacking context

Categories

(Core :: Layout, enhancement, P3)

Product:
Core
Component:
Layout
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: dholbert, Assigned: yusuf)

References

Details

Attachments

(1 file, 1 obsolete file)

Bug 1463599 - Create a stacking context for contain:paint.
59 bytes, text/x-review-board-request
mattwoodrow
: review+
Details 
We've got a (simple) CSS "contain:paint" implementation from bug 1170781 (controlled by about:config pref layout.css.contain.enabled), and it seems to work based on me just viewing the following testcase in a profile where I've enabled that ^^ pref in about:config
  data:text/html,<div style="contain:paint; width: 20px">ShouldBeClipped

Our implementation is from the CSS Containment spec as it stood a few years ago, though, so it might not match what the spec requires now.

Let's be sure our existing implementation still works & matches the up-to-date spec.
Looks like we don't create a stacking context for contain:paint yet! nsIFrame::IsStackingContext should return true.
Priority: -- → P3
Comment on attachment 8980763 [details]
Bug 1463599 - Create a stacking context for contain:paint.

https://reviewboard.mozilla.org/r/246924/#review253058

This looks pretty good, and I verified that the test fails in Nightly right now (without the patch), which is great because that confirms that it's indeed indeed testing something new.  Hooray!

I'm being a bit extra-picky on the testcase since we'll be sharing it with other browser vendors.

Also, once you've addressed these comments, you should also request review from mattwoodrow rather than me (i.e. put r=mattwoodrow rather than r=dholbert in the commit message).  The code-change *seems* fine to me, but Matt's more likely to know if there might be anything else you're missing here RE stacking-context formation. (Though I think you're probably not missing anything.)

::: layout/generic/nsFrame.cpp:10991
(Diff revision 1)
>  nsIFrame::IsVisuallyAtomic(EffectSet* aEffectSet,
>                             const nsStyleDisplay* aStyleDisplay,
>                             const nsStyleEffects* aStyleEffects) {
>    return HasOpacity(aEffectSet) ||
>           IsTransformed(aStyleDisplay) ||
> +         aStyleDisplay->IsContainPaint() ||

This chunk doesn't apply cleanly on current mozilla-central -- in particular, it looks like the next line of code about "mChildPerspective" has changed to "ChildrenHavePerspective(aStyleDisplay)", which is near enough for mercurial to throw up its hands and say the patch doesn't apply.

Please update your tree & rebase this on the latest mozilla-central, so that it applies cleanly (or else autoland won't be able to land this, because it'll try to automatically rebase and it'll fail).

::: layout/reftests/w3c-css/submitted/contain/contain-paint-stacking-context-zindex-001.html:6
(Diff revision 1)
> +  <link rel="author" title="Yusuf Sermet" href="mailto:ysermet@mozilla.com">
> +  <link rel="author" title="Mozilla" href="http://www.mozilla.org/">

No need to explicitly list mozilla as a second author -- just yourself is fine.

Typically people list their own name (and feel free to use either your mozilla.com or personal email address, whichever you prefer).

::: layout/reftests/w3c-css/submitted/contain/contain-paint-stacking-context-zindex-001.html:9
(Diff revision 1)
> +  <link rel="help" href="https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_Positioning/Understanding_z_index/The_stacking_context">
> +  <link rel="help" href="https://drafts.csswg.org/css-contain/#containment-paint">

I'm pretty sure the <link rel="help"> tags are meant to point to spec text, and MDN is more of an informative tutorial / how-to rather than authoritative spec text.

So I'd suggest replacing that MDN url with https://drafts.csswg.org/css2/visuren.html#x43 (the definition of "stacking context" in the up-to-date CSS2.* spec, I think)

::: layout/reftests/w3c-css/submitted/contain/contain-paint-stacking-context-zindex-001.html:13
(Diff revision 1)
> +    * {
> +      margin: 0;
> +    }
> +    html {
> +      padding: 20px;
> +      font: 12px/20px Arial, sans-serif;
> +    }

I think none of this is necessary to the test, so let's get rid of it.

::: layout/reftests/w3c-css/submitted/contain/contain-paint-stacking-context-zindex-001.html:23
(Diff revision 1)
> +    h1 {
> +      font: inherit;
> +      font-weight: bold;
> +    }
> +    #div1,

This styling is unnecessary as well.  (It'd be worth it for you to do a quick skim and get rid of anything else that's obviously-unnecessary.  Anything related to sizing/stacking/coloring is probably-useful of course, since that's relevant to the rendering & testing/demonstrating the feature.)

::: layout/reftests/w3c-css/submitted/contain/contain-paint-stacking-context-zindex-001.html:29
(Diff revision 1)
> +      border: 1px dashed #696;
> +      padding: 10px;
> +      background-color: #cfc;

Dotted & dashed borders occasionally cause fuzzy "near-miss" reftest failures (if the end of a dash falls on a fractional pixel value for example, I've seen us occasionally render the edge of the dash to be imperceptibly different, but different enough that the reftest hardness calls it a failure).

So: in the interests of avoiding intermittent failures, it's probably better to replace all dashed borders with a solid border (or just remove the borders altogether, if they're not really needed).

::: layout/reftests/w3c-css/submitted/contain/contain-paint-stacking-context-zindex-001.html:81
(Diff revision 1)
> +    }
> +  </style>
> +</head>
> +<body>
> +  <div id="div1">
> +    <h1>Division Element #1</h1>

Let's get rid of the text, since it's not strictly necessary for this test (it's just a label), and it's one piece of the web platform that varies widely between operating systems so it's a source of unexpected reftest failures & surprises.

(This is particularly a concern for reftests that we're explicitly intending for other browser-implementors to run, like the tests in this "w3c-css/submitted" directory. If this were one of our own private one-off reftests or crashtests, I'd be somewhat less concerned)

::: layout/reftests/w3c-css/submitted/contain/contain-paint-stacking-context-zindex-001.html:103
(Diff revision 1)
> +
> +    <div id="div5">
> +      <h1>Division Element #5</h1>
> +      <br/><br/>
> +    </div>
> +     

there's whitespace on a blank line here -- please delete that. (Note that MozReview highlights that in red, and "hg diff" does too with the right configuration options, so you should make a habit of skimming your patches for that when submitting them for review.)
Attachment #8980763 - Flags: review?(dholbert) → review-
Comment on attachment 8980763 [details]
Bug 1463599 - Create a stacking context for contain:paint.

https://reviewboard.mozilla.org/r/246924/#review253058

> I'm pretty sure the <link rel="help"> tags are meant to point to spec text, and MDN is more of an informative tutorial / how-to rather than authoritative spec text.
> 
> So I'd suggest replacing that MDN url with https://drafts.csswg.org/css2/visuren.html#x43 (the definition of "stacking context" in the up-to-date CSS2.* spec, I think)

Note: If you'd like to link to MDN as a reference from the testcase here, you still can, though I'd suggest doing it in an HTML comment, like:
 <!-- Note: This testcase is based on [link] -->

I'm just uneasy about using a <link rel="help"> tag for this linking, because the W3C/CSSWG has automated scripts that process these submitted tests to link them up with the specs that they reference (e.g. to report metrics for how much test coverage they have for various spec sections), and I'll bet those scripts might barf if they see a link that they don't recognize in the <link> tag.
One other thing that we need here -- now that "contain" can crate a stacking context, I think we need to technically update the style system to tell it about that, in order for that "will-change: contain" to figure out automagically that it should create a stacking context when it's set (even in the absence of contain:paint).  Its spec text at https://www.w3.org/TR/css-will-change-1/ says will-change:<property> needs to create a stacking context if any value of <property> would create a stacking context.

Code-wise, I *think* you just need to update the "flags" line here:
https://searchfox.org/mozilla-central/rev/ce86c6c0472d5021ef693cf99abaaa0644c89e55/servo/components/style/properties/longhand/box.mako.rs#591-596

...to include CREATES_STACKING_CONTEXT in the flags list (kind of like we have for "position" elsewhere in that file, here:
https://dxr.mozilla.org/mozilla-central/source/servo/components/style/properties/longhand/box.mako.rs#55

I'm pretty sure that'll make it so nsIFrame::IsStackingContext() will return true via its check for "aStyleDisplay->mWillChangeBitField & NS_STYLE_WILL_CHANGE_STACKING_CONTEXT". 

You can test whether this works or not by creating a second copy of your testcase, with "will-change:contain" instead of "contain:paint", and it should behave the same way for stacking-context-forming purposes.

(You might want to name these testcases with a 1-letter suffix like "foo-001a.html" and "foo-001b.html", if you like, so that they can more clearly share "foo-001-ref.html" as their one-and-only reference case.)
Attachment #8980763 - Attachment is obsolete: true
Comment on attachment 8981344 [details]
Bug 1463599 - Create a stacking context for contain:paint.

https://reviewboard.mozilla.org/r/247484/#review253446
Attachment #8981344 - Flags: review?(matt.woodrow) → review+
dholbert, thanks a lot for all your comments and detailed explanation! I updated the code accordingly, and submitted the changes for mattwoodrow's review.
Thanks! It looks like this is reviewed & ready to land -- hooray! -- except that it's not fixing the full scope[1] of what this bug was originally filed about, which makes it a bit weird to land patches when we don't have a full fix for this bug as-filed.  (This is largely my fault for filing this bug with a broad "fix a bunch of stuff" scope.)

So how about this: let's narrow the scope of this bug to just cover the stacking context changes. [Doing this now.] Then, we can land the patch and consider this bug "fixed", and we can spin off separate bugs[1] (which should be marked as depending on bug 1170781 and blocking bug 1150081) for any remaining contain:paint-related things things that need updating.

[1] e.g. one other spec change that we need to fix: as you noticed on IRC, we need to mess a bit with the contain:paint-->overflow:clip interactions, so that contain directly triggers clipping rather than influencing the value of the "overflow" property.  Please file a separate bug for that, and for any other changes needed here.
Summary: Make sure our "contain:paint" implementation matches current spec text → Make "contain:paint" trigger a stacking context
Keywords: checkin-needed
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e5b504e6fdbb
Create a stacking context for contain:paint. r=mattwoodrow
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e5b504e6fdbb
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox62: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
This failed the lint script, so I can't import to web-platform-tests.  The errors are at:
https://travis-ci.org/web-platform-tests/wpt/jobs/386294848

The problem is the use of CRs as line endings.

(I'm not sure if the "No newline at end of file" is also a problem...it doesn't appear to be per the lint, although maybe it should be.)
Flags: needinfo?(ysermet)
yusuf is on Windows, and I'm not sure it's easy to insta-fix CRs on Windows.

It's trivial to fix them on linux, though, with the "fromdos" utility, so I'll just do that quickly & push the fix to mozilla-inbound as a followup for this bug.
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0983130f7fc
Fix endline characters in contain-paint-stacking-context-001* test files. (no review, test-only & whitespace-only)
(Hopefully that ^ inbound push fixes the issues from comment 12 -- fixed newline characters & added newline at end of each file.  -> canceling needinfo.)
Flags: needinfo?(ysermet)
Thanks dbaron and dholbert for the warning and fix. Yes, apparently for those files the line endings was set to Windows. I updated it to unix format in the editor I use, so hopefully, we wouldn't encounter this problem again.
Hello,

1-
Are there other "contain: paint" stacking context tests besides

https://dxr.mozilla.org/mozilla-central/source/layout/reftests/w3c-css/submitted/contain/contain-paint-stacking-context-001a.html

?

2-
Would it be possible to improve contain-paint-stacking-context-001a test? Maybe not now but eventually?

3-
I believe Firefox 63.0a1 buildID=20180724100052 fails

http://www.gtalbot.org/BrowserBugsSection/CSS3Contain/contain-paint-stacking-context-039.html

Daniel or Yusuf or Matt, can you look at it and confirm or explain?
(In reply to Gérard Talbot from comment #19)
> 1-
> Are there other "contain: paint" stacking context tests besides
> 
> https://dxr.mozilla.org/mozilla-central/source/layout/reftests/w3c-css/
> submitted/contain/contain-paint-stacking-context-001a.html

Looks like there's also an "-001b" version of that testcase.

> 2-
> Would it be possible to improve contain-paint-stacking-context-001a test?
> Maybe not now but eventually?

Anything's possible / patches accepted. :D  Yusuf's internship is over soon and he's heads-down on contain:style right now, so I'd rather not redirect him away from that at this point.

If there are specific things you'd like to improve about the test, perhaps file a bug and CC me and mark it as "depends on" this one?

> 3-I believe Firefox 63.0a1 buildID=20180724100052 fails
> http://www.gtalbot.org/BrowserBugsSection/CSS3Contain/contain-paint-stacking-
> context-039.html

I think it might be a bug in the test's expectations... Chrome "fails" that test too, and both Firefox and Chrome also "fail" that test if I replace "contain:paint" with "transform:rotate(20deg)", which also causes a stacking context to be created.
In particular, this part of the explanatory comment in contain-paint-stacking-context-039.html seems to be flawed/incorrect:
 "...since div#abs-pos-red-level-minus-1 is under, in the back of div#contain-paint-green..."

Really, the spec requires us to paint div#abs-pos-red-level-minus-1 on top here.

The paint order for stacking contexts is described at https://www.w3.org/TR/CSS2/zindex.html -- if we consider those steps as applied to the stacking context for div#contain-paint-green, we see the following:

 - Step 2.1 has us paint div#contain-paint-green's background color -- "background color of element"

 - Step 3 has us paint div#abs-pos-red-level-minus-1 (including its red background color) -- "stacking contexts formed by positioned descendants with negative z-indices"

So, the spec requires that red innermost descendant to be painted later on (i.e. in front) as compared to the green background on the contain:paint element.
Daniel,

Thank you for your complete answer; I appreciate :) . I think I now understand what went wrong with my understanding of the spec.

I will adjust tests consequently.

Gérard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: