New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 771502 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Oct 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows
Pri: 1
Type: Bug-Regression



Sign in to add a comment

Regression :Unwanted scrollbar is seen on task manager overlay.

Reported by pranjali...@etouch.net, Oct 4 2017

Issue description

Chrome 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.







 
Actual_result.mp4
297 KB View Download
Expected_result.mp4
250 KB View Download
Labels: hasbisect-per-revision
Owner: erikc...@chromium.org
Status: Assigned (was: Unconfirmed)
You are probably looking for a change made after 506219 (known good), but no later than 506220 (first known bad).

CHANGELOG URL:

The script might not always return single CL as suspect as some perf builds might get missing due to failure.

  https://chromium.googlesource.com/chromium/src/+log/3f8dcf3a5016b8e02ab4c2ae81d298d0548d1b64..4925a99f1f1c95509e319a4a86bd0ff63d0a0a6f

Suspect:https://chromium.googlesource.com/chromium/src/+/4925a99f1f1c95509e319a4a86bd0ff63d0a0a6f
Labels: ReleaseBlock-Stable
Tagging with blocker label, please undo if not the case.
erikchen@,
Friendly ping to get an update on this issue as it is marked as stable blocker.
Thanks..!
M63 is branching soon, we will be taking only critical merges. It would be great to have a fix ASAP.
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.
Cc: tapted@chromium.org
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.
Screen Shot 2017-10-13 at 11.08.29 AM.png
86.7 KB View Download
Cc: sky@chromium.org
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.


Comment 8 by tapted@chromium.org, 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.
Project Member

Comment 9 by bugdroid1@chromium.org, 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

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..!
771502.mp4
569 KB View Download
Labels: -ReleaseBlock-Stable -M-63
Status: Fixed (was: Assigned)
The CL in question was already reverted from M-63.

Sign in to add a comment