Closed Bug 1465250 Opened 6 years ago Closed 6 years ago

Make contain:paint trigger clipping independent of overflow

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: yusuf, Assigned: yusuf, Mentored)

References

Details

Attachments

(1 file)

Bug 1465250 - Make contain:paint trigger clipping independent of the overflow property.
59 bytes, text/x-review-board-request
mattwoodrow
: review+
Details 
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.
Blocks: css-contain-1
Depends on: css-contain-paint
Assignee: nobody → ysermet
Mentor: dholbert
Priority: -- → P3
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 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 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 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 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 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.
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 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.
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.
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+
(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.
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 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.
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
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.
(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.
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.
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/4bc90c7b3b4b
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox62: --- → fixed
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.)
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.

Attachment

General

Creator:
Created:
Updated:
Size: