Bug 3416 - Scroll bars are sometimes not updated properly
Summary: Scroll bars are sometimes not updated properly
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 412
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Darin Adler
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2005-06-10 11:25 PDT by John Sullivan
Modified: 2005-12-07 09:53 PST (History)
2 users (show)

See Also:


Attachments
test case originally from radar 3996324 (356 bytes, text/html)
2005-06-10 11:26 PDT, John Sullivan
no flags Details
Screenshot of horizontal and vertical scrollbar artifacts (134.16 KB, image/png)
2005-06-15 04:34 PDT, David Kilzer (:ddkilzer)
no flags Details
QT movie of artifact being left on vertical scrollbar (364.74 KB, video/quicktime)
2005-06-15 05:00 PDT, David Kilzer (:ddkilzer)
no flags Details
Screenshot illustrating the issues (269.50 KB, image/png)
2005-06-21 19:02 PDT, Adam Schenker
no flags Details
Proposed patch (seems to improve the situation at least somewhat) (2.60 KB, patch)
2005-06-22 02:02 PDT, Adam Schenker
darin: review-
Details | Formatted Diff | Diff
New patch (changes to /WebCore/khtml/khtmlview.cpp only) (883 bytes, patch)
2005-06-22 11:25 PDT, Adam Schenker
no flags Details | Formatted Diff | Diff
Updated patch (only changes one line) (621 bytes, patch)
2005-06-24 01:28 PDT, Adam Schenker
no flags Details | Formatted Diff | Diff
Test Case for this issue (based on manual-test.html) (1.69 KB, text/html)
2005-06-24 01:34 PDT, Adam Schenker
no flags Details
New patch; appears to completely solve issue for me (1.41 KB, patch)
2005-06-27 04:37 PDT, Adam Schenker
hyatt: review-
Details | Formatted Diff | Diff
Patch that solves the problem for one test case; could this be the real fix!? (725 bytes, patch)
2005-08-06 18:20 PDT, Darin Adler
sullivan: review+
Details | Formatted Diff | Diff
3416 again? Or something new? (note URL progress indicator) (184.18 KB, image/png)
2005-12-07 08:47 PST, Adam Schenker
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description John Sullivan 2005-06-10 11:25:42 PDT
This is also in Radar as <rdar://problem/3996324>, where it has many duplicates.

This has been reported for many sites, with various degrees of reproducibility. Often it is completely 
reproducible for one person, but not for others.

Here is a test case that will sometimes reproduce the problem (uses a file that I'm about to attach)

1) Drag n drop test case  (frameset_test_file.htm) on Safari to launch it
2) When the file opens, it usually doesn't reproduce the scroll bar problem. In that case, drag the resize 
icon of the window so it's height is increased by at least 50 pixels. Close this window now.
3) Double click on the HTML file so that it opens in Safari. When frames are finished loading , you 
should see the painting issue on the scroll bar of the second frame. If this doesn't happen, close this 
window and double -click the file to reopen it again. This problem should occur if you continue this 
task.
Comment 1 John Sullivan 2005-06-10 11:26:44 PDT
Created attachment 2225 [details]
test case originally from radar 3996324
Comment 2 David Kilzer (:ddkilzer) 2005-06-14 14:56:52 PDT
Is the test case in attachment 2225 [details] supposed to be missing the initial '<' at
the beginning of the file (for the '<html>' tag)?
Comment 3 John Sullivan 2005-06-14 15:18:21 PDT
I'm 99.9% sure that the missing "<" is a copy/paste error somewhere, and not relevant for the test case. 
cpetersen@apple.com would know for sure.
Comment 4 David Kilzer (:ddkilzer) 2005-06-15 04:34:22 PDT
Created attachment 2357 [details]
Screenshot of horizontal and vertical scrollbar artifacts

Screenshot of artifacts left on horizontal and vertical scrollbars.  Taken from
my Radarweb duplicate bug, <rdar://problem/4139821>.
Comment 5 David Kilzer (:ddkilzer) 2005-06-15 04:43:32 PDT
Here is the Radarweb bug report I filed <rdar://problem/4139821>:

Summary: 

When loading a new web page in Safari, occasionally the scroll bars are not redrawn properly, leaving 
artifacts both horizontally and vertically.

Steps to Reproduce: 

1. Launch Safari 2.0.

2. Access this URL:  http://developer.apple.com/documentation/Carbon/Conceptual/
understanding_utis/index.html

3. Click on the "System-Declared Uniform Type Identifiers" link.

4. When the page loads, artifacts MAY appear.

5. If artifacts DO NOT appear, click on "An Overview of UTI Functions", then go to Step 3.  Most of time, 
I see the bug appear in Step 3 (when that link loads), but I've also seen it in Step 5, too.

Expected Results: 

No artifacts should be left on the screen.

Actual Results: 

Artifacts are left on the screen.

Regression: 

I have not tested other versions of Safari and/or Mac OS X.  The first time I saw it was with Safari 2.0 in 
Mac OS X 10.4.1.

