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

Issue 665412 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Apr 2017
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows
Pri: 2
Type: Bug



Sign in to add a comment

Focus rings not properly clipped by interior bounds of scroll view

Reported by jshan...@etouch.net, Nov 15 2016

Issue description

Chrome Version:56.0.2920.0 (Official Build) 378fc3fd49ebb678fc98cd99831b858fd304691f-refs/heads/master@{#432057}-32/64 bit
OS: Windows (7,8,8.1,10),Linux(Ubuntu 14.04 LTS)

Precondition: Enabled Material Design in the rest of the browser's native UI flag from chrome://flags.

Steps:
1. Launch Chrome, open NTP, right click on Bookmark bar and select 'Add folder' option
2. Click on 'New folder' button multiple time such that 12-14 folder is added and scroll bar appears.
3. Observe the focus ring of 'New folder' text box.

Actual: Focus ring of 'New folder' text box overlaps the scroll bar in 'New folder' overlay.

Expected: No such overlapping should be seen.

This is a regression issue broken in M-55, will soon update the bisect info:

Good Build: 55.0.2880.0
Bad Build: 55.0.2881.0

Note: Above issue is not seen on Mac OS.

 
Actual_result.jpg
106 KB View Download
Expected_result.jpg
107 KB View Download
Labels: Needs-Bisect
Status: Untriaged (was: Unconfirmed)

Comment 2 by jshan...@etouch.net, Nov 16 2016

Labels: -Needs-Bisect hasbisect
Owner: est...@chromium.org
Status: Assigned (was: Untriaged)
Narrow bisect:
https://chromium.googlesource.com/chromium/src/+log/5379f1724fc3c193828e137faba13c8d59c3f228..1aceebcec9263879acc6610e69d9350b34380a31?pretty=fuller&n=100

Suspecting: r422921 ?

Please help to re-assign if your change is not the cause for this issue.

Comment 3 by est...@chromium.org, Nov 16 2016

Labels: -Pri-1 -Proj-MaterialDesign-WebUI -M-55 Proj-MaterialDesign-NativeUI Pri-3
Owner: shrike@chromium.org
> Precondition: Enabled Material Design in the rest of the browser's native UI flag from chrome://flags.

please stop marking bugs for features that are not enabled as p1 regressions.

Comment 4 by shrike@chromium.org, Nov 16 2016

Cc: shrike@chromium.org
Labels: Proj-HarmonyControls
Owner: pkasting@chromium.org
pkasting@ - would you please help fix this regression?
Cc: pkasting@chromium.org
Owner: est...@chromium.org
It seems like the suspected CL in comment 2 is a good candidate for this, so maybe estade is the right owner.  (So far I have yet to do any Harmony work, which bodes ill for my prospects of investigating this...)

Comment 6 by est...@chromium.org, Nov 17 2016

Owner: shrike@chromium.org
Indeed, I'm sure this is due to my code, but I'm no longer working on Harmony.
Cc: -pkasting@chromium.org
Owner: pkasting@chromium.org
I see :/

I guess I'll take the assignment, but who knows when I will ever get to Harmony work.  Sigh.

Comment 8 by shrike@chromium.org, Nov 17 2016

Cc: pkasting@chromium.org
pkasting@ - there's nothing special about Harmony code. This is simply a bug in estade@'s Views code that needs to be fixed.

Comment 9 by shrike@chromium.org, Nov 17 2016

Also, jschuh@ says that you are available to work on Harmony.
I do intend to (someday) get to the Harmony work on my plate, but consider me unavailable for anything beyond this until further notice.
Cc: -nyerramilli@chromium.org -pkasting@chromium.org -msrchandra@chromium.org -ranjitkan@chromium.org -shrike@chromium.org
Labels: -hasbisect -Pri-3 Pri-2
Owner: ----
Status: Available (was: Assigned)
Summary: Focus rings not properly clipped by interior bounds of scroll view (was: Regression: Focus ring of 'New folder' text box overlaps the scroll bars in 'New folder' bookmark overlay.)
Wondering if this is an issue on MacViews
Labels: -Type-Bug-Regression Type-Bug
Owner: iyengar@chromium.org
Status: Assigned (was: Available)
Ananta, you want to take a look at this?
Owner: ananta@chromium.org
Status: Started (was: Assigned)
Project Member

Comment 16 by bugdroid1@chromium.org, Apr 12 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7720633b71d3f5d8d3c912b93d2466a0b2972c24

commit 7720633b71d3f5d8d3c912b93d2466a0b2972c24
Author: ananta <ananta@chromium.org>
Date: Wed Apr 12 03:48:16 2017

Fix an issue with the bookmarks bar in Harmony where in the focus ring would paint outside the scrollview.

The proposed fix here is to scroll the parent scrollview by the row bounds to ensure that the ring does not
paint outside the content area. There is an underlying problem here with the focus ring not getting clipped
when it tries to paint outside the parent.

The correct fix there is to notify the parents about the layer being used the focus ring which would require
them to create layers to host the child layer. Clipping can be done then.

The plan is to work on that in a subsequent patch.

BUG= 665412 

Review-Url: https://codereview.chromium.org/2813103002
Cr-Commit-Position: refs/heads/master@{#463923}

[modify] https://crrev.com/7720633b71d3f5d8d3c912b93d2466a0b2972c24/ui/views/controls/tree/tree_view.cc

Project Member

Comment 17 by bugdroid1@chromium.org, Apr 19 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5c05d7143480feda606f6425f78f3b18598f9310

commit 5c05d7143480feda606f6425f78f3b18598f9310
Author: ananta <ananta@chromium.org>
Date: Wed Apr 19 20:00:40 2017

Ensure that the focus ring in the bookmarks bar does not paint outside the parent view.

The proposed fix is to notify the parent views when a child enables layering. The parent in
this case the ScrollView overrides the newly added notification OnChildLayerChanged() and enables
viewport layering. This ensures that the ring gets clipped.

Longer term it seems like the focus ring should really be a property of the view and should not
be instantiated by different controls all over the place.

That for a later patchset.

BUG= 665412 , 656198
TEST=Covered by test ViewObserverTest.ScrollViewChildAddLayerTest and ViewObserverTest.ChildViewLayerNotificationTest

Review-Url: https://codereview.chromium.org/2813353002
Cr-Commit-Position: refs/heads/master@{#465720}

[modify] https://crrev.com/5c05d7143480feda606f6425f78f3b18598f9310/ui/views/controls/scroll_view.cc
[modify] https://crrev.com/5c05d7143480feda606f6425f78f3b18598f9310/ui/views/controls/scroll_view.h
[modify] https://crrev.com/5c05d7143480feda606f6425f78f3b18598f9310/ui/views/view.cc
[modify] https://crrev.com/5c05d7143480feda606f6425f78f3b18598f9310/ui/views/view.h
[modify] https://crrev.com/5c05d7143480feda606f6425f78f3b18598f9310/ui/views/view_unittest.cc

Status: Fixed (was: Started)

Sign in to add a comment