Scroll bars don't render correctly on some HTML pages, even though there is a portion of the view reserved for them

• Component: Layout and Rendering
• Status: NEW ---

[Ananta]

Launch Safari and navigate to pbskids.org.
When the page finishes rendering, observe that the vertical/horizontal
scroll bars dont show up and this portion of the view is just painted white.

It is basically a dead area in the view which is unresponsive to user input.
If you resize the window, the scroll bars render correctly sometimes.

I will be attaching a reduced test case with this.

[Ananta]

Created attachment 17721 [details]
Reduced test case

This test case has been created by reducing most of the content from
pbskids.org.

[Ananta]

It looks like this also happens on the Mac.

Pretty sure there is a duplicate in Bugzilla.

[Ananta]

Created attachment 17728 [details]
Please ignore this patch.

[Ananta]

Comment on attachment 17728 [details]
Please ignore this patch.

Index: WebCore/ChangeLog
===================================================================
--- WebCore/ChangeLog	(revision 28482)
+++ WebCore/ChangeLog	(working copy)
@@ -1,3 +1,31 @@
[phone]  Ananta iyengar  <[e-mail]>
+
+        Reviewed by NOBODY (OOPS!).
+
+		Proposed fix for http://bugs.webkit.org/show_bug.cgi?id=16310, which is an intermittent 
+		scrolling observed in Safari. This is a bug in webkit. Details as below:-
+
+		The decision on whether the frame view needs horizontal/vertical scroll bars is made in
+		the webkit function WebCore::ScrollView::updateScrollbars. In certain cases it 
+		makes this decision based on whether the view size is wider/larger than the frame 
+		width/height. It runs the corresponding loop twice apparently to cover for the case where 
+		a view had scroll bars initially and loses them. This is the reverse case, where the view 
+		originally did not have scroll bars and the first pass of the loop decides that it needs 
+		them. The second pass of the loop, first causes a frame layout to occur, which reduces the 
+		height/width of the view by	the height and width of the horizontal and vertical scroll bars 
+		respectively. It then checks if the view size is greater than the frame size, which is not 
+		the case anymore and thus removes the scrollbars from the view.
+
+		As a result we are left with a dead region in the view which is not painted and is not responsive 
+		to user input. Resizing the window causes the scrollbars to show up in some cases.
+		
+		We also need to suppress new scroll bar calculations during resize. These calculations can 
+		occur when the first layout after resize is done. 
+
+        * platform/win/ScrollViewWin.cpp:
+        (WebCore::ScrollView::setFrameGeometry):
+        (WebCore::ScrollView::updateScrollbars):
+
 2007-12-05  Darin Adler  <[e-mail]>
 
         Reviewed by Maciej.
