Bug 21256 - REGRESSION (r36906): horizontally repeating image leaves ghosts when vertical scrolling
Summary: REGRESSION (r36906): horizontally repeating image leaves ghosts when vertical...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Normal
Assignee: Cameron Zwarich (cpst)
URL: http://web.mit.edu/charles2/www
Keywords: InRadar, Regression
: 22440 (view as bug list)
Depends on:
Blocks:
 
Reported: 2008-09-30 15:26 PDT by Charles
Modified: 2008-12-12 18:19 PST (History)
4 users (show)

See Also:


Attachments
Proposed patch (1.33 KB, patch)
2008-11-30 06:21 PST, Cameron Zwarich (cpst)
no flags Details | Formatted Diff | Diff
Better patch in progress (3.02 KB, patch)
2008-12-10 20:13 PST, Cameron Zwarich (cpst)
no flags Details | Formatted Diff | Diff
Proposed patch (4.62 KB, patch)
2008-12-10 20:49 PST, Cameron Zwarich (cpst)
no flags Details | Formatted Diff | Diff
Proposed patch (with no tabs in ChangeLog) (4.67 KB, patch)
2008-12-10 20:51 PST, Cameron Zwarich (cpst)
no flags Details | Formatted Diff | Diff
Proposed patch (4.86 KB, patch)
2008-12-10 21:01 PST, Cameron Zwarich (cpst)
no flags Details | Formatted Diff | Diff
Revised proposed patch (5.72 KB, patch)
2008-12-11 13:31 PST, Cameron Zwarich (cpst)
hyatt: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Charles 2008-09-30 15:26:27 PDT
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.
Comment 1 mitz 2008-09-30 17:09:02 PDT
Failing to reset "slow repaints"/"can blit" mode on the back navigation.
Comment 2 mitz 2008-11-11 19:20:16 PST
<rdar://problem/6362978>
Comment 3 Cameron Zwarich (cpst) 2008-11-30 03:42:31 PST
This regressed between r36882 and r37006.
Comment 4 Cameron Zwarich (cpst) 2008-11-30 04:24:23 PST
It looks like r36906 might be the culprit:

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

I'll test this theory.
Comment 5 Cameron Zwarich (cpst) 2008-11-30 05:27:26 PST
I confirmed that this regressed with r36906. The fix is pretty easy.
Comment 6 Cameron Zwarich (cpst) 2008-11-30 06:21:15 PST
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 7 mitz 2008-11-30 22:08:16 PST
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.
Comment 8 Cameron Zwarich (cpst) 2008-11-30 22:13:38 PST
That's a good point. I'll try to fix it in a better way.
Comment 9 Cameron Zwarich (cpst) 2008-11-30 22:13:54 PST
Comment on attachment 25610 [details]
Proposed patch

Clearing review flag.
Comment 10 Cameron Zwarich (cpst) 2008-12-01 06:57:21 PST
(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?
Comment 11 Cameron Zwarich (cpst) 2008-12-04 08:44:20 PST
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.
Comment 12 Cameron Zwarich (cpst) 2008-12-10 20:13:27 PST
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.
Comment 13 Cameron Zwarich (cpst) 2008-12-10 20:49:34 PST
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?
Comment 14 Cameron Zwarich (cpst) 2008-12-10 20:51:01 PST
Created attachment 25937 [details]
Proposed patch (with no tabs in ChangeLog)

I always forget that TextMate on a new machine defaults to using tabs.
Comment 15 Cameron Zwarich (cpst) 2008-12-10 21:01:31 PST
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.
Comment 16 Cameron Zwarich (cpst) 2008-12-11 13:31:03 PST
Created attachment 25962 [details]
Revised proposed patch

Here is a patch addressing one of Hyatt's comments on IRC.
Comment 17 Dave Hyatt 2008-12-11 13:38:52 PST
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.
Comment 18 Cameron Zwarich (cpst) 2008-12-11 13:56:32 PST
Landed in r39217.
Comment 19 David Smith 2008-12-12 18:19:21 PST
*** Bug 22440 has been marked as a duplicate of this bug. ***