Summary: | Unnecessary horizontal scrollbar in Gmail under Chromium. ScrollView::updateScrollbars uses stale value for off/on/auto scrollbar mode. | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mark Mentovai <mark> | ||||||||||
Component: | Frames | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | dglazkov, eric, hyatt, me, playmobil, sam, tonikitoo, xan.lopez | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Other | ||||||||||||
OS: | OS X 10.5 | ||||||||||||
Attachments: |
|
Description
Mark Mentovai
2009-08-21 09:32:40 PDT
Created attachment 38376 [details]
Perform layout before checking scrollbar modes
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.
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. Ping? 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.
(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. 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. 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. Comment on attachment 38436 [details]
Perform layout before checking scrollbar modes
r=me
Landed as http://trac.webkit.org/changeset/47904. 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. Reverted http://trac.webkit.org/changeset/47909. 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). You can control preferences in DRT tests via: layoutTestController.overridePreference("WebKitJavaScriptEnabled", false); see examples in: LayoutTests/fast/harness Comment on attachment 38436 [details]
Perform layout before checking scrollbar modes
Clearing hyatt's r+ since there are newer patches.
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.
Comment on attachment 39036 [details]
v4, Eliminate duplicated (and incorrect) scrollbar mode tracking between FrameView and ScrollView
r=me
|