Bug 175235 – Layout viewport rect is too wide after window resize

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Ali Juma]

When using non-overlay scrollbars, ScrollView::updateScrollbars can reach its cMaxUpdateScrollbarsPass recursion limit, leaving layout in a state that's inconsistent wrt the presence of scrollbars.

For example, starting with a page that's vertically scrollable, resize the window to reduce its width. Here's what happens in updateScrollbars:
1) The totalContentsSize() is wider than the visibleWidth(), so a horizontal scrollbar is added. This triggers another layout and a recursive call into updateScrollbars.
2) Layout changes the totalContentsSize() so that is no longer wider than the visible width. So the horizontal scrollbar needs to be removed. But updateScrollbars always removes both scrollbars when one needs to be removed, so the vertical scrollbar is removed as well. Another layout and recursive call into updateScrollbars happens.
3) Layout changes the width of totalContentsSize() under the assumption that there's no vertical scrollbar. But totalContentsSize() is still taller than visibleHeight(), so the vertical scrollbar is added back. We need to trigger another layout to account for this, but we can't since m_updateScrollbarsPass is already 2 (that is, cMaxUpdateScrollbarsPass).

So we're left with the layout that was done under the assumption of no vertical scrollbar, even though we do have a vertical scrollbar. And that means the layout viewport is wider than it should be, by the width of the scrollbar. It does get the right size the next time something triggers layout.

One way to fix this is to not remove the vertical scrollbar in step 2. Then there won't be any scrollbar change in step 3. Alternatively, we could increase cMaxUpdateScrollbarsPass.

[Ali Juma]

Created attachment 317323 [details]
Patch

[Ali Juma]

Created attachment 318526 [details]
Patch

Rebased

[Frédéric Wang (:fredw)]

I'm not really familiar with this code but that lgtm. I'm not sure what was the rationale for the "always disable the other bar when removing one bar", maybe someone involved in https://trac.webkit.org/changeset/42334 should confirm it's too strong.

[Simon Fraser (smfr)]

This is "highly tuned" code and I suspect that the current logic is there for a good reason. Please talk to Dave Hyatt about this.

[Dave Hyatt]

This should be ok.

[WebKit Commit Bot]

Comment on attachment 318526 [details]
Patch

Rejecting attachment 318526 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 318526, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
 3.
patching file LayoutTests/fast/dom/Window/window-resize-update-scrollbars-expected.txt
patching file LayoutTests/fast/dom/Window/window-resize-update-scrollbars.html
patching file LayoutTests/platform/ios/TestExpectations
Hunk #1 FAILED at 1896.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/ios/TestExpectations.rej

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'David Hyatt']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.webkit.org/results/5052531

[Ali Juma]

Created attachment 325478 [details]
Patch for landing

Rebased.

[WebKit Commit Bot]

Comment on attachment 325478 [details]
Patch for landing

Clearing flags on attachment: 325478

Committed r224241: <https://trac.webkit.org/changeset/224241>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 325478 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=325478&action=review

> Source/WebCore/platform/ScrollView.cpp:618
> +            if (!newHasHorizontalScrollbar && hasHorizontalScrollbar && vScroll != ScrollbarAlwaysOn &&!hasVerticalScrollbar) {

Formatting mistake, missing space after "&&".

[Ali Juma]

Reopening to attach new patch.

[Ali Juma]

Created attachment 325913 [details]
Formatting fix

Oops, here's a fix for the missing space.

[WebKit Commit Bot]

Comment on attachment 325913 [details]
Formatting fix

Clearing flags on attachment: 325913

Committed r224409: <https://trac.webkit.org/changeset/224409>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35569029>