Bug 64010 – In the preview theme of Gmail, yellow ad bar at the bottom won't repaint when scrolled

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Ryosuke Niwa]

Reproduction steps:
1. Goto gmail.
2. Choose "Review (Dense)" theme.
3. Read emails and spend a couple of minutes

When you scroll, yellow bar that shows ads won't move properly and the content below the bar won't be repainted.  The bar and the content below are rendered properly when I try to select text in the bar or resize the window.  Also, this bug seems to stop reproducing if I restart Safari.

[Ryosuke Niwa]

Created attachment 99856 [details]
screenshot

[Adrienne Walker]

I can't seem to reproduce this at ToT Chromium in Linux.  Does this happen in both Safari and Chromium?

Is this possibly triggered by compositing at all, such as a video or flash embedded in an email?

[Ryosuke Niwa]

(In reply to comment #2)
> I can't seem to reproduce this at ToT Chromium in Linux.  Does this happen in both Safari and Chromium?
> 
> Is this possibly triggered by compositing at all, such as a video or flash embedded in an email?

This happens almost all the time on Mac.  So maybe it's Mac-specific?  As far as I can tell, I haven't had any emails that had flash or video.

[Adrienne Walker]

(In reply to comment #3)
> (In reply to comment #2)
> > I can't seem to reproduce this at ToT Chromium in Linux.  Does this happen in both Safari and Chromium?
> > 
> > Is this possibly triggered by compositing at all, such as a video or flash embedded in an email?
> 
> This happens almost all the time on Mac.  So maybe it's Mac-specific?  As far as I can tell, I haven't had any emails that had flash or video.

Ok, I repro'd on Linux by making an element in an email compositing by adding -webkit-transform:translateZ(0).  If I scroll such that the bar changes to overlap a composited element and become composited, then it causes the problem.

When that bar becomes composited, it looks like it doesn't invalidate the root layer where it used to be drawn.

[Adrienne Walker]

Created attachment 100329 [details]
Test case

[Adrienne Walker]

(In reply to comment #5)
> Created an attachment (id=100329) [details]
> Test case

Ignore the <script> fluff that I forgot to cut.

If you load this test case in Chrome and manually scroll, you get bizarre repainting behavior, where the green fixed position element repeats incorrectly.  With --disable-accelerated-compositing, the fixed position element stays where it should be.

It's hard to get reasonable repros from gmail, but I suspect that this is the same issue as above.  It only happens in an iframe and it only happens when the iframe is composited.

Most unfortunately, this behavior does not seem to be fixed by the patch to bug 55257.

[Adrienne Walker]

rniwa: This test case only seems to cause problems for Chromium.  Did you say that the gmail errors were occuring in Safari as well?

[Ryosuke Niwa]

(In reply to comment #7)
> rniwa: This test case only seems to cause problems for Chromium.  Did you say that the gmail errors were occuring in Safari as well?

Yes.  I could reproduce the same issue on ToT WebKit + Safari.  But reproducing it on Safari was somewhat harder (had to browser longer).

[Adrienne Walker]

Created attachment 101532 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 101532 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=101532&action=review

> Source/WebCore/page/FrameView.cpp:1352
> +    if (root && root->layer()->isComposited()) {
> +        GraphicsLayer* layer = root->layer()->backing()->graphicsLayer();
> +        if (layer && layer->drawsContent()) {
> +            IntRect layerRect = updateRect;
> +            layerRect.move(scrollOffset());
> +            layer->setNeedsDisplayInRect(layerRect);
> +            return;

You're going behind the back of the normal repaint mechanisms here; if the root had a transform (which it does on Mac when pageScale != 1), then your repaint rect will be wrong.

Also, this root layer is "special" on Mac (via the paintingGoesToWindow logic), but think your drawsContent() check will account for that.

[Adrienne Walker]

(In reply to comment #10)
> (From update of attachment 101532 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=101532&action=review
> 
> > Source/WebCore/page/FrameView.cpp:1352
> > +    if (root && root->layer()->isComposited()) {
> > +        GraphicsLayer* layer = root->layer()->backing()->graphicsLayer();
> > +        if (layer && layer->drawsContent()) {
> > +            IntRect layerRect = updateRect;
> > +            layerRect.move(scrollOffset());
> > +            layer->setNeedsDisplayInRect(layerRect);
> > +            return;
> 
> You're going behind the back of the normal repaint mechanisms here; if the root had a transform (which it does on Mac when pageScale != 1), then your repaint rect will be wrong.

Is this similar to the logic in RenderObject::repaintUsingContainer where transform()->mapRect is used? You're right that I should use mapRect here as well if there is a transform.

I do agree with you that this feels a little like this patch is going behind the back of repaint mechanisms.  However, once I got into the RenderObject repaint logic, I couldn't sort out a reasonable way to reach back out and find the right RenderLayer or RenderView for the iframe content if it had its own graphics layer.  This was the best approach I could come up.

[Adrienne Walker]

> Is this similar to the logic in RenderObject::repaintUsingContainer where transform()->mapRect is used? You're right that I should use mapRect here as well if there is a transform.

Actually, wait.  Unless I'm misunderstanding something, RenderLayer::setNeedsDisplayInRect is in layer space, so doesn't need the transform.  RenderView::repaintViewRectangle, used in the RenderObject repaint path, is in view space, so needs the transform applied.

I think my original code is correct.

[Simon Fraser (smfr)]

Comment on attachment 101532 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=101532&action=review

>>> Source/WebCore/page/FrameView.cpp:1352
>>> +            return;
>> 
>> You're going behind the back of the normal repaint mechanisms here; if the root had a transform (which it does on Mac when pageScale != 1), then your repaint rect will be wrong.
>> 
>> Also, this root layer is "special" on Mac (via the paintingGoesToWindow logic), but think your drawsContent() check will account for that.
> 
> Is this similar to the logic in RenderObject::repaintUsingContainer where transform()->mapRect is used? You're right that I should use mapRect here as well if there is a transform.
> 
> I do agree with you that this feels a little like this patch is going behind the back of repaint mechanisms.  However, once I got into the RenderObject repaint logic, I couldn't sort out a reasonable way to reach back out and find the right RenderLayer or RenderView for the iframe content if it had its own graphics layer.  This was the best approach I could come up.

backing()->setContentsNeedDisplayInRect() would be more correct. Your version doesn't account for any extra padding that the compositing layer has. It should be closer to what RenderObject::repaintUsingContainer() does.

[Adrienne Walker]

Created attachment 101650 [details]
Patch

[Adrienne Walker]

(In reply to comment #14)
> Created an attachment (id=101650) [details]
> Patch

Fixed to account for iframes with padding and borders.  Now the invalidation sent to the backing is proplery in the space of the backing, rather than in the space of the iframe's enclosing RenderLayer, which included padding and borders.

Also, fixed the test to actually fail, thanks to layoutTestController.display().

[Adrienne Walker]

Committed r91597: <http://trac.webkit.org/changeset/91597>

[Adrienne Walker]

(In reply to comment #16)
> Committed r91597: <http://trac.webkit.org/changeset/91597>

Re: some comments from smfr in IRC about WK2.

This test fails in the WK2 minibrowser prior to this patch and seems to work better afterwards.  A small sampling of tests with iframes with compositing in the minibrowser also seems to have correct behavior.

WK2 has some unrelated transient redraw issues with this test, which I've filed here: https://bugs.webkit.org/show_bug.cgi?id=65042

[Ryosuke Niwa]

This changeset appears to have caused a bunch of tests on fail on Chromium Linux:
http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Linux%2032/builds/3195

[James Robinson]

No, those failures are due to http://trac.webkit.org/changeset/91599.  Will follow up on https://bugs.webkit.org/show_bug.cgi?id=64958