Index: WebCore/platform/win/ScrollViewWin.cpp
===================================================================
--- WebCore/platform/win/ScrollViewWin.cpp	(revision 28433)
+++ WebCore/platform/win/ScrollViewWin.cpp	(working copy)
@@ -281,7 +281,11 @@ void ScrollView::setFrameGeometry(const 
         return;
 
     if (newGeometry.width() != oldGeometry.width() || newGeometry.height() != oldGeometry.height()) {
+        // Suppress new scroll bar calculations until the first
+        // layout after resize has occured.        
+        suppressScrollbars(true);
         updateScrollbars(m_data->m_scrollOffset);
+        suppressScrollbars(false);
         static_cast<FrameView*>(this)->setNeedsLayout();
     }
 
@@ -464,23 +468,36 @@ void ScrollView::updateScrollbars(const 
     const int cVerticalWidth = PlatformScrollbar::verticalScrollbarWidth();
     const int cHorizontalHeight = PlatformScrollbar::horizontalScrollbarHeight();
 
-    for (int pass = 0; pass < 2; pass++) {
-        bool scrollsVertically;
-        bool scrollsHorizontally;
+    bool scrollsVertically = false;
+    bool scrollsHorizontally = false;
 
+    for (int pass = 0; pass < 2; pass++) {
         if (!m_data->m_scrollbarsSuppressed && (hScroll == ScrollbarAuto || vScroll == ScrollbarAuto)) {
             // Do a layout if pending before checking if scrollbars are needed.
             if (hasVerticalScrollbar != oldHasVertical || hasHorizontalScrollbar != oldHasHorizontal)
                 static_cast<FrameView*>(this)->layout();
 
-            scrollsVertically = (vScroll == ScrollbarAlwaysOn) || (vScroll == ScrollbarAuto && contentsHeight() > height());
+            // The frame view needs horizontal/vertical scrollbars if either of the following 
+            // conditions are true:-
+            // 1. The decisions from the first pass of this loop.
+            // 2. The frame view height/width is greater than the frame height/width. 
+            // This fixes the following scenario.
+            // The first pass of the loop determines that we need a horizontal or vertical scroll bar. 
+            // As a result we invoke the layout call on the FrameView in the second pass. This reduces 
+            // the view size by the height and width of the horizontal/vertical scroll bars. The code
+            // below decides whether the view needs a horizontal/vertical scroll bar by checking
+            // if the view width/height is greater than the frame width/height. These checks fail
+            // thus causing the view to not have scrollbars when it should, thus resulting in a view
+            // which is not painted correctly. The scroll bar region of the view is a dead region 
+            // unresponsive to input.            
+            scrollsVertically |= (vScroll == ScrollbarAlwaysOn) || (vScroll == ScrollbarAuto && contentsHeight() > height());
             if (scrollsVertically)
-                scrollsHorizontally = (hScroll == ScrollbarAlwaysOn) || (hScroll == ScrollbarAuto && contentsWidth() + cVerticalWidth > width());
+                scrollsHorizontally |= (hScroll == ScrollbarAlwaysOn) || (hScroll == ScrollbarAuto && contentsWidth() + cVerticalWidth > width());
             else {
-                scrollsHorizontally = (hScroll == ScrollbarAlwaysOn) || (hScroll == ScrollbarAuto && contentsWidth() > width());
+                scrollsHorizontally |= (hScroll == ScrollbarAlwaysOn) || (hScroll == ScrollbarAuto && contentsWidth() > width());
                 if (scrollsHorizontally)
-                    scrollsVertically = (vScroll == ScrollbarAlwaysOn) || (vScroll == ScrollbarAuto && contentsHeight() + cHorizontalHeight > height());
-            }
+                    scrollsVertically |= (vScroll == ScrollbarAlwaysOn) || (vScroll == ScrollbarAuto && contentsHeight() + cHorizontalHeight > height());
+            }     
         }
         else {
             scrollsHorizontally = (hScroll == ScrollbarAuto) ? hasHorizontalScrollbar : (hScroll == ScrollbarAlwaysOn);

[Ananta]

Please ignore the initial patch and the last comment, which got added by mistake :(
I am uploading a new patch. Please review the same.

[Ananta]

Created attachment 17741 [details]
Proposed fix for this issue

[Dave Hyatt]

Comment on attachment 17741 [details]
Proposed fix for this issue

updateScrollbars is ported from the Mac.  Does the Mac code have the same bug?

(In reply to comment #8)
> (From update of attachment 17741 [details] [edit])
> updateScrollbars is ported from the Mac.  Does the Mac code have the same bug?
> 

Yes. See also bug 14790 and bug 12440 comment #21.

[Maciej Stachowiak]

Not a full review, but usually in WebKit we wouldn't put such a long comment in the middle of a function. A brief one-line comment citing this bug may be hflpful but even that does not seem necessary.

It would also be good to fix the Mac port along the same lines.

[Ananta]

Created attachment 17831 [details]
Updated patch with the comments in the code taken out as per request

[Ananta]

Created attachment 17832 [details]
Proposed scolling fix for Mac

Proposed scrolling fix for Mac. I could not test this as I don't have the necessary build environment for Mac. If this does not compile, please bear with me :(

[Darin Adler]

Comment on attachment 17832 [details]
Proposed scolling fix for Mac

Patch uses tabs in ChangeLog. Please don't use tabs in any source file. We can't check in patches that use tabs because the repository will reject them.

Are you certain that the rule is a good one? It seems that it will result in scroll bars in cases where we don't need them.

[Darin Adler]

Created attachment 17934 [details]
new patch that fixes the tabs issue and includes the Macintosh side

Here's a new patch. But the Mac version does not seem to include the whole fix. I don't see the scroll bar suppression part of the fix. So I'm not yet marking it for review. Someone should get the Mac side working.

[Darin Adler]

Created attachment 17935 [details]
slightly improved patch

Fixed name in change log.

[Ananta]

Hi Darin

I will do some more testing on this and let you know if there are scenarios where
we may end up with scrollbars when we don't need them.

-Ananta

[Ananta]

Hi Darin

I tested this patch a bit and it works well. 

However, looking at the code in updateScrollBars, there may be a corner case as below:-
1. The two pass loop makes a decision that scrollbars are needed in the first  
   pass.
2. The subsequent frame view layout in the second pass now results in the 
   contents size shrinking to the size of the visible area in the frame which 
   excludes the width/height of the frame. We do call updateScrollBars again in 
   the context of the layout function, however it does not do anything as it is
   in the same context.
3. After the two pass loop exits, we then check if the scrollbars should be 
   enabled, by checking the contentsheight/width with the visible height/width,
   which could reduce due to the layout. As a result we may end up with disabled
   scrollbars.

I could not reproduce this though. I think the patch is good for now.


 

[Ananta]

Hi Darin

It looks like there is indeed an issue with the disabled scrollbars showing up incorrectly. Please hold off on this update, while I do some more investigation into this. Will update this bug with more info.

Thanks
Ananta