Make contain:paint trigger clipping independent of overflow

RESOLVED FIXED in Firefox 62

Status

()

enhancement
P3
normal
RESOLVED FIXED
11 months ago
10 months ago

People

(Reporter: yusuf, Assigned: yusuf, Mentored)

Tracking

(Blocks 1 bug)

unspecified
mozilla62
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

11 months ago
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.
(Assignee)

Updated

11 months ago
Depends on: css-contain-paint
(Assignee)

Updated

11 months ago
Assignee: nobody → ysermet
(Assignee)

Updated

11 months ago
Mentor: dholbert
Priority: -- → P3
Comment hidden (mozreview-request)
(Assignee)

Comment 2

11 months ago
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

11 months ago
mozreview-review
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 hidden (mozreview-request)
(Assignee)

Comment 5

11 months ago
mozreview-review-reply
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 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

11 months ago
mozreview-review-reply
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

11 months ago
mozreview-review
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

11 months ago
mozreview-review
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.)
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.
(Assignee)

Comment 11

11 months ago
mozreview-review-reply
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.
(Assignee)

Comment 12

11 months ago
mozreview-review-reply
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 hidden (mozreview-request)
(Assignee)

Comment 14

11 months ago
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.
(Assignee)

Updated

11 months ago
Blocks: 1465936
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.
Attachment #8982047 - Flags: review?(matt.woodrow) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 17

11 months ago
(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?
(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.
(Assignee)

Comment 19

11 months ago
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 hidden (mozreview-request)
(Assignee)

Comment 21

11 months ago
mozreview-review
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.
(Assignee)

Updated

11 months ago
Keywords: checkin-needed
(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

11 months ago
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9bdd12949711
Make contain:paint trigger clipping independent of the overflow property. r=mattwoodrow
Keywords: checkin-needed
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
Flags: needinfo?(ysermet)
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.
(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.)
...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 hidden (mozreview-request)
(Assignee)

Comment 29

11 months ago
(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.
Flags: needinfo?(ysermet)
(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 hidden (mozreview-request)
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.
(Assignee)

Updated

11 months ago
Keywords: checkin-needed

Comment 33

11 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/4bc90c7b3b4b
Make contain:paint trigger clipping independent of the overflow property. r=mattwoodrow
Keywords: checkin-needed

Comment 34

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4bc90c7b3b4b
Status: NEW → RESOLVED
Last Resolved: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
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.)
(Assignee)

Comment 36

10 months ago
dbaron, thank you for the advise! I associated my mozilla email with my github account.
You need to log in before you can comment on or make changes to this bug.