MacViewsBrowser: Omnibox focus ring shows as double layered? |
|||||||||
Issue descriptionChrome Version: 67.0.3395.0 OS: macOS 10.13.5 Beta (17F35e) What steps will reproduce the problem? (1) --enable-features=ViewsBrowserWindows (2) Open new tab (3) Click on Omnibox to see the focus ring What is the expected result? Omnibox focus ring should be highlighted as single layer. What happens instead? Omnibox focus ring shows as double layered? Please see the attached screen shots for your reference. Thank you!
,
Apr 13 2018
The appropriate dev is probably me.
,
Apr 16 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1af8d3ffcdec96999a970c108b85586bb6f65746 commit 1af8d3ffcdec96999a970c108b85586bb6f65746 Author: Elly Fong-Jones <ellyjones@chromium.org> Date: Mon Apr 16 17:59:41 2018 macviews: use mac-y focus rings This change: 1) Uses the system focus ring color even when SecondaryUiMd is enabled 2) Moves focus ring thickness and inset out to PlatformStyle 3) Adds Mac overrides of the default values Bug: 832306 Change-Id: I6c18d9d0431dc411a5cb95f9c0639e41f4aa1127 Reviewed-on: https://chromium-review.googlesource.com/1012163 Commit-Queue: Elly Fong-Jones <ellyjones@chromium.org> Reviewed-by: Scott Violet <sky@chromium.org> Cr-Commit-Position: refs/heads/master@{#551030} [modify] https://crrev.com/1af8d3ffcdec96999a970c108b85586bb6f65746/ui/native_theme/native_theme_mac.mm [modify] https://crrev.com/1af8d3ffcdec96999a970c108b85586bb6f65746/ui/views/controls/focus_ring.cc [modify] https://crrev.com/1af8d3ffcdec96999a970c108b85586bb6f65746/ui/views/style/platform_style.cc [modify] https://crrev.com/1af8d3ffcdec96999a970c108b85586bb6f65746/ui/views/style/platform_style.h [modify] https://crrev.com/1af8d3ffcdec96999a970c108b85586bb6f65746/ui/views/style/platform_style_mac.mm
,
Apr 16 2018
Thank you Elly, can this be marked as fixed if nothing else is pending?
,
Apr 16 2018
Yep, the fix is landed on trunk now.
,
Apr 16 2018
CL listed at #3 will need a merge to M67. Please request a merge.
,
Apr 17 2018
Tested the issue on chrome version 68.0.3398.0 using Mac 10.12.6 with steps mentioned below: 1) Launched chrome version and enabled --enable-features=ViewsBrowserWindows from chrome://flags 2) Clicked on Omnibox to see the focus ring, seen the focus ring is overlapped with the ash colour ring ellyjones@ Please find the attached screenshot for your reference and help us in confirming the fix. Thanks!
,
Apr 17 2018
#7: That screenshot is working as intended.
,
Apr 17 2018
,
Apr 17 2018
Approving merge to M67 branch 3396 based on comments #7 and #8. Pls merge ASAP so we can pick it up for tomorrow's M67 dev release. Thank you.
,
Apr 17 2018
Rejecting merge to M67 per offline chat with ellyjones@.
,
Apr 17 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1af8d3ffcdec96999a970c108b85586bb6f65746 commit 1af8d3ffcdec96999a970c108b85586bb6f65746 Author: Elly Fong-Jones <ellyjones@chromium.org> Date: Mon Apr 16 17:59:41 2018 macviews: use mac-y focus rings This change: 1) Uses the system focus ring color even when SecondaryUiMd is enabled 2) Moves focus ring thickness and inset out to PlatformStyle 3) Adds Mac overrides of the default values Bug: 832306 Change-Id: I6c18d9d0431dc411a5cb95f9c0639e41f4aa1127 Reviewed-on: https://chromium-review.googlesource.com/1012163 Commit-Queue: Elly Fong-Jones <ellyjones@chromium.org> Reviewed-by: Scott Violet <sky@chromium.org> Cr-Commit-Position: refs/heads/master@{#551030} [modify] https://crrev.com/1af8d3ffcdec96999a970c108b85586bb6f65746/ui/native_theme/native_theme_mac.mm [modify] https://crrev.com/1af8d3ffcdec96999a970c108b85586bb6f65746/ui/views/controls/focus_ring.cc [modify] https://crrev.com/1af8d3ffcdec96999a970c108b85586bb6f65746/ui/views/style/platform_style.cc [modify] https://crrev.com/1af8d3ffcdec96999a970c108b85586bb6f65746/ui/views/style/platform_style.h [modify] https://crrev.com/1af8d3ffcdec96999a970c108b85586bb6f65746/ui/views/style/platform_style_mac.mm
,
Apr 20 2018
Able to reproduce the issue on chrome reported version 67.0.3395.0 Verified the fix on Mac 10.12.6 on Chrome version #68.0.3401.0 as per the comment#0 Attaching screenshot for reference. Observed "Omnibox focus ring is highlighted as single layer" Hence, the fix is working as expected. Adding the verified label. Thanks! |
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by gov...@chromium.org
, Apr 13 2018