Bug 80641 - REGRESSION (r93614): scrolling div does not repaint
Summary: REGRESSION (r93614): scrolling div does not repaint
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Shawn Singh
Depends on: 80833
  Show dependency treegraph
Reported: 2012-03-08 14:34 PST by Shawn Singh
Modified: 2012-03-12 07:01 PDT (History)
6 users (show)

See Also:

Patch (6.79 KB, patch)
2012-03-10 00:24 PST, Shawn Singh
no flags Details | Formatted Diff | Diff
Patch for landing (6.80 KB, patch)
2012-03-11 14:56 PDT, Shawn Singh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Shawn Singh 2012-03-08 14:34:13 PST
I believe it is a simple fix, but Julien should be able to make the "correct final fix" far more quickly and with awareness of the performance issues involved.

cc Dana just in case this is magically related to our other bugs where paint rects seem wrong, too.

To reproduce the problem:
   go to http://www.railgun3d.com:8080/gltest5/demo.htm   (requires webGL)
   scrolling the div does not repaint the divs contents.

If you cannot reproduce:
   go to inspector
   turn off "position: absolute" for the 200x200 div that has the scrollbar
   turn it back on.
   now it should reliably reproduce.

The problem is that the repaint rect associated with the 200x200 div is not re-computed, and therefore its position is way off to the side.  I bisected this down to 93614.

Hacked solution:
  forcing computeRepaintRects() in all cases for RenderLayer::updateLayerPositionsAfterScroll does indeed fix the problem.
Comment 1 Shawn Singh 2012-03-08 17:57:15 PST
To save time for Julien, he essentially told me the correct fix (and it seems to work);  I'll create a reduced test case, verify the fix, and upload a patch.

Thanks Julien!
Comment 2 Simon Fraser (smfr) 2012-03-08 20:42:42 PST
Comment 3 Shawn Singh 2012-03-09 14:05:27 PST
Update - our expected fix was not yet correct.  Needs further investigation, there seem to be enough clues to continue investigating.
Comment 4 Shawn Singh 2012-03-10 00:15:40 PST
I've traced through the problem, and I have it figured out.  Patch is coming in a moment.

1. At first, there is a non-composited layer which paints into an ancestor repaintContainer.  because it is positioned, the repaintRect has a non-zero offset.

2. Then, the layer changes position, and becomes composited.  At that time, we are not updating the repaintRect.  (The code in FrameView::layout() calls updateLayerPositions() where repaintRects are already computed, before updateCompositingLayers().)

3. However, because the layer became composited, it became its own repaintContainer, and the repaintRect should have been updated.  But its not, and the repaint rect is skewed by some incorrect offset, and the actual contents don't get repainted.

I feel it is correct to computeRepaintRects when a newly composited layer is created.  Specifically, we have to wait until the backing is created before we can computeRepaintRects correctly.
Comment 5 Shawn Singh 2012-03-10 00:24:22 PST
Created attachment 131162 [details]

The included layout test reproduces the problem on both webkit nightly and tip-of-tree chromium before this patch is applied.
Comment 6 Simon Fraser (smfr) 2012-03-11 11:28:45 PDT
Comment on attachment 131162 [details]

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

> LayoutTests/compositing/repaint/newly-composited-repaint-rect.html:67
> +        if (window.layoutTestController) layoutTestController.display();

Use two lines for this.
Comment 7 Shawn Singh 2012-03-11 14:56:51 PDT
Created attachment 131258 [details]
Patch for landing
Comment 8 WebKit Review Bot 2012-03-11 15:17:54 PDT
Comment on attachment 131258 [details]
Patch for landing

Clearing flags on attachment: 131258

Committed r110401: <http://trac.webkit.org/changeset/110401>
Comment 9 WebKit Review Bot 2012-03-11 15:17:59 PDT
All reviewed patches have been landed.  Closing bug.