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

Issue 702489 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: Apr 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug-Regression



Sign in to add a comment

Regression: Unnecessary blue focus is seen on Task manager Overlay

Project Member Reported by sandeepkumars@chromium.org, Mar 17 2017

Issue description

Chrome Version:  59.0.3041.0/9373.0.0 dev-channel Parrot, Banjo, Peach-Pit
OS: Chrome

What steps will reproduce the problem?
(1)Sign in to User>> launch Browser>> Click search + Esc to open Task manager>> and observe

Expected: Unnecessary blue focus should not be seen enabled by default
Actual: Instead Unnecessary blue focus is seen (Refer Screenshot)

This is a Regression issue as such issue is not seen in M56: 56.0.2924.110/9000.91.0 (Official Build) stable-channel Parrot
 
Actual_Task Manager.png
41.3 KB View Download
Expected.png
158 KB View Download
Owner: afakhry@chromium.org
Status: Assigned (was: Untriaged)
Cc: afakhry@chromium.org
Labels: Needs-Bisect
Owner: weidongg@chromium.org
Nothing was change in the task manager UI code that I'm aware of. It's probably a dependency regression.

A bisect is needed. Weidong, can you please handle this task?
Cc: abodenha@chromium.org dhadd...@chromium.org sdantul...@chromium.org abod...@chromium.org
 Issue 702313  has been merged into this issue.
Labels: M-58
reprod on M58-9334.16.0 too
I got the first bad revision. Here's the revision hash code f084da03407bcb848d8ef8f232c28db56883d648

Here's the commit message:

Change focus indicator for TableView and TreeView. 
   
   This affects surfaces such as the task manager, the bookmark bubble
   editor, and the cookies view.
   
   The indicator for selection (the background of the row) is unchanged.
   When the window is focused, instead of drawing a dashed focus rect
   which looks dated and especially bad at fractional scales, we draw the
   table/tree's border in blue. This matches textfields and the general
   trend of blue outlines/rings for focus indication.
   
   This will change in Harmony to be a focus ring so it's mainly just an
   interim solution that makes the current state of the world look less
   bad while we wait for Harmony to arrive.
Cc: est...@chromium.org
Owner: est...@chromium.org

Comment 8 by est...@chromium.org, Mar 23 2017

Status: Started (was: Assigned)
the behavior here is WAI but the visuals are an unintended consequence. I have a change here to address it: https://codereview.chromium.org/2769243002/
Project Member

Comment 9 by bugdroid1@chromium.org, Mar 27 2017

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

commit 2fc3ba8587526eb5e5f7455ca463b0c166b1ee95
Author: estade <estade@chromium.org>
Date: Mon Mar 27 18:02:00 2017

Adjust initial activation/focus Task Manager/Views TableView.

In the task manager the table no longer has initial focus. When you open
the task manager, you'll need to press tab to focus the table.

When a TableView is focused it will activate the first row if no row is already active.

BUG= 702489 

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

[modify] https://crrev.com/2fc3ba8587526eb5e5f7455ca463b0c166b1ee95/chrome/browser/ui/views/task_manager_view.cc
[modify] https://crrev.com/2fc3ba8587526eb5e5f7455ca463b0c166b1ee95/chrome/browser/ui/views/task_manager_view.h
[modify] https://crrev.com/2fc3ba8587526eb5e5f7455ca463b0c166b1ee95/ui/views/controls/table/table_view.cc
[modify] https://crrev.com/2fc3ba8587526eb5e5f7455ca463b0c166b1ee95/ui/views/controls/table/table_view_unittest.cc

Labels: Merge-Request-58
Project Member

Comment 11 by sheriffbot@chromium.org, Mar 28 2017

Labels: -Merge-Request-58 Hotlist-Merge-Approved Merge-Approved-58
Your change meets the bar and is auto-approved for M58. Please go ahead and merge the CL to branch 3029 manually. Please contact milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 12 by bugdroid1@chromium.org, Mar 30 2017

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

commit b554454239ebb239fc7da684b70b6be8ae860c8b
Author: estade <estade@chromium.org>
Date: Thu Mar 30 01:07:34 2017

Fix initial focus of Task Manager for ChromeOS.

The current code appeared to work because a DialogDelegate actually
defaults to focusing the default button ("End Task") rather than nothing
(like a default WidgetDelegate). Thus when you press Shift+Escape the
row for the active tab is selected, and "End Task" is focused. This
"fixed" the blue line around the table.

But on ChromeOS it's possible to have no windows open and press
Shift+Escape, and then the Browser row is selected initially. You're not
allowed to end that task, so the button is disabled.
Widget::SetInitialFocus has code to advance focus if the initial focus
View can't actually receive focus, hence the table winds up receiving
focus.

BUG= 702489 

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

[modify] https://crrev.com/b554454239ebb239fc7da684b70b6be8ae860c8b/chrome/browser/ui/views/task_manager_view.cc
[modify] https://crrev.com/b554454239ebb239fc7da684b70b6be8ae860c8b/chrome/browser/ui/views/task_manager_view.h

I will merge these two fixes early next week.
Project Member

Comment 14 by sheriffbot@chromium.org, Apr 3 2017

This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: -Hotlist-Merge-Approved -Merge-Approved-58
Status: Fixed (was: Started)
I don't really think this is worth merging any more because
a) it's just a graphical difference
b) we'd have to merge 4 CLs (two on this bug, one for fixing some tests and one for addressing new gesture event issues)

so i'm just going to mark it fixed without merging.
Status: Verified (was: Fixed)
Chrome OS 9460.5.0/ 59.0.3071.15

Sign in to add a comment