Bug 21256 – REGRESSION (r36906): horizontally repeating image leaves ghosts when vertical scrolling

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

[Charles]

My personal page (http://web.mit.edu/charles2/www) makes use of a fixed, horizontally repeating image via css properties. Whenever i click through to another URL from the page, then return to the page either by the back button, or by the "cmd-[" keyboard combination, the image leaves ghost image as i scroll up the page.

visit: http://mit/charles2/www/webkit-bug/ for a reference image of the issue.

Failing to reset "slow repaints"/"can blit" mode on the back navigation.

<rdar://problem/6362978>

[Cameron Zwarich (cpst)]

This regressed between r36882 and r37006.

[Cameron Zwarich (cpst)]

It looks like r36906 might be the culprit:

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

I'll test this theory.

[Cameron Zwarich (cpst)]

I confirmed that this regressed with r36906. The fix is pretty easy.

[Cameron Zwarich (cpst)]

Created attachment 25610 [details]
Proposed patch

Since we can't test back/forward cache behaviour in DRT, should I add a manual test for this? I'll do it, but I just want to know if this change is correct first.

Comment on attachment 25610 [details]
Proposed patch

I think this is not the best way to address the problem. This fix implies that there is a time when ScrollView::canBlitOnScroll() returns true while in fact it should be returning false. The correct fix would be to identify when the state of the platform widget changes and make sure that the flag in ScrollView is kept in sync.

[Cameron Zwarich (cpst)]

That's a good point. I'll try to fix it in a better way.

[Cameron Zwarich (cpst)]

Comment on attachment 25610 [details]
Proposed patch

Clearing review flag.

[Cameron Zwarich (cpst)]

(In reply to comment #7)
> (From update of attachment 25610 [details] [review])
> I think this is not the best way to address the problem. This fix implies that
> there is a time when ScrollView::canBlitOnScroll() returns true while in fact
> it should be returning false. The correct fix would be to identify when the
> state of the platform widget changes and make sure that the flag in ScrollView
> is kept in sync.

Is this really a good solution? Multiple ScrollViews can be linked to the same platform-level object. Do you traverse the back/forward cache and set it on each ScrollView every time that it is changed in the current page?

[Cameron Zwarich (cpst)]

My last comment was stupid. I don't really know much about Cocoa, so if this is going to be fixed the "right way", I won't be the one to do it. I am unassigning this bug.

[Cameron Zwarich (cpst)]

Created attachment 25935 [details]
Better patch in progress

I am just uploading this patch so I can get it on my new machine, where this bug is actually reproducible. My old machine has weird graphics driver issues that make it impossible to reproduce this bug.

[Cameron Zwarich (cpst)]

Created attachment 25936 [details]
Proposed patch

Here is a patch fixing this problem. It is impossible to test this in DRT, because the back/forward cache is disabled, but should I make a manual test for this problem?

[Cameron Zwarich (cpst)]

Created attachment 25937 [details]
Proposed patch (with no tabs in ChangeLog)

I always forget that TextMate on a new machine defaults to using tabs.

[Cameron Zwarich (cpst)]

Created attachment 25938 [details]
Proposed patch

I hope this is the final version of this, but I realized that my previous patch would break non-Mac platforms that have a platform widget for ScrollView.

[Cameron Zwarich (cpst)]

Created attachment 25962 [details]
Revised proposed patch

Here is a patch addressing one of Hyatt's comments on IRC.

[Dave Hyatt]

Comment on attachment 25962 [details]
Revised proposed patch

r=me.  I might add a little comment in the header file next to bool m_canBlitOnScroll; that explains it's not used on Mac.

[Cameron Zwarich (cpst)]

Landed in r39217.

[David Smith]

*** Bug 22440 has been marked as a duplicate of this bug. ***