Bug 28614 – Unnecessary horizontal scrollbar in Gmail under Chromium. ScrollView::updateScrollbars uses stale value for off/on/auto scrollbar mode.

• Component: Frames
• Status: RESOLVED FIXED

[Mark Mentovai]

While using Gmail in Chromium on Mac and Linux, WebKit shows a horizontal scrollbar when it shouldn't.

http://crbug.com/7976
http://crbug.com/14647

Assuming that the window is wide enough to not need a horizontal scrollbar in Gmail, and assuming that Chromium doesn't pop open any info bars or anything that change the size of the view causing a relayout:

The document width is remaining at the frame width when the page is loaded, even though the appearance of the vertical scrollbar should cause the document width to be constrained to the visible width.  This causes a horizontal scrollbar to appear.

When the window is resized, the horizontal scrollbar disappears (assuming that the visible width is at least the page's minimum width).

ScrollView::updateScrollbars is using stale data for the scrollbar modes, hScroll and vScroll, by setting them prior to performing the initial layout.  In the case of Gmail, the vertical scrollbar mode is ScrollbarAuto prior to layout, but after the call to visibleContentsResized(), it changes to ScrollbarAlwaysOn.  However, for the remainder of updateScrollbars, even after visibleContentsResized() returns, the stale cached value of ScrollbarAuto will be used.  This results in no vertical scrollbar being set during this initial run of updateScrollbars, because the document size at that point matches the frame size exactly.

The above describes the underlying bug.  The appearance of the horizontal scrollbar is actually a symptom of this bug in conjunction with peculiarities of Gmail.  With no vertical scrollbar, Gmail will remember the visible width as the width of the frame without the scrollbar, and will adjust itself to fill that space.  The next time updateScrollbars is reached, a vertical scrollbar will be enabled (owing to its being set to ScrollbarAlwaysOn), shrinking the actual visible width, but Gmail will continue to use the original value.  When updateScrollbars recurses back into itself, a horizontal scrollbar will be enabled: the horizontal scrollbar is set to ScrollbarAuto, and the document width is now narrower than the visible width, because the vertical scrollbar is present.

I have a patch, which I'll attach to this bug shortly.

[Mark Mentovai]

Created attachment 38376 [details]
Perform layout before checking scrollbar modes

[Dave Hyatt]

Comment on attachment 38376 [details]
Perform layout before checking scrollbar modes

I am not convinced this is right.  You aren't supposed to just always invoke visibleContentsResized.

Also note that any change to this logic has to be replicated in WebDynamicScrollbarsView on Mac to keep the two in sync.

Finally I see no reason to change the word "recur" to "recurse."  They mean exactly the same thing, and changing around English just because you don't like it pisses me off.

[Mark Mentovai]

Created attachment 38436 [details]
Perform layout before checking scrollbar modes

> I am not convinced this is right.  You aren't supposed to just always invoke
> visibleContentsResized.

The existing code always calls visibleContentsResized, though.  I'm just moving the call up earlier to avoid using stale data for the off/on/auto value.

There's one additional behavior change.  The existing code avoids visibleContentsResized if if both the horizontal and vertical scrollbars are forced on or off.  With the proposed patch, this is not taken into account.  I considered writing the patch to respect these values, but felt that would be unwise.  The bug as it relates to Gmail involves the vertical scrollbar mode being changed from auto to forced-on during the call to visibleContentsResized.  It seems just as plausible for forced-on/off scrollbars to change back to auto in that call.

> Also note that any change to this logic has to be replicated in
> WebDynamicScrollbarsView on Mac to keep the two in sync.

This version of the patch takes that into account.  It also moves a couple of minor things around, with no functional changes, to keep it synchronized with ScrollView::updateScrollbars.

> Finally I see no reason to change the word "recur" to "recurse."  They mean
> exactly the same thing, and changing around English just because you don't like
> it pisses me off.

OK.  It should be pretty clear that I didn't intend to piss anyone off.

I've run this through the layout test suite and it doesn't introduce any regressions.  It fixes the Gmail bug in Chromium.

[Mark Mentovai]

Ping?

[Dave Hyatt]

Comment on attachment 38436 [details]
Perform layout before checking scrollbar modes

When both scrollbars are forced on, this is because of overflow:scroll.  That's why we checked that case... since you know you don't need to do a layout if both scrollbars are locked on.  That was my concern.  I don't think you want to cause a layout to happen in that case.

[Mark Mentovai]

(In reply to comment #5)
> When both scrollbars are forced on, this is because of overflow:scroll.  That's
> why we checked that case... since you know you don't need to do a layout if
> both scrollbars are locked on.  That was my concern.  I don't think you want to
> cause a layout to happen in that case.

Dave, I still think that there is still a reason that layout might be necessary in that case.  For example:

 - page has overflow:scroll
 - page is resized
 - page's JS responds to resize by changing the value of overflow

The only way to handle that case properly is to do the layout before even looking at the state of m_horizontalScrollbarMode and m_verticalScrollbarMode.  If you're worried about "too much layout," FrameView::visibleContentsResized should only do the layout if one is actually needed, and the common case is "auto" anyway and there doesn't seem to be "too much layout" with the code structured as-is.  I do think that the existing patch (v2) is the correct one.

I'm primarily concerned with fixing the horizontal scrollbar bug as it relates to Gmail, so if you really think that further limiting the conditions in which visibleContentsResized is called, I'm happy to change it.  I'll upload another patch in a moment, feel free to pick which one you prefer.

[Mark Mentovai]

Created attachment 38616 [details]
v3, only do early layout when at least one scrollbar is in "auto" mode

See comment 6.  I believe that attachment 38436 [details] is the correct approach here, but if you prefer this one, it should also fix the specific bug I'm working on right now too.

[Mark Mentovai]

Both v2 (attachment 38436 [details]) and v3 (attachment 38616 [details]) fix my specific Gmail bug (http://crbug.com/7976), and neither introduces any layout test regressions.

[Dave Hyatt]

Comment on attachment 38436 [details]
Perform layout before checking scrollbar modes

r=me

[Dimitri Glazkov (Google)]

Landed as http://trac.webkit.org/changeset/47904.

[Mark Mentovai]

This may have caused fast/dom/Window/new-window-opener.htm to fail on the Mac according to the buildbot.  It didn't locally, but my working copy is not completely up to date.  I'll investigate.

[Dimitri Glazkov (Google)]

Reverted http://trac.webkit.org/changeset/47909.

[Mark Mentovai]

The test that's failing, fast/dom/Window/new-window-opener.html, passes in "run-safari" when pop-up blocking is disabled.  The test depends on disabling the pop-up blocker.  It fails when run via DumpRenderTree ("run-webkit-tests"), but not because pop-ups are blocked (they aren't).

[Eric Seidel (no email)]

You can control preferences in DRT tests via:
        layoutTestController.overridePreference("WebKitJavaScriptEnabled", false);

see examples in:
LayoutTests/fast/harness

[Eric Seidel (no email)]

Comment on attachment 38436 [details]
Perform layout before checking scrollbar modes

Clearing hyatt's r+ since there are newer patches.

[Mark Mentovai]

Created attachment 39036 [details]
v4, Eliminate duplicated (and incorrect) scrollbar mode tracking between FrameView and ScrollView

I figured out why fast/dom/Window/new-window-opener.html was failing in DumpRenderTree on the Mac.  Both FrameView and its superclass ScrollView maintain members that track the off/on/auto state of each scrollbar.  In certain cases, they could become desynchronized.

The problem occurs when setCanHaveScrollbars is called.  In this layout test, that method was being called to turn off the scrollbars in response to a JavaScript window.open(..., "scrollbars=no").  In the existing code, FrameView::setCanHaveScrollbars first calls the superclass method ScrollView::setCanHaveScrollbars, and then updates its m_[hv]mode members used to track the scrollbar state.  Unfortunately (for me), ScrollView::setCanHaveScrollbars eventually enters FrameView::layout, which uses m_[hv]mode before they have been properly set.

I can't see any good reason to maintain the duplication between m_[hv]mode in FrameView and m_{horizontal|vertical}ScrollbarMode in ScrollView.  In this patch, I'm getting rid of the duplicated fields in FrameView, along with some extra bookkeeping methods and members that were supposed keep everything synchronized.

The ScrollView.cpp and WebDynamicScrollBarsView.mm parts of this patch already had r+ from hyatt in v2, and are unchanged in this version.

This produced no layout test regressions on the Mac in DumpRenderTree, or on any of Chromium's three platforms.

[Dave Hyatt]

Comment on attachment 39036 [details]
v4, Eliminate duplicated (and incorrect) scrollbar mode tracking between FrameView and ScrollView

r=me

[Mark Mentovai]

http://trac.webkit.org/changeset/48064