1343538 - Overpainting on Google docs

Status: NEW

(Core :: Web Painting, enhancement)

Description

[(no longer active)]

We seem to be overpainting in google docs quite a bit.  I'll add more useful info to the bug later...

Comment 1

[(no longer active)]

It seems like one way to reproduce the issue is to place the mouse on the side area of the doc, and then slowly move it horizontally over to the content area of the doc.  We typically paint the entire content area of the doc.  A similar issue happens when overlaying the Outline view on the left side.  Same with any comment boxes on the right side.

Sometimes a horizontal bar between the UI area of the docs and their content section gets repainted but it's hard to say what that corresponds to (perhaps the ruler at the top but the painted area is taller than the ruler.)

Overall the situation is pretty terrible.  Turning on paint flashing on GDocs should be enough to find tons of issues.  kats saw some of the issues on his machine too.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

This is probably an invalidation bug since when I was seeing it nothing appeared to be visibly changing on the screen but paintflashing was going crazy repainting a large area.

Comment 3

[(no longer active)]

Matt, any chance you could take a look at this, please?

Comment 4

[Matt Woodrow (:mattwoodrow)]

I can reproduce the issue with the Outline view consistently, and it looks like a layerization issue.

It looks like we have an opacity layer added around some content, and this is forced into an active layer since it has an animation/transition on it.

For some reason though, this is moving most of the page content into a new PaintedLayer on top of the opacity layer, and we have to repaint the old layer (to remove the popped content) as well as the new layer (to draw the popped content). The reverse happens again when the opacity is removed.

It's not obvious why this happening though, and there doesn't look like there's anything that both intersects the opacity, but is behind the rest of the page content.

When the opacity is removed our layer tree changes like this: https://pastebin.mozilla.org/8982137

The opacity ContainerLayer 0x130d2e000, along with 3 PaintedLayers on top of it all removed.

The display list difference between these two paints is: https://www.diffchecker.com/blr3kgnT

Most of the changes are just differences in visible regions (being either 0, or real), I think due to us not having called RecomputeVisibility on one of the lists.

The first interesting difference is line 896, where the opacity item goes away.

The first item after the opacity still made it in to the background PaintedLayer (0x12a971c00), but the next painted item (Border, line 910) is popped into a new layer.

The border doesn't intersect the opacity layer, and has the same AGR, ASR and clip chain as the transform, so I can't see why we decided it needed a new layer instead of using 0x12a971c00.

From there, most of the rest of the changes are items going in to the popped layers instead of the background.

Any ideas why this might happen Markus?

Comment 5

[Markus Stange [:mstange]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #4)
> Any ideas why this might happen Markus?

No, and I agree with you that it seems unexpected. We'll need to capture this in an rr recording and debug it properly.

Comment 6

[Jet Villegas (inactive)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #4)
> The border doesn't intersect the opacity layer, and has the same AGR, ASR
> and clip chain as the transform, so I can't see why we decided it needed a
> new layer instead of using 0x12a971c00.
> 
> From there, most of the rest of the changes are items going in to the popped
> layers instead of the background.

Adding a dependency on bug 1351811 so that we address this when we start retaining clips, clip chain, AGR and ASR information on the frame tree.

Comment 7

[(no longer active)]

Jet, you're assigned to the bug, but I think that means you're looking for someone else to work on it.  What's the current status of this bug?  This came up again in the light of bug 1271544 and bug 1271935 where I suspect we're possibly being hurt by this bug.  Do you mind giving an update please?

Comment 8

[Jet Villegas (inactive)]

(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #7)
> Jet, you're assigned to the bug, but I think that means you're looking for
> someone else to work on it.  

Actually, QF:P1 bugs assigned to me mean that we'll address the issue in other work. In this case, the work for bug 1351811 should improve things here. 

Matt: can you fire this up in your local build and see if it still happens? If so, we'll have to plug this another way.

Comment 9

[Matt Woodrow (:mattwoodrow)]

(In reply to Jet Villegas (:jet) from comment #8)
> (In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from
> comment #7)
> > Jet, you're assigned to the bug, but I think that means you're looking for
> > someone else to work on it.  
> 
> Actually, QF:P1 bugs assigned to me mean that we'll address the issue in
> other work. In this case, the work for bug 1351811 should improve things
> here. 
> 
> Matt: can you fire this up in your local build and see if it still happens?
> If so, we'll have to plug this another way.

It still does. Bug 1351811 helps us build display lists faster, but this is an issue where we're layerizing differently and needing a large repaint to update to that.

Comment 10

[Jet Villegas (inactive)]

Bug 1370575 sounds like another instance of this problem. I'll find a lucky winner with a rr setup to carve into this one as per comment 5.

Comment 11

[Markus Stange [:mstange]]

I'm expecting the patch in bug 1217748 to help with this. I'm currently looking into the test failures it caused.

Comment 12

[Mike Conley (:mconley) (:⚙️)]

Now that bug 1217748 is closed, we should re-test this.

Comment 13

[Bobby Holley (:bholley)]

(In reply to Mike Conley (:mconley) (:⚙️) (Catching up on needinfos / reviews) from comment #12)
> Now that bug 1217748 is closed, we should re-test this.

Looks like the patch got backed out. Markus, will you have time to get this relanded in the 64 timeframe?

Comment 14

[Markus Stange [:mstange]]

Unfortunately not. This bug isn't that important anyway; we're doing a bunch of unnecessary work but it doesn't slow down the interaction very much, as far as I'm aware, and with WebRender on the horizon it's less valuable to spend effort on invalidation bugs.