Issue metadata
Sign in to add a comment
|
Regression :Unwanted scrollbar is seen on task manager overlay.
Reported by
pranjali...@etouch.net,
Oct 4 2017
|
||||||||||||||||||||||
Issue descriptionChrome Version:63.0.3232.0 (Official Build)61a8a7b24ebea8329a2aca7536b7dd881ecb2ea5-refs/heads/master@{#506256}(32/64 bit) OS:Windows (7,8,10),Linux (14.04 LTS). Steps to reproduce: 1.Launch chrome,Open NTP and Press Shift + Esc 2.Observe task manager overlay. Actual Result :Unwanted scrollbar is seen on task manager overlay. Expected Result :scrollbar should not be seen on task manager overlay. This is Regression issue broken in 'M-63' Good Build:63.0.3231.0 Bad Build:63.0.3232.0 Note:Will soon provide the bisect info.
,
Oct 4 2017
Tagging with blocker label, please undo if not the case.
,
Oct 10 2017
erikchen@, Friendly ping to get an update on this issue as it is marked as stable blocker. Thanks..!
,
Oct 11 2017
M63 is branching soon, we will be taking only critical merges. It would be great to have a fix ASAP.
,
Oct 12 2017
The CL in question has already been reverted. But I bet the problem is that the CL causes a horizontal scrollbar to appear, and I bet the vertical scrollbar appears whenever the horizontal scrollbar appears. We should probably adjust the default size to be slightly larger...although that doesn't fix the general problem.
,
Oct 13 2017
This also occurs on stable channel. All you have to do is add another column. I spent a while staring at TableView and ScrollView. The layout flow is very confusing. I'm pretty sure that this is a symptom of some deeper, underlying problems. The important function is ScrollView::ComputeScrollBarsVisibility. This compares content size to viewport size to determine which scrollbars need to be shown. Content size is computed from TableView::size(), and viewport size is computed from ScrollView::size(). TableView::size() is set in TableView::Layout(), and is std::max(preferred_size, ScrollView::contents_viewport_.size(). But ScrollView::contents_viewport_ is set from viewport bounds. Suspicious? Furthermore, the comment where contents_viewport_ is set says: """ // Assumes a vertical scrollbar since most of the current views are designed // for this. ... viewport_bounds.set_width(viewport_bounds.width() - vert_sb_layout_width); """ Commenting out the line fixes the problem, but presumably induces other changes.
,
Oct 13 2017
The crux of the problem is that: 1) The size of the contents view depends on whether scrollbars are present. 2) Whether scrollbars are present is dependent on the size of the content view. In this case: the content view sets its height to the viewport's height. But the content's width is wider than the viewport's width. This causes the scrollview to add a horizontal scrollbar. But now horizontal scrollbar + content's height > viewport's height, so a vertical scrollbar is also necessary. A full fix to this problem would require a large refactor to avoid layout-dependency cycles. Instead, I will extend the existing fix by doing the following: 3) Prior to laying-out the contents view, the viewport's size is artificially shrunk to include room for both a horizontal and vertical scrollbar. [previously, the logic only made room for a vertical scrollbar]. 4) This way, when the contents increases its size to the viewport's size, there's still room for both scrollbars - if necessary. This in turn means that the scrollbars *don't* have to appear.
,
Oct 16 2017
I think there's always some kind of chicken-and-egg problem with this kind of stuff. I filed Issue 630516 a while ago. But it's pretty low priority, just because there are so few dialogs in Chrome's that want to scroll horizontally. But be aware that some dialogs (e.g. extension install) want to size their *window* height based on the minimum-sized scroll view that can hold the content without scrolling (so long as the content "fits" in some threshold). E.g. extensions with lots of permission requirements scroll, but those with less than some threshold get a scrollview that fits perfectly and never needs to scroll (unless, perhaps, if a `Details` expander is clicked). Window height needs to be determined before layout, then layout fits to the given window. This may complicate things.. but maybe if the artificial-shrinking is restricted just to ScrollViews that want a horizontal scroll bar there won't be knock-on effects.
,
Oct 17 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4e94f6984be1fe602353192d74a1e41d133e7cc2 commit 4e94f6984be1fe602353192d74a1e41d133e7cc2 Author: erikchen <erikchen@chromium.org> Date: Tue Oct 17 07:11:13 2017 Prevent vertical scrollbar from unnecessarily appearing. The existing code for views::ScrollView and views::TableView contains a layout-dependency cycle. * The size of the TableView depends on whether scrollbars are present. * Whether scrollbars are present is dependent on the size of the TableView. This causes the following problem: The TableView sets its height to ScrollView::content_viewport_'s height. But the TableView's width is wider than the viewport's width. This causes the ScrollView to add a horizontal scrollbar. But now horizontal scrollbar + content's height > viewport's height, so a vertical scrollbar is also necessary. A full fix to this problem would require a large refactor to avoid layout-dependency cycles. Instead, I will extend an existing workaround. Prior to invoking layout for the TableView, the contents_viewport_ will be artificially shrunk to make space for both a vertical and horizontal scrollbar. Previously, it was only shrunk to make space for a vertical scrollbar. This way, if the contents chooses to take up the full height, there's enough space for a horizontal scrollbar to appear without causing a vertical scrollbar to appear. Bug: 771502 Change-Id: Ib97ddd8f5619d16342ee85e7ad280c049a1566e5 Reviewed-on: https://chromium-review.googlesource.com/719380 Commit-Queue: Erik Chen <erikchen@chromium.org> Reviewed-by: Scott Violet <sky@chromium.org> Cr-Commit-Position: refs/heads/master@{#509312} [modify] https://crrev.com/4e94f6984be1fe602353192d74a1e41d133e7cc2/ui/views/controls/scroll_view.cc [modify] https://crrev.com/4e94f6984be1fe602353192d74a1e41d133e7cc2/ui/views/controls/scroll_view_unittest.cc
,
Oct 24 2017
Tested this issue on Windows 7 & Ubuntu 14.04 using chrome dev-63.0.3239.9 & latest Canary-64.0.3247.0 as per C#0. No scroll bar is seen on task manager overlay on NTP for the clean profile. Please find the attached screencast for reference. erikchen@,Could you please merge this code back to M63 as M63 branched at 3239. Thanks in advance..!
,
Oct 24 2017
The CL in question was already reverted from M-63. |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by pranjali...@etouch.net
, Oct 4 2017Owner: erikc...@chromium.org
Status: Assigned (was: Unconfirmed)