Bug 42949 – REGRESSION: Incorrect repaint on scrolling with position:fixed element on Windows

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

Rendering issues in latest Webkit were discovered when visiting Chromium's bug tracker at http://code.google.com/p/chromium/issues/detail?id=30101 and scrolling around.  Specifically, I scrolled to the bottom of the page and then hit the right-arrow key to scroll all the way to the right, then the left-arrow key to scroll back to the left.  This left a white line through the text on the left, as shown in the left part of the attached image.  Also, if I scroll up very quickly, text is cut off, as shown in the right part of the attached image.  Both artifacts are reset back to normal if I then move the page slightly up or down to trigger a repaint.  This was first discovered in Chromium running on FreeBSD and using WebKit 534.3-63992, then someone else confirmed on Windows 7 with Chromium using the same Webkit version.  I also tried it with latest Safari 5.0 using WebKit 533.16 on Windows XP and wasn't able to reproduce, but was able to reproduce on the same machine if I ran the Webkit nightly 63957.  This implies that this rendering bug snuck in somewhere between 533.16 and HEAD.

Created attachment 62529 [details]
image of rendering errors, mostly whitespace blocks

had to reattach because previous didn't show up

I just tested the latest webkit nightly 64160 on Windows XP and was able to reproduce again.  One issue might be that when I scroll to the right, the text from the bug comments moves underneath the bug status text at the top left.  To reproduce this, perhaps the browser window needs to be smaller, as it won't scroll for a very large window/monitor.

[Simon Fraser (smfr)]

<rdar://problem/8248500>

[Simon Fraser (smfr)]

Did this work in earlier Safari versions?

