New issue
Advanced search Search tips

Issue 848299 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 2
Type: Bug



Sign in to add a comment

macviews: use round focus rings for round topchrome items

Project Member Reported by ellyjo...@chromium.org, May 31 2018

Issue description

These items should have round focus rings:

* The new tab button
* The back/forward/reload button
* The profile switcher
* The app menu
* The security chip

The new tab button should not use the dashed focus ring it has right now.
 
Status: Started (was: Assigned)
https://chromium-review.googlesource.com/c/chromium/src/+/1080887
Screen Shot 2018-05-31 at 11.34.15 AM.png
26.0 KB View Download
Project Member

Comment 2 by bugdroid1@chromium.org, May 31 2018

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

commit 7682a8f7db28e76c21fbe5fb426dde16bfbaaddc
Author: Elly Fong-Jones <ellyjones@chromium.org>
Date: Thu May 31 16:58:35 2018

macviews: add focus ring to refresh new tab button

The refresh new tab button (the plus) should not use the default dashed focus
painter. A rounded focus ring looks good here.

Bug:  848299 
Change-Id: I8a081886bab7e0613ba30f4b6bb704d548570277
Reviewed-on: https://chromium-review.googlesource.com/1080887
Reviewed-by: Allen Bauer <kylixrd@chromium.org>
Commit-Queue: Elly Fong-Jones <ellyjones@chromium.org>
Cr-Commit-Position: refs/heads/master@{#563273}
[modify] https://crrev.com/7682a8f7db28e76c21fbe5fb426dde16bfbaaddc/chrome/browser/ui/views/tabs/new_tab_button.cc
[modify] https://crrev.com/7682a8f7db28e76c21fbe5fb426dde16bfbaaddc/chrome/browser/ui/views/tabs/new_tab_button.h

https://chromium-review.googlesource.com/#/c/chromium/src/+/1081049
Screen Shot 2018-05-31 at 1.43.33 PM.png
39.2 KB View Download
Screen Shot 2018-05-31 at 1.43.40 PM.png
43.5 KB View Download
https://chromium-review.googlesource.com/#/c/chromium/src/+/1080889
Screen Shot 2018-05-31 at 4.58.46 PM.png
43.9 KB View Download
Screen Shot 2018-05-31 at 4.58.51 PM.png
44.0 KB View Download
Labels: Needs-Feedback
Tested the issue on latest chrome version 69.0.3450.0 using Mac 10.12.6 with steps mentioned below:
1) Launched chrome reported version and enabled 'Refresh' and 'Mac views' flags in chrome flags
2) Opened one secure site and pressed Tab button to switch the focus, observed round focus on 'New Tab button', 'Forward/Back/Reload button', 'Profile switches' and 'App Menu' but round focus is not seen on 'Secure Chip'

@Elly Fong-Jones: Please find the attached screencast for your reference and help us in confirming the fix.

Thanks!
848299.mp4
1.3 MB View Download
Project Member

Comment 7 by bugdroid1@chromium.org, Jun 7 2018

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

commit d9cf0d0676d512c201d9c53017788bcaedeadc6d
Author: Elly Fong-Jones <ellyjones@chromium.org>
Date: Thu Jun 07 16:09:27 2018

macviews: use rounded focus rings for omnibox buttons

This change adds rounded focus rings to the location icon button and to page
action buttons.

Bug:  848299 
Change-Id: I06b501b664315ad615d334fee23149854550bd06
Reviewed-on: https://chromium-review.googlesource.com/1080889
Reviewed-by: Allen Bauer <kylixrd@chromium.org>
Commit-Queue: Elly Fong-Jones <ellyjones@chromium.org>
Cr-Commit-Position: refs/heads/master@{#565286}
[modify] https://crrev.com/d9cf0d0676d512c201d9c53017788bcaedeadc6d/chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc

Project Member

Comment 8 by bugdroid1@chromium.org, Jun 8 2018

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

commit a0c4aeac1add95b255d9adaa583969b6dc88eb7c
Author: Elly Fong-Jones <ellyjones@chromium.org>
Date: Fri Jun 08 17:43:06 2018

views: support focus rings on tab-close buttons

This change:
1) Adds focus rings to the tab-close button
2) Moves the new tab button to last in the TabStrip's focus order if it is not
   in LEADING position so that the keyboard focus order within the TabStrip is
   correct

Positioning the ring for the tab-close button requires some finesse: the tab
close button has its margins adjusted, but the focus ring only needs to draw
around the visible part of the button.

Bug:  848299 
Change-Id: I226683d3a8829f5a88e9bf102b4c8ba4ecfae459
Reviewed-on: https://chromium-review.googlesource.com/1091131
Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Reviewed-by: Peter Boström <pbos@chromium.org>
Commit-Queue: Elly Fong-Jones <ellyjones@chromium.org>
Cr-Commit-Position: refs/heads/master@{#565673}
[modify] https://crrev.com/a0c4aeac1add95b255d9adaa583969b6dc88eb7c/chrome/browser/ui/views/tabs/tab.cc
[modify] https://crrev.com/a0c4aeac1add95b255d9adaa583969b6dc88eb7c/chrome/browser/ui/views/tabs/tab_close_button.cc
[modify] https://crrev.com/a0c4aeac1add95b255d9adaa583969b6dc88eb7c/chrome/browser/ui/views/tabs/tab_close_button.h
[modify] https://crrev.com/a0c4aeac1add95b255d9adaa583969b6dc88eb7c/chrome/browser/ui/views/tabs/tab_strip.cc

Status: Fixed (was: Started)
Labels: TE-Verified-69.0.3493.0 TE-Verified-M69
Able to reproduce the issue on chrome reported version 69.0.3450.0(Build without fix)
Verified the fix on Mac 10.12.6 on Chrome version #69.0.3493.0 as per the comment#0
Attaching screen cast for reference.
Observed "The round focus on 'New Tab button', 'Forward/Back/Reload button', 'Profile switches', 'App Menu' and 'Secure Chip'"
Hence, the fix is working as expected.
Adding the verified label.

Thanks!
848299.mp4
2.6 MB View Download

Sign in to add a comment