Bug 108255 - Scrollbar and scroll corner composited layers positioned incorrectly
Summary: Scrollbar and scroll corner composited layers positioned incorrectly
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: James Robinson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-01-29 16:50 PST by James Robinson
Modified: 2013-01-29 22:33 PST (History)
4 users (show)

See Also:


Attachments
Patch (2.37 KB, patch)
2013-01-29 21:14 PST, James Robinson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Robinson 2013-01-29 16:50:51 PST
Originally reported as https://code.google.com/p/chromium/issues/detail?id=170264

Open this repro case in Chrome with --force-compositing-mode or a recent WebKit nightlies:

<!DOCTYPE html>
<html>
 <body>
  <div style="position:relative; float:right; width:40px; height:20px; right:-20px; background-color:#090"></div>
  <div style="height:400px; background-color:#900;"></div>
 </body>
</html>


data:text/html;charset=utf-8,<!DOCTYPE%20html>%0A<html>%0A%20<body>%0A%20%20<div%20style%3D"position%3Arelative%3B%20float%3Aright%3B%20width%3A40px%3B%20height%3A20px%3B%20right%3A-20px%3B%20background-color%3A%23090"><%2Fdiv>%0A%20%20<div%20style%3D"height%3A400px%3B%20background-color%3A%23900%3B"><%2Fdiv>%0A%20<%2Fbody>%0A<%2Fhtml>

and resize the window to be close to the size of the red square.  At certain sizes, the page goes nearly white.  The scroll corner layer on the root frame is positioned at 15x15 and occupies the rest of the frame.  Somehow the overflow control layer positioning logic is running before the scrollbars get correctly sized.
Comment 1 James Robinson 2013-01-29 17:07:48 PST
When we end up doing multiple passes through ScrollView::updateScrollbars(), we sometimes end up not calling positionScrollbarLayers() at all since it's inside this check:

http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/platform/ScrollView.cpp&exact_package=chromium&q=ScrollView.cpp&type=cs&l=608

Just unconditionally calling positionScrollbarLayers() fixes this, but that's probably not ideal.

+cc a few other ScrollView folks
Comment 2 James Robinson 2013-01-29 18:49:23 PST
We end up recursively calling updateScrollbars repeatedly on this page up to cMaxUpdateScrollbarPass.  On the first call, the ScrollView has horizontal and vertical scrollbars.  Layout results in no overflow, so updateScrollbars is called to remove them.  After a few rounds of this, we end up calling positionScrollbarLayers() with this callstack:

