1465250 - Make contain:paint trigger clipping independent of overflow

Status: RESOLVED FIXED

(Core :: Layout, enhancement, P3)

Description

[Yusuf Sermet [:yusuf]]

Contain:paint currently triggers clipping by setting overflow as overflow: MozHiddenUnscrollable (i.e. clip). This needs to be updated so that:

- The clipping is directly triggered instead of changing the value of "overflow" property.
- It only clips to the limits of padding box.

Comment 1

[Yusuf Sermet [:yusuf]]

Attachment: Bug 1465250 - Make contain:paint trigger clipping independent of the overflow property.

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

Comment 2

[Yusuf Sermet [:yusuf]]

With the new patch, contain:paint does not mess with the value of the overflow property, and directly triggers clipping. It clips to the padding box by default, however, when contain:paint is used at the same time with overflow-clip-box: content-box, we are letting the more severe clipping (i.e. content box) apply as suggested by dholbert. A new reftest is also added to confirm this case.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8982047 [details]
Bug 1465250 - Make contain:paint trigger clipping independent of the overflow property.

https://reviewboard.mozilla.org/r/248090/#review254208

::: servo/components/style/style_adjuster.rs
(Diff revision 1)
>                  .mutate_box()
>                  .set_adjusted_display(Display::InlineBlock, false);
>          }
> -
> -        // When 'contain: paint', update overflow from 'visible' to 'clip'.
> -        if self.style

nit: drive-by, but can you update the comment above? Thank you for removing this!

Also (only if you know off-hand, no worries if not) dropping a spec link for the display change (or filing a bug about removing it) would be great :)

Comment 4

[Yusuf Sermet [:yusuf]]

Comment on attachment 8982047 [details]
Bug 1465250 - Make contain:paint trigger clipping independent of the overflow property.

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

Comment 5

[Yusuf Sermet [:yusuf]]

Comment on attachment 8982047 [details]
Bug 1465250 - Make contain:paint trigger clipping independent of the overflow property.

https://reviewboard.mozilla.org/r/248090/#review254208

> nit: drive-by, but can you update the comment above? Thank you for removing this!
> 
> Also (only if you know off-hand, no worries if not) dropping a spec link for the display change (or filing a bug about removing it) would be great :)

Thanks for your comments! I updated the above comment, but I am not really sure which spec link to put it there, sorry about that. I'll make sure to update it when I found out.

Comment 6

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8982047 [details]
Bug 1465250 - Make contain:paint trigger clipping independent of the overflow property.

https://reviewboard.mozilla.org/r/248090/#review254260

