Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Description • 8 years ago

The CSS clip and clip-path properties are not properly supported with APZ - the compositor doesn't move the clip properly during async scroll. Instead, it relies on main-thread repaints to update the clip to the right place. However, with paint-skipping, the observed behaviour got a lot worse because we don't paint nearly as often. We should disable paint-skipping when we run into clip or clip-path properties to avoid this problem. Bug 1214146 tracks fixing this more correctly, but this bug has a patch to disable paint-skipping for clip CSS properties.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 1 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b37d6b477782

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 2 • 8 years ago

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

	Markus Stange [:mstange]
	Comment 3 • 8 years ago

Comment on attachment 8768387 [details]
Bug 1284586 - Disable paint-skipping for scrollframes that we detect as having a CSS-clipped descendant.

https://reviewboard.mozilla.org/r/62656/#review60782

::: layout/base/DisplayListClipState.h:185
(Diff revision 1)
>     */
>    const DisplayItemScrollClip* mScrollClipContentDescendants;
>    const DisplayItemScrollClip* mScrollClipContainingBlockDescendants;
>  
>    /**
> +   * Magical property from Markus.

"The scroll clip that was in effect when mClipContentDescendants was set."

::: layout/generic/nsGfxScrollFrame.h:389
(Diff revision 1)
>    bool IsTransformingByAPZ() const {
>      return mTransformingByAPZ;
>    }
>    void SetScrollableByAPZ(bool aScrollable);
>    void SetZoomableByAPZ(bool aZoomable);
> +  void SetHasCSSClippedDescendant();

Let's call this "SetScrollsClipOnUnscrolledOutOfFlow".

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 4 • 8 years ago

Updated patch per review comments, did a new try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=377535c5888d&group_state=expanded which shows no obvious signs of bustage. I'll go ahead and land this.

	Pulsebot
	Comment 5 • 8 years ago

Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d26ac38f26d
Disable paint-skipping for scrollframes that we detect as having a CSS-clipped descendant. r=mstange

	Carsten Book [:Tomcat]
	Comment 6 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/1d26ac38f26d

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 7 • 8 years ago

Comment on attachment 8768387 [details]
Bug 1284586 - Disable paint-skipping for scrollframes that we detect as having a CSS-clipped descendant.

Approval Request Comment
[Feature/regressing bug #]: APZ in general, but bug 1253860 made it more noticeable
[User impact if declined]: CSS clips lag behind scrolling, which can result in temporarily mispainted content. It can be quite user-visible, since "temporarily" could be multiple seconds, and the mispaints can be large.
[Describe test coverage new/current, TreeHerder]: manually tested
[Risks and why]: Markus might be better able to answer this, since he wrote the hard parts of the patch. But fundamentally this patch just turns off paint-skipping on some pages, and so even in the worst case we just lose some performance that APZ bought us.
[String/UUID change made/needed]: none

	Gerry Chang [:gchang]
	Comment 8 • 8 years ago

Comment on attachment 8768387 [details]
Bug 1284586 - Disable paint-skipping for scrollframes that we detect as having a CSS-clipped descendant.

This patch fixes CSS clips issue while scrolling. Take it in 48 beta 9 and aurora.

	Gerry Chang [:gchang]
	Comment 9 • 8 years ago

Hi Kartikaya,
Can you help to provide some information to verify this? This needs to be verified in 48 beta 9.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 10 • 8 years ago

Please verify using the the reduced testcases in attachment 8673015 [details], attachment 8673082 [details], and the URL from bug 1214146 comment 0. In all of these cases the behaviour should be improved with this patch - although it still won't be perfect, it should be closer to what it looks like in other browsers.

	Carsten Book [:Tomcat]
	Comment 11 • 8 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-aurora/rev/1c72f075dd79

	Carsten Book [:Tomcat]
	Comment 12 • 8 years ago

  https://hg.mozilla.org/releases/mozilla-beta/rev/cf21aa15aad0

	Petruta Horea [:phorea], Desktop QA
	Comment 13 • 8 years ago

Environments: Win 10 64-bit, Ubuntu 14.04 64-bit and Mac OS X 10.9.5.
Builds: Firefox 48 beta 9, latest Aurora 49.0a2, latest Nightly 50.0a1 2016-07-20

The scrolling on the test pages improved a lot with Aurora and Nightly across platforms. On the other hand, 48 beta 9 still has the scrolling issues (see attachment) even though the patch was uplifted on beta. 

Kats, can I help in investigating further the reason for why this is happening?

Also, the scrolling (generally speaking) on Ubuntu seems a bit worse than on the other platforms. I haven't found anything logged on Bugzilla so I wonder if I should log it or it's caused by some platform limitations. 

Thanks!

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 14 • 8 years ago

I can reproduce what you're seeing using b9 on the two test pages. The full page at http://www.bbc.co.uk/news/resources/idt-248d9ac7-9784-4769-936a-8d3b435857a8 seems to behave fine with b9 though. I'll try to figure out what's going on, leaving needinfo on me for now.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 15 • 8 years ago

Markus and I looked at this a little bit. We're definitely taking a different codepath in beta than we are in Nightly, and it's something subtle that would take some time to track down fully. Even if we did track it down, it's unlikely that we could come up with a fix that would be safe to uplift this late in the 48 cycle.

Given that, and given that it's still fixed on most of the real-world pages we tested, we agreed that we'll just leave this as-is on 48. 49 and up will have the fuller fix anyway.

	Petruta Horea [:phorea], Desktop QA
	Comment 16 • 8 years ago

I'm setting the flag to affected on 48 so we know that the fix didn't worked here.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 17 • 8 years ago

Changing it to wontfix then, that feels more appropriate.