#0  WebCore::ScrollView::positionScrollbarLayers (this=0x79ac6800) at Source/WebCore/platform/ScrollView.cpp:940
#1  0x0c89576a in WebCore::RenderLayerCompositor::updateOverflowControlsLayers (this=0x79471db0) at Source/WebCore/rendering/RenderLayerCompositor.cpp:2411
#2  0x0c894614 in WebCore::RenderLayerCompositor::frameViewDidChangeSize (this=0x79471db0) at Source/WebCore/rendering/RenderLayerCompositor.cpp:1175
#3  0x0d7dbb2f in WebCore::FrameView::visibleContentsResized (this=0x79ac6800) at Source/WebCore/page/FrameView.cpp:2009
#4  0x0aca7354 in WebCore::ScrollView::updateScrollbars (this=0x79ac6800, desiredOffset=@0xbfff0a70) at Source/WebCore/platform/ScrollView.cpp:474
#5  0x0aca9670 in WebCore::ScrollView::setContentsSize (this=0x79ac6800, newSize=@0xbfff0b40) at Source/WebCore/platform/ScrollView.cpp:308
#6  0x0d7d2734 in WebCore::FrameView::setContentsSize (this=0x79ac6800, size=@0xbfff0b40) at Source/WebCore/page/FrameView.cpp:557
#7  0x0d7d2a01 in WebCore::FrameView::adjustViewSize (this=0x79ac6800) at Source/WebCore/page/FrameView.cpp:586
#8  0x0d7d5a42 in WebCore::FrameView::layout (this=0x79ac6800, allowSubtree=true) at Source/WebCore/page/FrameView.cpp:1231
#9  0x0d7dbae9 in WebCore::FrameView::visibleContentsResized (this=0x79ac6800) at Source/WebCore/page/FrameView.cpp:2004
#10 0x0aca7aac in WebCore::ScrollView::updateScrollbars (this=0x79ac6800, desiredOffset=@0xbfff1030) at Source/WebCore/platform/ScrollView.cpp:547
#11 0x0aca9670 in WebCore::ScrollView::setContentsSize (this=0x79ac6800, newSize=@0xbfff1100) at Source/WebCore/platform/ScrollView.cpp:308
#12 0x0d7d2734 in WebCore::FrameView::setContentsSize (this=0x79ac6800, size=@0xbfff1100) at Source/WebCore/page/FrameView.cpp:557
#13 0x0d7d2a01 in WebCore::FrameView::adjustViewSize (this=0x79ac6800) at Source/WebCore/page/FrameView.cpp:586
#14 0x0d7d5a42 in WebCore::FrameView::layout (this=0x79ac6800, allowSubtree=true) at Source/WebCore/page/FrameView.cpp:1231
#15 0x0d7dbae9 in WebCore::FrameView::visibleContentsResized (this=0x79ac6800) at Source/WebCore/page/FrameView.cpp:2004
#16 0x0aca7aac in WebCore::ScrollView::updateScrollbars (this=0x79ac6800, desiredOffset=@0xbfff15f0) at Source/WebCore/platform/ScrollView.cpp:547
#17 0x0aca9670 in WebCore::ScrollView::setContentsSize (this=0x79ac6800, newSize=@0xbfff16c0) at Source/WebCore/platform/ScrollView.cpp:308
#18 0x0d7d2734 in WebCore::FrameView::setContentsSize (this=0x79ac6800, size=@0xbfff16c0) at Source/WebCore/page/FrameView.cpp:557
#19 0x0d7d2a01 in WebCore::FrameView::adjustViewSize (this=0x79ac6800) at Source/WebCore/page/FrameView.cpp:586
#20 0x0d7d5a42 in WebCore::FrameView::layout (this=0x79ac6800, allowSubtree=true) at Source/WebCore/page/FrameView.cpp:1231
#21 0x0d7e0d5b in WebCore::FrameView::updateLayoutAndStyleIfNeededRecursive (this=0x79ac6800) at Source/WebCore/page/FrameView.cpp:3418

where we get a non-sensical scrollCornerRect():

  m_location = {
    m_x = 15, 
    m_y = 15
  }, 
  m_size = {
    m_width = 769, 
    m_height = 413
  }

since the scrollbars are inconsistent.  The scrollbars get fixed up as the stack unwinds, but since the ScrollView ends up with both vertical and horizontal scrollbars (just like it started with), it doesn't update the layer geometry again:

    if (hasHorizontalScrollbar != (m_horizontalScrollbar != 0) || hasVerticalScrollbar != (m_verticalScrollbar != 0)) {
        // FIXME: Is frameRectsChanged really necessary here? Have any frame rects changed?
        frameRectsChanged();
        positionScrollbarLayers();
        updateScrollCorner();
Comment 3 James Robinson 2013-01-29 21:14:16 PST
Created attachment 185387 [details]
Patch
Comment 4 James Robinson 2013-01-29 21:15:12 PST
I've had no luck creating a layout test.  The problem only seems to manifest on a Widget resize and I can't make it happen inside an iframe.

I am not 100% convinced this is the right patch but it fixes this bug and does not regress any other layout tests, so it's something for consideration.
Comment 5 James Robinson 2013-01-29 21:45:58 PST
Comment on attachment 185387 [details]
Patch

After staring at it a bit more I'm pretty sure this is correct.  This should only cause us to do the update in more cases than we currently do, never less, and repositioning/etc more often should never produce incorrect behavior.
Comment 6 WebKit Review Bot 2013-01-29 22:33:15 PST
Comment on attachment 185387 [details]
Patch

Clearing flags on attachment: 185387

Committed r141226: <http://trac.webkit.org/changeset/141226>
Comment 7 WebKit Review Bot 2013-01-29 22:33:18 PST
All reviewed patches have been landed.  Closing bug.