Closed Bug 1284586 Opened 3 years ago Closed 3 years ago

Disable paint-skipping for elements with the CSS clip property

Categories

(Core :: Panning and Zooming, defect, P1)

48 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla50
Tracking Status
firefox48 --- wontfix
firefox49 --- verified
firefox50 --- verified

People

(Reporter: kats, Assigned: kats)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(2 files)

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.
Assignee: nobody → bugmail
Priority: -- → P1
Whiteboard: [gfx-noted]
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".
Attachment #8768387 - Flags: review?(mstange) → review+
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.
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
https://hg.mozilla.org/mozilla-central/rev/1d26ac38f26d
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
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
Attachment #8768387 - Flags: approval-mozilla-beta?
Attachment #8768387 - Flags: approval-mozilla-aurora?
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.
Attachment #8768387 - Flags: approval-mozilla-beta?
Attachment #8768387 - Flags: approval-mozilla-beta+
Attachment #8768387 - Flags: approval-mozilla-aurora?
Attachment #8768387 - Flags: approval-mozilla-aurora+
Hi Kartikaya,
Can you help to provide some information to verify this? This needs to be verified in 48 beta 9.
Flags: qe-verify+
Flags: needinfo?(bugmail)
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.
Flags: needinfo?(bugmail)
Attached image 48beta scrolling.png
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!
Flags: needinfo?(bugmail)
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.
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.
Flags: needinfo?(bugmail)
I'm setting the flag to affected on 48 so we know that the fix didn't worked here.
Status: RESOLVED → VERIFIED
Changing it to wontfix then, that feels more appropriate.
You need to log in before you can comment on or make changes to this bug.