::: layout/generic/nsFrame.cpp:2505
(Diff revision 2)
>    }
>    nsRect clipRect;
>    bool haveRadii = false;
>    nscoord radii[8];
>    auto* disp = aFrame->StyleDisplay();
> -  if (disp->mOverflowClipBoxBlock == NS_STYLE_OVERFLOW_CLIP_BOX_PADDING_BOX &&
> +  bool hasOverflowClipBoxPaddingBox =

I see dholbert suggested this, but my reading of the spec is that we shouldn't.

"geometry must be clipped to the padding edge of the element’s principal box"

I also see "This is as if to overflow: visible was changed to overflow: clip at used value." which could potentially mean that we should take overflow-clip-box into account, but I think that's a non-normative note.

It does seem like the right/useful behaviour though, so should we file a spec bug to try get this clarified and matching what we want to do?

::: layout/generic/nsFrame.cpp:2512
(Diff revision 2)
> +	  disp->mOverflowClipBoxInline == NS_STYLE_OVERFLOW_CLIP_BOX_PADDING_BOX;
> +  bool hasOverflowClipBoxContentBox =
> +	  disp->mOverflowClipBoxBlock == NS_STYLE_OVERFLOW_CLIP_BOX_CONTENT_BOX &&
> +	  disp->mOverflowClipBoxInline == NS_STYLE_OVERFLOW_CLIP_BOX_CONTENT_BOX;
> +  if (hasOverflowClipBoxPaddingBox ||
> +     (!hasOverflowClipBoxContentBox && disp->IsContainPaint())) {

This logic appears to be:

If both axes are clipped to the padding box, or one of the axes are clipped to the padding box and we're contain:paint, clip to the padding box.

If both axes are clipped to the content box, or just one (and we're *not* contain:paint), then take the path that handles clipping per-axis.

Is that expected? Why does contain:paint make us clip to the padding box if the two axes are requesting different clipping?

It seems like the else branch handles basically everything, and we don't really need the if() branch at all. The exception is clipping, but we could just fix that.

::: servo/components/style/style_adjuster.rs:325
(Diff revision 2)
>          use properties::longhands::contain::SpecifiedValue;
>  
>          // An element with contain: paint needs to be a formatting context, and
> -        // also implies overflow: clip.
> +        // also implies overflow: clip, which is handled in nsFrame.cpp:ApplyOverflowClipping.
>          //
>          // TODO(emilio): This mimics Gecko, but spec links are missing!

I tried to look up the spec for this, and am struggling.

I see this "if the element’s principal box is a non-atomic inline-level box, paint containment has no effect."

Doesn't that mean that the correct behaviour is to ignore contain:paint, rather than change display:inline -> display:inline-block?

Comment 7

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8982047 [details]
Bug 1465250 - Make contain:paint trigger clipping independent of the overflow property.

https://reviewboard.mozilla.org/r/248090/#review254260

> This logic appears to be:
> 
> If both axes are clipped to the padding box, or one of the axes are clipped to the padding box and we're contain:paint, clip to the padding box.
> 
> If both axes are clipped to the content box, or just one (and we're *not* contain:paint), then take the path that handles clipping per-axis.
> 
> Is that expected? Why does contain:paint make us clip to the padding box if the two axes are requesting different clipping?
> 
> It seems like the else branch handles basically everything, and we don't really need the if() branch at all. The exception is clipping, but we could just fix that.

Sorry, the exception is rounded rectangle corner clipping.

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 8982047 [details]
Bug 1465250 - Make contain:paint trigger clipping independent of the overflow property.

https://reviewboard.mozilla.org/r/248090/#review254264

::: layout/generic/nsFrame.cpp:2505
(Diff revision 2)
>    }
>    nsRect clipRect;
>    bool haveRadii = false;
>    nscoord radii[8];
>    auto* disp = aFrame->StyleDisplay();
> -  if (disp->mOverflowClipBoxBlock == NS_STYLE_OVERFLOW_CLIP_BOX_PADDING_BOX &&
> +  bool hasOverflowClipBoxPaddingBox =

hmm, you're probably right.

The spec doesn't take "overflow-clip" into account because "overflow-clip" **is a non-standard thing, not exposed to web content** (which I'm just now realizing/remembering). And "overflow-clip" defaults to "padding-box", so that's what web-content will get by default (since it can't set it to anything else).

So I think we can do whatever makes sense internally here, and it probably does indeed make sense to respect "overflow-clip-box".

References:
https://developer.mozilla.org/en-US/docs/Mozilla/CSS/overflow-clip-box
https://dxr.mozilla.org/mozilla-central/rev/5866d6685849311f057e7e229b9ace63a2641c29/modules/libpref/init/all.js#2928-2929

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 8982047 [details]
Bug 1465250 - Make contain:paint trigger clipping independent of the overflow property.

https://reviewboard.mozilla.org/r/248090/#review254266

::: layout/reftests/w3c-css/submitted/contain/reftest.list:1
(Diff revision 2)
> -default-preferences pref(layout.css.contain.enabled,true)
> +default-preferences pref(layout.css.contain.enabled,true) pref(layout.css.overflow-clip-box.enabled,true)
>  
>  == contain-paint-clip-001.html contain-paint-clip-001-ref.html
>  == contain-paint-clip-002.html contain-paint-clip-002-ref.html
>  == contain-paint-clip-003.html contain-paint-clip-003-ref.html
>  == contain-paint-clip-004.html contain-paint-clip-004-ref.html
>  == contain-paint-clip-005.html contain-paint-clip-003-ref.html
> +== contain-paint-clip-006.html contain-paint-clip-006-ref.html

You probably only want to turn on the "overflow-clip-box" pref for this one specific test, rather than for all tests in this directory.

You can do that by adding this "pref(...)" annotation at the beginning of the line for this test.  (And if you happen to only need it for the testcase -- not the reference case -- then you could use "test-pref(...)" instead, to be even a bit more targeted.)

Comment 10

[Matt Woodrow (:mattwoodrow)]

Assuming we all agree on the comments then my suggested steps forward are:

* File a new bug for dealing with the display:inline -> display:inline-block change.

* Write a helper that shares code for nsIFrame::GetPaddingBoxBorderRadii and nsIFrame::GetContentBoxBorderRadii. Maybe nsIFrame::GetInsetBorderRadii(nscoord aRadii[8], nsMargin aInset)?

* Make the else branch in ApplyOverflowClipping handle rounded corners by adding a call to GetInsetBorderRadii(bp).

* Get rid of the if() branch entirely, since the else now hopefully handles all cases.

* Don't add any logic for contain:paint into ApplyOverflowClipping, since we want the same behaviour for both overflow:clip and contain:paint.

Comment 11

[Yusuf Sermet [:yusuf]]

Comment on attachment 8982047 [details]
Bug 1465250 - Make contain:paint trigger clipping independent of the overflow property.

https://reviewboard.mozilla.org/r/248090/#review254260

> I tried to look up the spec for this, and am struggling.
> 
> I see this "if the element’s principal box is a non-atomic inline-level box, paint containment has no effect."
> 
> Doesn't that mean that the correct behaviour is to ignore contain:paint, rather than change display:inline -> display:inline-block?

Filing a new bug for dealing with this.

Comment 12

[Yusuf Sermet [:yusuf]]

Comment on attachment 8982047 [details]
Bug 1465250 - Make contain:paint trigger clipping independent of the overflow property.

https://reviewboard.mozilla.org/r/248090/#review254266

> You probably only want to turn on the "overflow-clip-box" pref for this one specific test, rather than for all tests in this directory.
> 
> You can do that by adding this "pref(...)" annotation at the beginning of the line for this test.  (And if you happen to only need it for the testcase -- not the reference case -- then you could use "test-pref(...)" instead, to be even a bit more targeted.)

Thanks! Good to know. For this one, we need the pref for both the test and the ref.

Comment 13

[Yusuf Sermet [:yusuf]]

Comment on attachment 8982047 [details]
Bug 1465250 - Make contain:paint trigger clipping independent of the overflow property.

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

Comment 14

[Yusuf Sermet [:yusuf]]

Thank you all for the review and detailed steps. The code is now updated according to comment #10. I also updated one of the test cases (contain-paint-clip-002.html and its ref) to check the rounded corner rectangle clipping.

Comment 15

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8982047 [details]
Bug 1465250 - Make contain:paint trigger clipping independent of the overflow property.

https://reviewboard.mozilla.org/r/248090/#review254524

::: layout/generic/nsIFrame.h:1413
(Diff revision 3)
>                                Sides aSkipSides,
>                                nscoord aRadii[8]) const;
>    bool GetBorderRadii(nscoord aRadii[8]) const;
>    bool GetMarginBoxBorderRadii(nscoord aRadii[8]) const;
>    bool GetPaddingBoxBorderRadii(nscoord aRadii[8]) const;
>    bool GetContentBoxBorderRadii(nscoord aRadii[8]) const;

You could implement these 3 existing GetXXXXBoxBorderRadii function using the new helper to avoid duplicating code.

Comment 16

[Yusuf Sermet [:yusuf]]

Comment on attachment 8982047 [details]
Bug 1465250 - Make contain:paint trigger clipping independent of the overflow property.

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

Comment 17

[Yusuf Sermet [:yusuf]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #15)

I updated the code accordingly. Since GetMarginBoxBorderRadii uses OutsetBorderRadii (in contrast with InsetBorderRadii used in the other two), I updated the helper function's definition as GetBoxBorderRadii(nscoord aRadii[8], nsMargin aInset, bool aIsOutset) in which aIsOutset determines if we'll do 'InsetBorderRadii' or 'OutsetBorderRadii'

Also, just realized that the parameter aInset might need a better name. I'm thinking of aOffset?

Comment 18

[Matt Woodrow (:mattwoodrow)]

(In reply to Yusuf Sermet [:yusuf] from comment #17)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #15)
> 
> I updated the code accordingly. Since GetMarginBoxBorderRadii uses
> OutsetBorderRadii (in contrast with InsetBorderRadii used in the other two),
> I updated the helper function's definition as GetBoxBorderRadii(nscoord
> aRadii[8], nsMargin aInset, bool aIsOutset) in which aIsOutset determines if
> we'll do 'InsetBorderRadii' or 'OutsetBorderRadii'
> 
> Also, just realized that the parameter aInset might need a better name. I'm
> thinking of aOffset?

Looks great!

Yeah, aOffset or aMargin would work.

Comment 19

[Yusuf Sermet [:yusuf]]

As dholbert advised (https://bugzilla.mozilla.org/show_bug.cgi?id=1465936#c1), the display:inline -> display:inline-block change is no longer needed with the new spec. We also are not handling the clipping there anymore. So, I removed the whole function (servo/components/style/style_adjuster.rs:adjust_for_contain()).

Comment 20

[Yusuf Sermet [:yusuf]]

Comment on attachment 8982047 [details]
Bug 1465250 - Make contain:paint trigger clipping independent of the overflow property.

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

Comment 21

[Yusuf Sermet [:yusuf]]

Comment on attachment 8982047 [details]
Bug 1465250 - Make contain:paint trigger clipping independent of the overflow property.

https://reviewboard.mozilla.org/r/248090/#review254774

::: layout/reftests/w3c-css/submitted/contain/contain-paint-clip-002.html:1
(Diff revision 5)
>  <!DOCTYPE HTML>

On a different note, I tried to run this test case, that checks if contain:paint takes rounded corners into account when clipping, in chrome as well. It only clipped to the padding box without considering the rounder corners. Filed a bug to chrome. Just FYI.

Comment 22

[Daniel Holbert [:dholbert]]

(In reply to Yusuf Sermet [:yusuf] from comment #21)
> [Chrome] only clipped to the padding box without considering the rounder
> corners. Filed a bug to chrome. Just FYI.

For the record, this was https://bugs.chromium.org/p/chromium/issues/detail?id=848793 , and it seems this was fixed in Chrome 67 [current beta]. Hooray!

I'm going to go ahead and trigger autoland here -- looks like we've got a Try run[1] with a few test failures that I'm pretty confident are unrelated to the changes here. (In particular, the most persistent failure -- a Linux x64 QuantumRender reftest failure -- seems to be bug 1465273 comment 3, i.e. being unlucky enough to have this Try push based on top of a kinda-broken commit.)

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=c849fa8a386aec0f5ba194ae42ea615de3a97449

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9bdd12949711
Make contain:paint trigger clipping independent of the overflow property. r=mattwoodrow

Comment 24

[Raul Gurzau (:RaulG)]

Backed out changeset 9bdd12949711 (bug 1465250) for multiple failures on /css-contain/contain-paint-002.html on a CLOSED TREE

Backout link: https://hg.mozilla.org/integration/autoland/rev/b3035b04f08004699bb21b6501496dd8641061fe

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=9bdd129497115fe7254ccaae1b2ed58ed4dde0dc

Log link: https://treeherder.mozilla.org/logviewer.html#?job_id=182212667&repo=autoland&lineNumber=7123

Log snippet: 

[task 2018-06-07T03:26:58.885Z] 03:26:58     INFO - TEST-START | /css/css-contain/contain-paint-002.html
[task 2018-06-07T03:26:58.901Z] 03:26:58     INFO - PID 5382 | 1528342018886	Marionette	INFO	Testing http://web-platform.test:8000/css/css-contain/contain-paint-002.html == http://web-platform.test:8000/css/css-contain/reference/contain-size-001-ref.html
[task 2018-06-07T03:26:58.949Z] 03:26:58     INFO - TEST-UNEXPECTED-PASS | /css/css-contain/contain-paint-002.html | Testing http://web-platform.test:8000/css/css-contain/contain-paint-002.html == http://web-platform.test:8000/css/css-contain/reference/contain-size-001-ref.html

Comment 25

[Daniel Holbert [:dholbert]]

Ah, that is a test we didn't run on Try - I hadn't noticed that the Try run was reftest-only. (Typically for maximum safety, we should run reftests, web-platform-tests, mochitests, and crashtests on Try, to sanity-check how a change to a rendering feature impacts our test suites.)

Good news that it's an "unexpected pass", though! (i.e. a test that formerly failed that this changeset is fixing)

Looks like we should remove the current expected-fail annotation by deleting ("hg rm") this file:
 https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/meta/css/css-contain/contain-paint-002.html.ini
...as part of this bug's fix.

Comment 26

[Daniel Holbert [:dholbert]]

(Also: to allow yourself to post a new version of the patch, you'll need to use the "Reopen" (or "Reopen for Review"?) button/link on the MozReview page at https://reviewboard.mozilla.org/r/248088/ -- otherwise "hg push review" will fail because it thinks this bug's patch has already been landed.)

Comment 27

[Daniel Holbert [:dholbert]]

...and it looks like you need to update the expectations in layout/style/test/test_contain_formatting_context.html as well -- that test had test failures as shown in https://treeherder.mozilla.org/logviewer.html#?job_id=182211202&repo=autoland&lineNumber=3011 , since that test (or that piece of it) is testing for the old inline-->inline-block conversion.

Comment 28

[Yusuf Sermet [:yusuf]]

Comment on attachment 8982047 [details]
Bug 1465250 - Make contain:paint trigger clipping independent of the overflow property.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/248090/diff/5-6/

Comment 29

[Yusuf Sermet [:yusuf]]

(In reply to Daniel Holbert [:dholbert] from comment #25)
> Ah, that is a test we didn't run on Try - I hadn't noticed that the Try run
> was reftest-only. (Typically for maximum safety, we should run reftests,
> web-platform-tests, mochitests, and crashtests on Try, to sanity-check how a
> change to a rendering feature impacts our test suites.)

Got it, thanks!

> Looks like we should remove the current expected-fail annotation by deleting
> ("hg rm") this file:
>  https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/meta/
> css/css-contain/contain-paint-002.html.ini
> ...as part of this bug's fix.

Deleted.

(In reply to Daniel Holbert [:dholbert] from comment #27)
> ...and it looks like you need to update the expectations in
> layout/style/test/test_contain_formatting_context.html as well -- that test
> had test failures as shown in
> https://treeherder.mozilla.org/logviewer.
> html#?job_id=182211202&repo=autoland&lineNumber=3011 , since that test (or
> that piece of it) is testing for the old inline-->inline-block conversion.

I deleted this file rather than updating. Because it seems like its whole purpose is to check if the promotion of the display property is accurate and we are not changing something else (https://bugzilla.mozilla.org/show_bug.cgi?id=1170781#c31). Since, we no longer mess with the display value for contain:paint, I'm thinking this test is no longer strictly needed.

Comment 30

[Daniel Holbert [:dholbert]]

(In reply to Yusuf Sermet [:yusuf] from comment #29)
> I deleted this file rather than updating. Because it seems like its whole
> purpose is to check if the promotion of the display property is accurate and
> we are not changing something else

Sounds good. Thanks! Once you've rebased and gotten a try push with reftests/crashtests/mochitests/web-platform-tests, I think this will be good to re-land.

Comment 31

[Yusuf Sermet [:yusuf]]

Comment on attachment 8982047 [details]
Bug 1465250 - Make contain:paint trigger clipping independent of the overflow property.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/248090/diff/6-7/

Comment 32

[Daniel Holbert [:dholbert]]

For convenience -- the diff between the initially-landed patch and the updated one is:
  https://reviewboard.mozilla.org/r/248090/diff/5-7/

That interdiff makes sense to me, so I think you're good to carry forward the existing r+ and just add checkin-needed (or directly request landing from me or somebody) if the new try run ends up OK.

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4bc90c7b3b4b
Make contain:paint trigger clipping independent of the overflow property. r=mattwoodrow

Comment 34

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/4bc90c7b3b4b

Comment 35

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

web-platform-tests import is at https://github.com/web-platform-tests/wpt/pull/11450 .

(Also, Yusuf, if you have a github account, you might want to associate your @mozilla.com account with it.)

Comment 36

[Yusuf Sermet [:yusuf]]

dbaron, thank you for the advise! I associated my mozilla email with my github account.