[root layer scrolls] should preferred size include scrollbars? |
||||
Issue descriptionSpinoff of http://crrev.com/c/758480 review thread. I suspect RLS changes the behavior of WebView::ContentsPreferredMinimumSize() with respect to inclusion / exclusion of main frame scrollbars. Furthermore I suspect that this behavior is untested, because the preferred size tests in render_view_browsertest.cc call SetCanHaveScrollbars(false). Those tests currently fail with RLS ( issue 778084 ), but for a different reason (SetCanHaveScrollbars not triggering relayout). We should verify that RLS matches non-RLS behavior for the following things when the main frame has a non-overlay scrollbar: - Mac [+] button (see repro steps in http://crbug.com/440824#c2 ) - extension popups (see repro steps in http://crbug.com/734085 )
,
Jan 9 2018
,
Jan 9 2018
[Noting for future reference:] In addition to tracking RLS compat for the observable behavior effects of WebView::ContentsPreferredMinimumSize, this bug tracks fixing the following tests: RenderViewImplTest.PreferredSizeZoomed RenderViewImplScaleFactorTest.PreferredSizeWithScaleFactor These tests currently fail in the content_browsertests step on five trybots: https://ci.chromium.org/buildbot/tryserver.chromium.linux/cast_shell_linux/517283 https://ci.chromium.org/buildbot/tryserver.chromium.linux/linux_chromium_asan_rel_ng/521864 https://ci.chromium.org/buildbot/tryserver.chromium.linux/linux_chromium_rel_ng/620275 https://ci.chromium.org/buildbot/tryserver.chromium.linux/linux_chromium_tsan_rel_ng/227446 https://ci.chromium.org/buildbot/tryserver.chromium.mac/mac_chromium_rel_ng/623932 On linux_chromium_rel_ng it also fails in related recipe steps: network_service_content_browsertests site_per_process_content_browsertests viz_content_browsertests
,
Jan 18 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/47fe541fa72d49cd9b2fca01b0ca677505850d9a commit 47fe541fa72d49cd9b2fca01b0ca677505850d9a Author: Steve Kobes <skobes@chromium.org> Date: Thu Jan 18 19:13:06 2018 Include FrameView scrollbar in WebView::ContentsPreferredMinimumSize. This improves accuracy of Mac [+] (window maximize) and extension options popup size, and brings non-RLS behavior in sync with RLS. Bug: 785088 Change-Id: I4a77aef709f9875a3a0e0692a9290572aa8904f1 Reviewed-on: https://chromium-review.googlesource.com/867518 Reviewed-by: Robert Sesek <rsesek@chromium.org> Reviewed-by: John Abd-El-Malek <jam@chromium.org> Reviewed-by: David Bokan <bokan@chromium.org> Commit-Queue: Steve Kobes <skobes@chromium.org> Cr-Commit-Position: refs/heads/master@{#530222} [modify] https://crrev.com/47fe541fa72d49cd9b2fca01b0ca677505850d9a/chrome/browser/ui/cocoa/browser_window_controller.mm [modify] https://crrev.com/47fe541fa72d49cd9b2fca01b0ca677505850d9a/content/renderer/render_view_browsertest.cc [modify] https://crrev.com/47fe541fa72d49cd9b2fca01b0ca677505850d9a/third_party/WebKit/Source/core/exported/WebViewImpl.cpp
,
Jan 18 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/bee67a7f4af33c563c9f701552d0bbf72ac84314 commit bee67a7f4af33c563c9f701552d0bbf72ac84314 Author: Sorin Jianu <sorin@chromium.org> Date: Thu Jan 18 20:56:18 2018 Revert "Include FrameView scrollbar in WebView::ContentsPreferredMinimumSize." This reverts commit 47fe541fa72d49cd9b2fca01b0ca677505850d9a. Reason for revert: it breaks some tests such as: Retrying 1 test (retry #1) [ RUN ] RenderViewImplTest.PreferredSizeZoomed ../../content/renderer/render_view_browsertest.cc:2186: Failure Expected equality of these values: gfx::Size(400 + scrollbar_width, 400) Which is: 420x400 size Which is: 415x400 [ FAILED ] RenderViewImplTest.PreferredSizeZoomed (68 ms) [2252/2252] RenderViewImplTest.PreferredSizeZoomed (127 ms) Retrying 1 test (retry #2) [ RUN ] RenderViewImplTest.PreferredSizeZoomed ../../content/renderer/render_view_browsertest.cc:2186: Failure Expected equality of these values: gfx::Size(400 + scrollbar_width, 400) Which is: 420x400 size Which is: 415x400 [ FAILED ] RenderViewImplTest.PreferredSizeZoomed (67 ms) [2253/2253] RenderViewImplTest.PreferredSizeZoomed (123 ms) Retrying 1 test (retry #3) [ RUN ] RenderViewImplTest.PreferredSizeZoomed ../../content/renderer/render_view_browsertest.cc:2186: Failure Expected equality of these values: gfx::Size(400 + scrollbar_width, 400) Which is: 420x400 size Which is: 415x400 [ FAILED ] RenderViewImplTest.PreferredSizeZoomed (66 ms) [2254/2254] RenderViewImplTest.PreferredSizeZoomed (123 ms) 1 test failed: RenderViewImplTest.PreferredSizeZoomed (../../content/renderer/render_view_browsertest.cc:2170) https://logs.chromium.org/v/?s=chromium%2Fbb%2Fchromium.mac%2FMac10.9_Tests%2F52896%2F%2B%2Frecipes%2Fsteps%2Fcontent_browsertests%2F0%2Fstdout Original change's description: > Include FrameView scrollbar in WebView::ContentsPreferredMinimumSize. > > This improves accuracy of Mac [+] (window maximize) and extension > options popup size, and brings non-RLS behavior in sync with RLS. > > Bug: 785088 > Change-Id: I4a77aef709f9875a3a0e0692a9290572aa8904f1 > Reviewed-on: https://chromium-review.googlesource.com/867518 > Reviewed-by: Robert Sesek <rsesek@chromium.org> > Reviewed-by: John Abd-El-Malek <jam@chromium.org> > Reviewed-by: David Bokan <bokan@chromium.org> > Commit-Queue: Steve Kobes <skobes@chromium.org> > Cr-Commit-Position: refs/heads/master@{#530222} TBR=jam@chromium.org,bokan@chromium.org,skobes@chromium.org,rsesek@chromium.org Change-Id: I69aec20ccfe488729a83f3944cfab12288332b37 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 785088 Reviewed-on: https://chromium-review.googlesource.com/874601 Reviewed-by: Sorin Jianu <sorin@chromium.org> Commit-Queue: Sorin Jianu <sorin@chromium.org> Cr-Commit-Position: refs/heads/master@{#530265} [modify] https://crrev.com/bee67a7f4af33c563c9f701552d0bbf72ac84314/chrome/browser/ui/cocoa/browser_window_controller.mm [modify] https://crrev.com/bee67a7f4af33c563c9f701552d0bbf72ac84314/content/renderer/render_view_browsertest.cc [modify] https://crrev.com/bee67a7f4af33c563c9f701552d0bbf72ac84314/third_party/WebKit/Source/core/exported/WebViewImpl.cpp
,
Jan 22 2018
I've been unable to locally reproduce the failures on the Mac waterfall bots that began after r530222 landed. The failures were flaky, and the failure mode seems to be that the scrollbar reports native "thickness" instead of the custom thickness specified in ::-webkit-scrollbar styling. This suggests some race condition in the application of custom scrollbar styles, or in the reporting of preferred size. To further isolate I am thinking of landing a speculative patch that directly queries the rect and type (native vs. custom) of the scrollbar in the test, after the call to LoadHTML. @bokan / @szager, let me know if you have any better ideas.
,
Jan 22 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/48616a6d014c0a64553d6ed6d3f52277763bf831 commit 48616a6d014c0a64553d6ed6d3f52277763bf831 Author: Steve Kobes <skobes@chromium.org> Date: Mon Jan 22 19:45:57 2018 Allow scrollbars in preferred size tests. This is the first step in trying to reland http://crrev.com/530222. Bug: 785088 Change-Id: I6240d13978a9756973ebc393d9e415fc37f5caaf Reviewed-on: https://chromium-review.googlesource.com/879121 Reviewed-by: Avi Drissman <avi@chromium.org> Reviewed-by: David Bokan <bokan@chromium.org> Commit-Queue: Steve Kobes <skobes@chromium.org> Cr-Commit-Position: refs/heads/master@{#530956} [modify] https://crrev.com/48616a6d014c0a64553d6ed6d3f52277763bf831/content/renderer/render_view_browsertest.cc
,
Jan 24 2018
Update: I discovered we can measure the width of the native scrollbar from the content layer by querying WebFrame::VisibleContentRect. That lets us get these tests working with RLS and punt on the custom scrollbar issue (which is unrelated to RLS, preferred width, or the original implementation of the tests in question).
,
Jan 24 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2d918b85c5a2253ae200c3aa6d8c7c188ef9df2a commit 2d918b85c5a2253ae200c3aa6d8c7c188ef9df2a Author: Steve Kobes <skobes@chromium.org> Date: Wed Jan 24 18:59:03 2018 [Reland] Include FrameView scrollbar in ContentsPreferredMinimumSize. This improves accuracy of Mac [+] (window maximize) and extension options popup size, and brings non-RLS behavior in sync with RLS. This relands r530222, reverted at r530265, and updates the test to avoid custom scrollbar styles. Bug: 785088 Change-Id: I821d1a79cbfb0173a49afd1ce06208cb890ef0c7 Reviewed-on: https://chromium-review.googlesource.com/874606 Reviewed-by: Stefan Zager <szager@chromium.org> Reviewed-by: Avi Drissman <avi@chromium.org> Commit-Queue: Steve Kobes <skobes@chromium.org> Cr-Commit-Position: refs/heads/master@{#531629} [modify] https://crrev.com/2d918b85c5a2253ae200c3aa6d8c7c188ef9df2a/chrome/browser/ui/cocoa/browser_window_controller.mm [modify] https://crrev.com/2d918b85c5a2253ae200c3aa6d8c7c188ef9df2a/content/renderer/render_view_browsertest.cc [modify] https://crrev.com/2d918b85c5a2253ae200c3aa6d8c7c188ef9df2a/third_party/WebKit/Source/core/exported/WebViewImpl.cpp
,
Jan 24 2018
,
Jan 25 2018
Hmm, these tests are still failing with RLS.
,
Jan 26 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a1dceecc377acf195a2794851a9340279faf0cf1 commit a1dceecc377acf195a2794851a9340279faf0cf1 Author: Steve Kobes <skobes@chromium.org> Date: Fri Jan 26 20:06:55 2018 Update WebFrame::VisibleContentRect for RLS. TESTED=RenderViewImplTest.PreferredSizeZoomed passes with RLS Bug: 785088 Change-Id: Ibce3e168a7b777b7ee4a51593177d6cbc162ec87 Reviewed-on: https://chromium-review.googlesource.com/886837 Reviewed-by: David Bokan <bokan@chromium.org> Commit-Queue: Steve Kobes <skobes@chromium.org> Cr-Commit-Position: refs/heads/master@{#532048} [modify] https://crrev.com/a1dceecc377acf195a2794851a9340279faf0cf1/third_party/WebKit/Source/core/frame/WebLocalFrameImpl.cpp
,
Jan 26 2018
The content_browsertests step now passes with RLS. https://ci.chromium.org/buildbot/tryserver.chromium.linux/linux_chromium_rel_ng/633286 |
||||
►
Sign in to add a comment |
||||
Comment 1 by skobes@chromium.org
, Jan 8 2018Owner: skobes@chromium.org
Status: Started (was: Available)
Update: Pre-RLS, WebView::ContentsPreferredMinimumSize never included main frame scrollbars. For Mac maximize [+] button (replaced by fullscreen button in Yosemite but still accessible by holding Option), the -windowWillUseStandardFrame function in browser_window_controller.mm adds 16px to the WebView's preferred width to allow for a vertical scrollbar (even if it's overlay, or absent entirely): const int kScrollbarWidth = 16; // TODO(viettrungluu): ugh. For extension options dialogs, we use WebView::ContentsPreferredMinimumSize with no further adjustment for scrollbars. This is suboptimal for options dialogs with non-overlay vertical scrollbars. With RLS, WebView::ContentsPreferredMinimumSize will include the width of the main frame vertical scrollbar, if there is one, and it accounts for overlay vs. non-overlay correctly. This means RLS actually improves behavior for extension options dialogs, and would give us the right behavior for maximize if browser_window_controller.mm weren't adding 16px to it. I think ideally we would just skip the 16px hack when RLS is on, and make RenderViewImplTest.PreferredSizeZoomed and RenderViewImplScaleFactorTest.PreferredSizeWithScaleFactor verify the RLS behavior. Alternatively, if we wanted to be strictly behavior-preserving, we could make WebView::ContentsPreferredMinimumSize exclude the LayoutView's scrollbars with RLS. Regardless, I think it's better for the tests to exercise the scrollbars-enabled path, instead of calling SetCanHaveScrollbars(false).310 KB
310 KB View Download
304 KB
304 KB View Download