Notes: 

- At times, I must click back and forth 20-30 times before I will see the bug appear.
- The bug seems to be related mostly to timing and how fast parts of the HTML document arrive at the 
client from the server.  [It's as if the screen update gets interrupted by itself to redraw the scrollbar, 
leaving an artifact.]
Comment 6 David Kilzer (:ddkilzer) 2005-06-15 05:00:34 PDT
Created attachment 2358 [details]
QT movie of artifact being left on vertical scrollbar

Quicktime movie captured by Snapz Pro 2 of artifacts left on vertical
scrollbar.  Taken from
my Radarweb duplicate bug, <rdar://problem/4139821>.
Comment 7 Adam Schenker 2005-06-21 18:44:24 PDT
This page often exhibits the scroll bar issues for me (see attachment): http://www.digitpress.com/
index2.htm. This problem has been around for awhile, and appears to manifest the following 
properties:

* it is intermittent (i.e. not always reproducible)
* it causes an extra copy of part of the knob graphic to appear in the wrong position on the scroll bar 
(see attachment)
* it causes part of the page background color to overwrite part of the scroll bar, usually the outer half 
(see attachment)
* it seems to happen on pages with multiple frames which include tables
* it affects both horizontal and vertical scroll bars (but vertical bars are more frequent for me)

I have been trying to locate this bug, but I haven't had any success yet. Anyone have any ideas?
Comment 8 Adam Schenker 2005-06-21 19:02:03 PDT
Created attachment 2538 [details]
Screenshot illustrating the issues
Comment 9 Adam Schenker 2005-06-22 02:02:55 PDT
Created attachment 2543 [details]
Proposed patch (seems to improve the situation at least somewhat)

This patch affects 3 files:

1. WebCore/khtml/khtmlview.cpp:
    - calls to suppressScrollBars() function apparently missing correct second
parameter value in some
    cases

2. /WebKit/WebView.subproj/WebDynamicScrollBarsView.m:
     - implemented initWithFrame for this class to handle missing
initialization of inUpdateScrollers
     - removed return at start of updateScrollers for inUpdateScrollers == true
(I think this
     implementation could drop some messages)

3./WebKit/WebView.subproj/WebDynamicScrollBarsView.h:
     added interface definition for initWithFrame

This appears to fix the problems for me, at least most of the time. However,
there are still some artifacts during rendering while the page is still
loading.
Comment 10 Darin Adler 2005-06-22 10:45:14 PDT
Comment on attachment 2543 [details]
Proposed patch (seems to improve the situation at least somewhat)

The -[WebDynamicScrollBarsView initWithFrame:] part of this patch is definitely
wrong. All instance variables in Objective-C start with 0, so there's no need
to write code to set inUpdateScrollers to false.

The change to not use inUpdateScrollers in WebDynamicScrollBarsView is
definitely wrong. The whole reason that boolean exists is to prevent reentrancy
that causes even more problems.

I'm not as sure, though, about the KHTMLView changes. I'll wait to hear what
Dave Hyatt has to say about those.

We're definitely not going to commit a patch based on guesswork without some
clearer idea of exactly what it does fix. In particular, we need an example of
at least one set of reproducible steps that failed before and now works with a
patch before we'd even consider it.
Comment 11 Adam Schenker 2005-06-22 11:25:19 PDT
Created attachment 2547 [details]
New patch (changes to /WebCore/khtml/khtmlview.cpp only)
Comment 12 Adam Schenker 2005-06-22 11:40:08 PDT
About the changes to /WebCore/khtml/khtmlview.cpp:

The second parameter to suppressScrollBars() is a bool "repaintOnUnsuppress" which is ultimately 
passed to setScrollBarsSuppressed (in WebDynamicScrollBarsView.m) as BOOL "repaint". The default for 
repaintOnUnsuppress is false. This means that with the following code:

    if (suppressed || repaint) {
        [[self verticalScroller] setNeedsDisplay: !suppressed];
        [[self horizontalScroller] setNeedsDisplay: !suppressed];
    }

the statements inside the conditional are never executed unless either: 1. the scrollbars are suppressed 
(not supposed to be drawn) or 2. repaint (i.e. repaintOnUnsuppress) is true. Consequently, visible 
scrollbars don't have their needsDisplay set to YES here.
Comment 13 Adam Schenker 2005-06-24 01:28:59 PDT
Created attachment 2615 [details]
Updated patch (only changes one line)
Comment 14 Adam Schenker 2005-06-24 01:34:33 PDT
Created attachment 2620 [details]
Test Case for this issue (based on manual-test.html)
Comment 15 Adam Schenker 2005-06-24 02:13:23 PDT
I have provided an explanation of the patch and a test case, which I hope will be acceptable for a 
review. For me it fixes the artifact issues associated with this bug (pieces of scroll bar knobs and 
background colors being left in the scroll bar); I'm less sure about the knob size issue, but at the very 
least it is much improved.