(In reply to comment #4)
> Did this work in earlier Safari versions?

I don't know as I don't use Safari normally, I just downloaded Safari 5.0 to test for this bug.  Since Safari 5.0/Webkit 533.16 doesn't have this bug, nor did earlier Chromium versions that used earlier Webkit rolls to my knowledge, I would guess it is a new bug, introduced sometime since Webkit 533.16.

I'm not sure this bug is specific to position:fixed and certainly not exclusive to Windows, the linked webpage was simply where I was able to reproduce this rendering flakiness consistently.  As noted earlier, I can also reproduce consistently on Chromium for FreeBSD, which basically uses the same rendering pipeline as linux.  I have also noticed that if I quickly scroll a page up or down, there are occasionally artifacts where a rectangle in the middle of the page is missing, but this is tougher to reproduce.  Perhaps this bug is related to scrolling movements somehow, as that was how I was able to reproduce the rendering error shown in the right-hand part of the attached image.

I went back and tried my last Chromium build on FreeBSD, 6.0.470 using Webkit 63601, to see if I could reproduce this bug and I could not.  I then tried some more Webkit nightlies in the vicinity of those commits on Windows XP and found that the r63859 nightly doesn't have this bug but the r63957 nightly is the first one where it shows up.  This should at least narrow down the commits where this bug was introduced to the 98 commits between 63859 and 63957.

This bug is also reproducible at http://copyfree.org.  Scroll up and down and notice how the menu on the left flickers and when you stop scrolling, the top or bottom of the menu is blocked by white artifacts.  I searched for scrolling in the commit log, around the 98 commits where this bug most likely was created, and this is the only one that seems relevant:

http://trac.webkit.org/changeset/63907

[Simon Fraser (smfr)]

Looks like I broke this.

[Simon Fraser (smfr)]

My patch changed the order in which scrollContents() and repaintFixedElementsAfterScrolling() are called.

[Darin Fisher (:fishd, Google)]

(In reply to comment #10)
> My patch changed the order in which scrollContents() and repaintFixedElementsAfterScrolling() are called.

That's definitely incorrect for Chromium.  It also doesn't match the name of the function... "repaint after scrolling" seems like something that should happen after scrolling.

Are you planning on fixing this?  Reverting?  Or, ...

[Simon Fraser (smfr)]

Note that jamesr did run test my patch on Chromium and said it was OK before I landed :)

I do plan to fix this; not sure if I'll be able to get to it any time soon though.

[Darin Fisher (:fishd, Google)]

Created attachment 64606 [details]
v1 patch

Here's a potential fix.  This reverts portions of http://trac.webkit.org/changeset/63907
in an effort to not break WebKit2 scrolling.  However, I have not confirmed that WebKit2
scrolling still works.

I think the repaintFixedElementsAfterScrolling function name is confusing since it
doesn't actually do any repainting.  It just updates paint rects in preparation for
scrollContentsFastPath, which does the actual painting.  So, it is necessary that
repaintFixedElementsAfterScrolling precede scrollContentsFastPath.

Perhaps the updateCompositingLayers call should not be made from
repaintFixedElementsAfterScrolling, but I don't know enough about how the compositor
works.

[Darin Fisher (:fishd, Google)]

I also don't know how to write a layout test for this :-(

[Darin Fisher (:fishd, Google)]

This patch does not attempt to fix the 'repaintFixedElementsAfterScrolling' name.  I want to just focus on the regression fix for now.  I'll file another bug about cleaning that up.  (It is also confusing that repaintFixedElementsAfterScrolling is responsible for re-positioning plugins!)

[Darin Adler]

Darin: Hyatt uses the verb "repaint" throughout WebKit to mean what you refer to here as "update paint rects". Cocoa uses the phrase "set needs display for this", Carbon uses "invalidate" for this. If we can come up with a better term we can perhaps remove the confusion.

[Darin Fisher (:fishd, Google)]

(In reply to comment #16)
> Darin: Hyatt uses the verb "repaint" throughout WebKit to mean what you refer 
> to here as "update paint rects". Cocoa uses the phrase "set needs display for 
> this", Carbon uses "invalidate" for this. If we can come up with a better term 
> we can perhaps remove the confusion.

I see.  Note, the function repaintFixedElementsAfterScrolling calls a
function named updateRepaintRectsAfterScroll, which actually does the work.
That function's name uses the erm "repaint" in a way that is not confusing.

[Darin Adler]

(In reply to comment #17)
> I see.  Note, the function repaintFixedElementsAfterScrolling calls a
> function named updateRepaintRectsAfterScroll, which actually does the work.
> That function's name uses the erm "repaint" in a way that is not confusing.

Well, it's good you like that function name, but it uses the phrase "repaint rects" where repaint is an adjective and was not what I was referring to.

I believe the use of "repaint" that was confusing you is the use of repaint as a verb as in these functions:

RenderObject::repaintUsingContainer
RenderObject::repaint
RenderObject::repaintRectangle
RenderObject::repaintAfterLayoutIfNeeded
RenderObject::repaintDuringLayoutIfMoved
RenderObject::repaintOverhangingFloats

RenderBox::repaintLayerRectsForImage

RenderLayer::repaintBlockSelectionGaps
RenderLayer::repaintIncludingDescendants
RenderLayer::repaintIncludingNonCompositingDescendants

MediaPlayer::repaint

Document::repaintMarkers

ScrollView::repaintContentRectangle
ScrollView::repaintFixedElementsAfterScrolling

FileChooser::repaint

MediaPlayerClient::mediaPlayerRepaint

Those are the ones we might want to change if we think they lead to confusion.

[Darin Adler]

Comment on attachment 64606 [details]
v1 patch

Thanks for tackling this.

Normally we use prefixes like [Chromium] on patches that only touch platform-specific code. This patch is entirely in platform-independent code, so we should not use the [Chromium] prefix on the bug's title. I think that's fixed in the bug, but it's still wrong in the patch.

Is there any way to make a regression test for this? Until we have at least a bit of automated testing I think we'll keep breaking this.

The division of responsibilities between ScrollView and FrameView here is not good. It seems bizarre that repaintFixedElementsAfterScrolling is a virtual function, yet scrollPositionChanged is non-virtual. I think we'd be better off if we made scrollPositionChanged a virtual function called by ScrollView. If we did that, then I think we might be able to leave the call to repaintFixedElementsAfterScrolling inside scrollPositionChanged.

It seems clear to me that the calls to repaintFixedElementsAfterScrolling should all be inside ScrollView, but the name of that function blurs the lines of responsibilities. After all, a scroll view doesn't know about HTML and the term "fixed elements" seems a little HTML-specific.

It would be worth regularizing and fixing this, although not required in this bug. At this point, it seems likely that any class other than FrameView derived from ScrollView will not work correctly because of the way we've set the classes up.

r=me on the change, but please consider spending a bit more time on this -- if we improve the division of responsibilities between ScrollView and FrameView we could save ourselves future headaches.

[Darin Fisher (:fishd, Google)]

(In reply to comment #19)
> Normally we use prefixes like [Chromium] on patches that only touch platform-specific code. This patch is entirely in platform-independent code, so we should not use the [Chromium] prefix on the bug's title. I think that's fixed in the bug, but it's still wrong in the patch.

I'll remove the [Chromium] label.  I added it based on the bug being Chromium specific, but you make a good point about this being a cross-platform code change.


> Is there any way to make a regression test for this? Until we have at least a bit of automated testing I think we'll keep breaking this.

James and I talked about this.  We don't have a way to regression test this today because it frankly depends on Chromium-specific painting behavior.  Instead of a repaint test, I want to add a framework to layout tests for logging scrolls and invalidates.  I think this will give us a good cross-platform way to test this code.  I'll fork that task off to a new bug.


> The division of responsibilities between ScrollView and FrameView here is not good. It seems bizarre that repaintFixedElementsAfterScrolling is a virtual function, yet scrollPositionChanged is non-virtual. I think we'd be better off if we made scrollPositionChanged a virtual function called by ScrollView. If we did that, then I think we might be able to leave the call to repaintFixedElementsAfterScrolling inside scrollPositionChanged.

It used to be the case that ScrollView called a virtual function named scrollPositionChanged.  That was refactored into repaintFixedElementsAfterScrolling.  I agree that this is all a big mess, and I'd like to get it cleaned up.


> It seems clear to me that the calls to repaintFixedElementsAfterScrolling should all be inside ScrollView, but the name of that function blurs the lines of responsibilities. After all, a scroll view doesn't know about HTML and the term "fixed elements" seems a little HTML-specific.

Indeed.  That layering violation struck me as unfortunate as well.


> r=me on the change, but please consider spending a bit more time on this -- if we improve the division of responsibilities between ScrollView and FrameView we could save ourselves future headaches.

Thanks, I'm going to commit this patch to fix the regression and then fork off additional work to separate bug(s).

[Simon Fraser (smfr)]

(In reply to comment #17)
> (In reply to comment #16)
> > Darin: Hyatt uses the verb "repaint" throughout WebKit to mean what you refer 
> > to here as "update paint rects". Cocoa uses the phrase "set needs display for 
> > this", Carbon uses "invalidate" for this. If we can come up with a better term 
> > we can perhaps remove the confusion.
> 
> I see.  Note, the function repaintFixedElementsAfterScrolling calls a
> function named updateRepaintRectsAfterScroll, which actually does the work.
> That function's name uses the term "repaint" in a way that is not confusing.

The main issue with repaintFixedElementsAfterScrolling() is that it doesn't actually do any repainting (aka invalidation). It just tells layers to update their rects (which does not actually cause repainting). And, of course, it now does other stuff.

[Simon Fraser (smfr)]

So the patch removes the one, true post-scroll bottleneck that was the intent of my original patch. I filed bug 44125 to put something back in.

[Darin Fisher (:fishd, Google)]

Landed as http://trac.webkit.org/changeset/65534

[Darin Fisher (:fishd, Google)]

I filed bug 44187 on devising changes to the layout test system that would help us not regress this again.