Bug 175235 - Layout viewport rect is too wide after window resize
Summary: Layout viewport rect is too wide after window resize
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ali Juma
Keywords: InRadar
Depends on: 12440
Blocks: 174362
  Show dependency treegraph
Reported: 2017-08-04 19:13 PDT by Ali Juma
Modified: 2017-11-15 13:13 PST (History)
10 users (show)

See Also:

Patch (6.18 KB, patch)
2017-08-04 19:25 PDT, Ali Juma
no flags Details | Formatted Diff | Diff
Patch (6.11 KB, patch)
2017-08-18 12:17 PDT, Ali Juma
no flags Details | Formatted Diff | Diff
Patch for landing (6.15 KB, patch)
2017-10-31 13:02 PDT, Ali Juma
no flags Details | Formatted Diff | Diff
Formatting fix (1.60 KB, patch)
2017-11-03 10:33 PDT, Ali Juma
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ali Juma 2017-08-04 19:13:11 PDT
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.
Comment 1 Ali Juma 2017-08-04 19:25:14 PDT Comment hidden (obsolete)
Comment 2 Ali Juma 2017-08-18 12:17:41 PDT
Created attachment 318526 [details]

Comment 3 Frédéric Wang (:fredw) 2017-10-12 10:31:56 PDT
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.
Comment 4 Simon Fraser (smfr) 2017-10-12 10:35:07 PDT
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.
Comment 5 Dave Hyatt 2017-10-31 11:24:33 PDT
This should be ok.
Comment 6 WebKit Commit Bot 2017-10-31 11:26:28 PDT
Comment on attachment 318526 [details]

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:
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
Comment 7 Ali Juma 2017-10-31 13:02:22 PDT
Created attachment 325478 [details]
Patch for landing

Comment 8 WebKit Commit Bot 2017-10-31 13:27:22 PDT
Comment on attachment 325478 [details]
Patch for landing

Clearing flags on attachment: 325478

Committed r224241: <https://trac.webkit.org/changeset/224241>
Comment 9 WebKit Commit Bot 2017-10-31 13:27:23 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Darin Adler 2017-11-03 09:15:47 PDT
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 "&&".
Comment 11 Ali Juma 2017-11-03 10:33:05 PDT
Reopening to attach new patch.
Comment 12 Ali Juma 2017-11-03 10:33:06 PDT
Created attachment 325913 [details]
Formatting fix

Oops, here's a fix for the missing space.
Comment 13 WebKit Commit Bot 2017-11-03 11:32:22 PDT
Comment on attachment 325913 [details]
Formatting fix

Clearing flags on attachment: 325913

Committed r224409: <https://trac.webkit.org/changeset/224409>
Comment 14 WebKit Commit Bot 2017-11-03 11:32:24 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 Radar WebKit Bug Importer 2017-11-15 13:13:17 PST