Basically this one line patch changes the call to suppressScrollBars() in KHTMLView::restoreScrollBar() 
from suppressScrollBars(false) to suppressScrollBars(false, true), so that the scroll bars will be repainted 
correctly.

I also wanted to bring up some things about the WebDynamicScrollBars.m code (which is not changed 
in this patch) for discussion.

First, in updateScrollers, the conditional on lines 77-78 starts with:

hasVerticalScroller != oldHasVertical || hasHorizontalScroller != oldHasHorizontal || ...

However, as far as I can tell, both of these always evaluate to false, because the two variables being 
compared are always equal; if this is indeed the case, those conditions and the oldHasVertical and 
oldHasHorizontal variables can be removed.

Second, also in updateScrollers, on lines 56-57:

    if (inUpdateScrollers)
        return;

I'm not too sure about this code for handling critical sections (but I'm new at Obj-C). Shouldn't we use 
@synchronize(self) {...} here? Or maybe:

    while(inUpdateScrollers)
        ; // do nothing

It just seems that it is possible (and likely) that a call to this method could be ignored, because the 
caller does not wait to obtain access to the critical section. Instead a call just immediately returns 
without anything happening.

Finally, I'm not really confident about how setNeedsDisplay: on the scroll bars is done in this file. The 
only place in this file that does setNeedsDisplay:YES is in setScrollBarsSuppressed; shouldn't it be done 
in updateScrollers as well?
Comment 16 Adam Schenker 2005-06-27 04:37:36 PDT
Created attachment 2664 [details]
New patch; appears to completely solve issue for me

I think this finally fixes the issue completely. The change to
/WebCore/khtml/khtmlview.cpp is as before. This change fixes the bits of scroll
bar knob graphic and page background colors being left on the scroll bar. As
mentioned previously, the problem is the call to

setScrollBarsSuppressed:(BOOL)suppressed repaintOnUnsuppress:(BOOL)repaint

originating in KHTMLView::restoreScrollBar does not set repaint to true. The
other change is in /WebKit/WebView.subproj/WebDynamicScrollBarsView.m, which
removes this chunk of code (from reflectScrolledClipView):

    // Validate the scrollers if they're being suppressed.
    if (suppressScrollers) {
	[[self verticalScroller] setNeedsDisplay: NO];
	[[self horizontalScroller] setNeedsDisplay: NO];
    }

This change fixes the problem of the scroll knob being set to the incorrect
size and then leaving artifacts while scrolling. This code is already performed
in the function updateScrollers, which is called just before these lines, and
is therefore redundant. The problem with having this code fragment in this
method appears to be that it causes a race condition or other synchronization
issue. As far as I can tell, this code is completely superfluous, even in
updateScrollers; the only time setNeedsDisplay should be done is in
setScrollBarsSuppressed, which is where suppressScrollers is assigned.
Otherwise there is no telling when the value of suppressScrollers may change.

I have repeatedly tried both test cases many times, and they both worked every
time with this patch. There are still sometimes artifacts briefly during
loading/rendering of the page, but once the page has loaded they are correct.
Comment 17 Dave Hyatt 2005-07-09 13:54:50 PDT
Comment on attachment 2664 [details]
New patch; appears to completely solve issue for me

Patch is not right.  Completely commenting out suppression of scrollers, the
bug still occurs.  The only reason this patch happens to fix the bug is it
forces a scroller repaint when parsing is finished (because of the change to
restoreScollBar).

The paint bug in reality is unrelated to the suppression code and probably has
more to do with the setAsideSubviews hackery to make frames work properly with
z-index.
Comment 18 Darin Adler 2005-08-06 18:20:58 PDT
Created attachment 3245 [details]
Patch that solves the problem for one test case; could this be the real fix!?

I think I may have found the real underlying problem. Here's the patch.
Comment 19 John Sullivan 2005-08-06 20:19:18 PDT
Comment on attachment 3245 [details]
Patch that solves the problem for one test case; could this be the real fix!?

r=me, assuming you either tested performance or are confident that this won't
affect our performance. Excellent!
Comment 20 Adam Schenker 2005-08-07 13:55:04 PDT
Looks good. I can reproduce the bug with the CVS build, but this patch appears to fix it.
Comment 21 Adam Schenker 2005-12-07 08:45:35 PST
MacNN (http://macnn.com) recently updated their page design; the updated design causes this bug to 
appear intermittently on both the latest official release (2.0.2/416.13) as well as the current CVS build. In 
addition, sometimes the scroll knob will jump to a lower position on the page or the progress indicator in 
the URL bar will have glitches (see attachment).
Comment 22 Adam Schenker 2005-12-07 08:47:00 PST
Created attachment 4990 [details]
3416 again? Or something new? (note URL progress indicator)
Comment 23 Darin Adler 2005-12-07 09:53:16 PST
If you're seeing a similar symptom on MacNN I would *not* assume it's the same as what we fixed in this 
bug. Please file a new bug report.