Bug 3416

Summary:

Scroll bars are sometimes not updated properly

Product:

WebKit

Reporter:

John Sullivan <sullivan>

Component:

New Bugs

Assignee:

Darin Adler <darin>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

aschenke, ddkilzer

Priority:

Version:

412

Hardware:

Mac

OS:

OS X 10.4

Attachments:

Description	Flags
test case originally from radar 3996324	none
Screenshot of horizontal and vertical scrollbar artifacts	none
QT movie of artifact being left on vertical scrollbar	none
Screenshot illustrating the issues	none
Proposed patch (seems to improve the situation at least somewhat)	darin: review-
New patch (changes to /WebCore/khtml/khtmlview.cpp only)	none
Updated patch (only changes one line)	none
Test Case for this issue (based on manual-test.html)	none
New patch; appears to completely solve issue for me	hyatt: review-
Patch that solves the problem for one test case; could this be the real fix!?	sullivan: review+
3416 again? Or something new? (note URL progress indicator)	none

John Sullivan

Reported 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.

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
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

John Sullivan

Comment 1 2005-06-10 11:26:44 PDT

Created attachment 2225 [details] test case originally from radar 3996324

David Kilzer (:ddkilzer)

Comment 2 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)?

John Sullivan

Comment 3 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.

David Kilzer (:ddkilzer)

Comment 4 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>.

David Kilzer (:ddkilzer)

Comment 5 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.]

David Kilzer (:ddkilzer)

Comment 6 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>.

Adam Schenker

Comment 7 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?

Adam Schenker

Comment 8 2005-06-21 19:02:03 PDT

Created attachment 2538 [details] Screenshot illustrating the issues

Adam Schenker

Comment 9 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.

Darin Adler

Comment 10 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.

Adam Schenker

Comment 11 2005-06-22 11:25:19 PDT

Created attachment 2547 [details] New patch (changes to /WebCore/khtml/khtmlview.cpp only)

Adam Schenker

Comment 12 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.

Adam Schenker

Comment 13 2005-06-24 01:28:59 PDT

Created attachment 2615 [details] Updated patch (only changes one line)

Adam Schenker

Comment 14 2005-06-24 01:34:33 PDT

Created attachment 2620 [details] Test Case for this issue (based on manual-test.html)

Adam Schenker

Comment 15 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?

Adam Schenker

Comment 16 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.

Dave Hyatt

Comment 17 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.

Darin Adler

Comment 18 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.

John Sullivan

Comment 19 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!

Adam Schenker

Comment 20 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.

Adam Schenker

Comment 21 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).

Adam Schenker

Comment 22 2005-12-07 08:47:00 PST

Created attachment 4990 [details] 3416 again? Or something new? (note URL progress indicator)

Darin Adler

Comment 23 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.

Note You need to log in before you can comment on or make changes to this bug.