Bug 64010 - In the preview theme of Gmail, yellow ad bar at the bottom won't repaint when scrolled
Summary: In the preview theme of Gmail, yellow ad bar at the bottom won't repaint when...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adrienne Walker
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-07-06 11:38 PDT by Ryosuke Niwa
Modified: 2011-07-22 13:58 PDT (History)
5 users (show)

See Also:


Attachments
screenshot (77.17 KB, image/png)
2011-07-06 11:38 PDT, Ryosuke Niwa
no flags Details
Test case (1.58 KB, patch)
2011-07-11 10:52 PDT, Adrienne Walker
no flags Details | Formatted Diff | Diff
Patch (5.97 KB, patch)
2011-07-20 17:30 PDT, Adrienne Walker
no flags Details | Formatted Diff | Diff
Patch (5.53 KB, patch)
2011-07-21 15:30 PDT, Adrienne Walker
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2011-07-06 11:38:18 PDT
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.
Comment 1 Ryosuke Niwa 2011-07-06 11:38:38 PDT
Created attachment 99856 [details]
screenshot
Comment 2 Adrienne Walker 2011-07-07 14:51:04 PDT
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?
Comment 3 Ryosuke Niwa 2011-07-07 15:11:27 PDT
(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.
Comment 4 Adrienne Walker 2011-07-07 15:19:52 PDT
(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.
Comment 5 Adrienne Walker 2011-07-11 10:52:41 PDT
Created attachment 100329 [details]
Test case
Comment 6 Adrienne Walker 2011-07-11 10:58:09 PDT
(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.
Comment 7 Adrienne Walker 2011-07-11 14:11:34 PDT
rniwa: This test case only seems to cause problems for Chromium.  Did you say that the gmail errors were occuring in Safari as well?
Comment 8 Ryosuke Niwa 2011-07-11 14:29:01 PDT
(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).
Comment 9 Adrienne Walker 2011-07-20 17:30:12 PDT
Created attachment 101532 [details]
Patch
Comment 10 Simon Fraser (smfr) 2011-07-20 17:36:10 PDT
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.
Comment 11 Adrienne Walker 2011-07-20 17:50:56 PDT
(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.
Comment 12 Adrienne Walker 2011-07-20 17:58:36 PDT
> 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.
Comment 13 Simon Fraser (smfr) 2011-07-20 18:07:25 PDT
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.
Comment 14 Adrienne Walker 2011-07-21 15:30:02 PDT
Created attachment 101650 [details]
Patch
Comment 15 Adrienne Walker 2011-07-21 15:34:38 PDT
(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().
Comment 16 Adrienne Walker 2011-07-22 12:53:04 PDT
Committed r91597: <http://trac.webkit.org/changeset/91597>
Comment 17 Adrienne Walker 2011-07-22 12:58:51 PDT
(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
Comment 18 Ryosuke Niwa 2011-07-22 13:53:02 PDT
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
Comment 19 James Robinson 2011-07-22 13:58:46 PDT
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