Material Refresh - Focus Ring may be too thick. |
|||||||||||||||||||||||||||||||||||||
Issue description
,
Jun 8 2018
,
Jun 8 2018
Manukh, here's one more for you. The current focus rings don't match the mocks, and in fact differ per platform. After getting clarity from UX, we should update the code to match UX intent. See the mocks for the comment thread discussing what we should do.
,
Jun 14 2018
,
Jun 15 2018
>> 1. apparently, the focus ring is thicker (4px) on mac, than on linux or windows (2px). is this intentional, or should one set of os's be changed to match the other? Let's move all OS's to 4px. >> 2. on mac, the focus ring is drawn partly inside the location bar, and party outside the location bar. on windows / linux, the focus ring is drawn outside the location bar. this issue 851122 is to change the ring to be displayed inside the location bar per the design mockups. Should this apply to all 3 OS's, or just linux & windows? Draw the ring partly inside the location bar for all 3 Os's
,
Jun 15 2018
To clarify, the differences between mac and the other os's (2px v 4px thickness & drawn outside vs partially inside) is true for all current focus rings, not just the ring for the location bar. Would you like these changes (4px thickness and drawn partially inside) to apply to all focus rings or just the location bar's?
,
Jun 15 2018
,
Jun 20 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/debc708809bcd2c56863db2b032dcc2d580d911f commit debc708809bcd2c56863db2b032dcc2d580d911f Author: manuk <manukh@chromium.org> Date: Wed Jun 20 18:01:39 2018 Adjust focus ring drawing on windows & linux to match mac Previously, on Windows and Linux, the focus ring was drawn outside its parent element, and was 2px's thick. Now Windows and Linux match Mac's behavior; the focus ring is drawn half outside and half inside the parent element, and is 4px's thick. Bug: 851122 Change-Id: I3a048d0ae96095e2487e12f474b1336e16b06651 Reviewed-on: https://chromium-review.googlesource.com/1101540 Reviewed-by: Scott Violet <sky@chromium.org> Reviewed-by: Tommy Li <tommycli@chromium.org> Commit-Queue: manuk hovanesian <manukh@chromium.org> Cr-Commit-Position: refs/heads/master@{#568919} [modify] https://crrev.com/debc708809bcd2c56863db2b032dcc2d580d911f/ui/views/style/platform_style.cc [modify] https://crrev.com/debc708809bcd2c56863db2b032dcc2d580d911f/ui/views/style/platform_style_mac.mm
,
Jun 21 2018
,
Jun 25 2018
bettes: Should we reevaluate the thickness of the focus ring?
,
Jun 28 2018
Um... this is problematic for multiple reasons. First, we moved from 1 px to 4 DIP. That's a huge change. On 200% screens the omnibox border is now 8x thicker than before, which feels surprising. Second, because we're drawing part of the ring inside the element, we can now overdraw it with the selection rect for scripts with large ascents/descents. The code in the omnibox tries to clamp the font size, but it clips to its own borders, not 2 DIP inside on all edges. We need to either only draw outside, or change the sizing and clipping code in the omnibox. Third, do we have any other places in the UI which do focus rings like this? It feels a bit without precedent. Fourth, subjectively, I really don't like this. That's probably less important but my reaction was "this has to be a bug, it can't possibly be right". Recommendation: return to 2 DIP focus rings fully outside the omnibox, which minimizes the functional bugs.
,
Jun 28 2018
Assigning to bettes@ to make a call on this one.
,
Jun 28 2018
Issue 836946 has been merged into this issue.
,
Jun 29 2018
Issue 859144 has been merged into this issue.
,
Jun 29 2018
,
Jun 29 2018
Pasting my comments from the de-duplicated issue 859144 below: Suggesting to re-think the omnibox focus border. Here are the issues: 1) The new omnibox focus border seems too thick, too pronounced. Is this necessary? Could it be a little thinner and a little bit more subtle. It looks that it is 3px thick (judging by the eye). Attaching screenshots. Maybe 1px would be better? 2) Because the border is so pronounced, the following steps are basically creating a "flash". - Focus omnibox with the keyboard or mouse. Border appears. - Start typing a query, as soon as you type. Border disappears. This appear->disappear sequence is a bit too much, especially since typing is always what one does after focusing the omnibox. 3) Note that this is a radical change compared to the old omnibox UI, where when focused there was no focus indication at all, except from the blinking line.
,
Jul 2
The focus ring is currently PlatformStyle::kFocusHaloThickness thick and outset by PlatformStyle::kFocusHaloInset - it doesn't do any special customization of that. These are 4pt and -2pt respectively (so inset by 2pt). LocationBarView doesn't do any specific customization of it: see LocationBarView::RefreshFocusRing(). If we want to change the thickness for this use case probably the best bet is to add a setter on FocusRing for that.
,
Jul 4
Don't have much stake into this. Functional reasonings sound like a thing. 2dp, all platforms, all areas of the UI SGTM. Assigning back to tommycli@ P.S. This applies to Fuschia now?
,
Jul 6
If we want to adjust it globally I'll do that.
,
Jul 6
For <https://chromium-review.googlesource.com/c/chromium/src/+/1127929>: omni-4 / text-4 are the existing 4pt focus ring omni-2 / text-2 / button-2 are a 2pt focus ring
,
Jul 9
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/332d32b73fe6367cc4fdf1baca9ac7909d3bbed5 commit 332d32b73fe6367cc4fdf1baca9ac7909d3bbed5 Author: Elly Fong-Jones <ellyjones@chromium.org> Date: Mon Jul 09 18:43:22 2018 views: reduce focus halo thickness to 2dip This change reduces focus rings from 4dip thick inset by 2dip to 2dip thick inset by 1dip. Bug: 851122 Change-Id: I8e38df43fd410cb4f1dc526c767b4f77b26733cd Reviewed-on: https://chromium-review.googlesource.com/1127929 Commit-Queue: Elly Fong-Jones <ellyjones@chromium.org> Reviewed-by: Scott Violet <sky@chromium.org> Cr-Commit-Position: refs/heads/master@{#573381} [modify] https://crrev.com/332d32b73fe6367cc4fdf1baca9ac7909d3bbed5/ui/views/style/platform_style.cc
,
Jul 10
Able to reproduce the issue on Mac 10.13.3 using chrome build without fix. Verified the fix on Mac 10.13.3 and Ubuntu 17.10 using latest chrome version #69.0.3487.0 as per the comment #23 and #24. Attaching screen shots for reference. Observed that focus Ring thickness have been reduced from 4 dip to 2 dip. Hence, the fix is working as expected. Adding the verified labels. Thanks...!!
,
Jul 10
Thanks for verifying TE!
,
Jul 11
Thanks for reducing from 4 DIP to 2! I'm still concerned about insetting the focus ring partly into the control, though, for the reason expressed in comment 14. Does it make sense to have 2 DIP with 0 inset? Maybe not since this was running the focus ring very close to the top of the toolbar, but was this discussed? Does a 1 DIP focus ring (inset 0) make sense in that case? Note that pre-refresh the omnibox focus ring was 1 px, so 1 DIP would still be bigger on >1 DSF. Or is 1 DIP too subtle elsewhere, and would be inconsistent to do only in the omnibox? If neither of these is possible, have we made sure that the omnibox font sizing/positioning code at least won't allow the text box to grow into this inset area? Not reopening this yet, but I'm happy to reopen, or open new bugs, as needed to address anything above.
,
Jul 12
,
Jul 12
#27: Screenshots attached. I was wrong in #20 - the focus ring is inset by both kFocusHaloThickness / 2 and kFocusHaloInset. Right now, kFocusHaloThickness / 2 is 1.0f and kFocusHaloInset is -1.0f, so the overall inset is 0. The focus ring is already flush with the content of the field. Screenshots of this are the flush-* images attached. I tried setting kFocusHaloInset to 0, so the overall inset would be 1; that yielded the inset-* images attached. I like the look of inset-text more than flush-text, but inset-round has a glitch (probably fixable by having the round buttons outset the path a little to compensate for the new inset). What do you think?
|
|||||||||||||||||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||||||||||||||||
Comment 1 by tommycli@chromium.org
, Jun 8 2018Owner: tommycli@chromium.org
Status: Started (was: Untriaged)