108255 – Scrollbar and scroll corner composited layers positioned incorrectly

RESOLVED FIXED 108255

Scrollbar and scroll corner composited layers positioned incorrectly

https://bugs.webkit.org/show_bug.cgi?id=108255

Summary Scrollbar and scroll corner composited layers positioned incorrectly

James Robinson

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

Attachments
Patch (2.37 KB, patch) 2013-01-29 21:14 PST, James Robinson	no flags	Details Formatted Diff Diff
View All Add attachment proposed patch, testcase, etc.

James Robinson

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

James Robinson

Comment 2 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();

James Robinson

Comment 3 2013-01-29 21:14:16 PST

Created attachment 185387 [details] Patch

James Robinson

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

James Robinson

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

WebKit Review Bot

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

WebKit Review Bot

Comment 7 2013-01-29 22:33:18 PST

All reviewed patches have been landed. Closing bug.

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

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware Unspecified

OS Unspecified

Product WebKit

Component New Bugs

Assignee

James Robinson

Reported

2013-01-29 16:50 PST

Modified

2013-01-29 22:33 PST History

CC List

4 users Show

URL

Keywords

Depends on

Blocks