Issue metadata
Sign in to add a comment
|
Regression:Focus ring overlap on folder icon after adding new folder.
Reported by
vku...@etouch.net,
Nov 23 2017
|
||||||||||||||||||||||
Issue descriptionChrome Version:64.0.3276.0 (Official Build) ccea2aa0a41cbb3dcbbf5961be4820d428b93a4c-refs/heads/master@{#518818}(64-bit) OS:Mac(10.12.6,10.13.2). What steps will reproduce the problem? (1)Launch chrome and open any webpage, click on star icon > edit (2)Click on 'new folder' from edit overlay and observe the focus ring. Actual: Focus ring overlap on folder icon after adding new folder. Expected: Focus ring should not overlap on folder icon after adding new folder. This is a regression issue broken in 'M64' and will soon update other info
,
Nov 23 2017
Manual bisect range: Good Build: 64.0.3260.2 Bad Build: 64.0.3261.0
,
Nov 24 2017
+bettes: what's your preference here? (or maybe elly is already working on a fix here?) The side of the folder icon is actually cut off by the textfield border as well as its focus ring. This is due to the increased padding under Harmony in textfields. But we didn't also increase the icon-to-text padding in TreeView, so it's currently impossible to position a textfield over a TreeView label without either a) shifting the text to the left after the textfield goes away; or b) cutting off part of the folder icon; or c) using a smaller textfield padding while in a TreeView also -> elly, since I've seen you doing a lot of treeview stuff lately.
,
Nov 27 2017
I have not yet worked on this. bettes, do you have an opinion? I think any of the fixes listed in #3 are doable technically.
,
Jan 2 2018
If there's the expected 12px between icon and label, the focus ring should be unobstructed. See attached gif. Also note the extra spacing between treeview border | arrow | and folder. I can't define what the bounds of the expand arrow is but the L and R margins of that arrow might need to be 8px.
,
Jan 4 2018
I think the problem is that in TreeView::LayoutEditor(), we do this:
row_bounds.Inset(kTextHorizontalPadding, kTextVerticalPadding);
row_bounds.Inset(-empty_editor_size_.width() / 2,
-(empty_editor_size_.height() - font_list_.GetHeight()) / 2);
I think the intent of the first part of that inset call is to consume the editor's normal horizontal spacing and replace it with kTextHorizontalPadding, but kTextHorizontalPadding is too small for a Harmony focus ring: focus rings are 2pt wide and kTextHorizontalPadding is also 2. This isn't an issue pre-Harmony because pre-Harmony focus indicators are 1pt.
One option is to remove these two calls altogether and use the normal Textfield padding, which works okay but causes the text to move when editing begins, which I guess is the intent of the original pair of Inset() calls.
Another option is to change the default value of kTextHorizontalPadding to leave room for the ring even in Harmony mode. Unfortunately this leaves a bit too much space between the text and the icon when not editing or not in Harmony mode. Still, it's not too bad.
The existing Cocoa treeviews on Mac are simply not editable, so there's no real precedent either way.
I think the better option is to put up with the text moving here, so I'll put up a CL to make that change.
,
Jan 4 2018
Ehhh, I've played with it some more and I think actually increasing the padding is better. I'll increase it in Harmony mode only.
,
Jan 4 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a1a251f4dd04319e9c541d4a5a45a940b337515e commit a1a251f4dd04319e9c541d4a5a45a940b337515e Author: Elly Fong-Jones <ellyjones@chromium.org> Date: Thu Jan 04 19:24:52 2018 views: increase TreeView editor padding sometimes Harmony Textfields have focus rings, which are 2pt wide and draw outside the Textfield's normal bounds. Since TreeView leaves only 2pt of padding between the Textfield's bounds and the icon on a row, the focus ring can overdraw the icon. To fix that, when focus rings are present, leave a bit of extra space. This doesn't look too bad when not editing since Harmony has more padding across the board, and when editing the focus ring no longer overdraws the icon. Bug: 788128 Change-Id: I4b0752fdec652d6ef3a6f47756ac21d777fa6468 Reviewed-on: https://chromium-review.googlesource.com/850915 Commit-Queue: Elly Fong-Jones <ellyjones@chromium.org> Reviewed-by: Scott Violet <sky@chromium.org> Cr-Commit-Position: refs/heads/master@{#527059} [modify] https://crrev.com/a1a251f4dd04319e9c541d4a5a45a940b337515e/ui/views/controls/tree/tree_view.cc [modify] https://crrev.com/a1a251f4dd04319e9c541d4a5a45a940b337515e/ui/views/controls/tree/tree_view.h
,
Jan 5 2018
Rechecked above issue on latest canary version 65.0.3312.0(Official build) on Mac OSX(10.12.6,10.13.2) and issue is still reproducible. Please refer attached screencast.
,
Jan 5 2018
#9: Thanks for checking again - I'll go check my fix on Mac.
,
Jan 5 2018
That's interesting - with the MD icons, there isn't enough space. The MD icon is a bit narrower but I wonder if it has less internal padding than the non-MD icons do. Hmmmm.
,
Jan 5 2018
Yup. The pre-MD open icon is wider than the closed icon, so to make the icons the same size, the closed icon gets some side padding, which helps prevent the overlap in pre-MD. With the MD icon, there's no such extra padding because there's no open icon at all. In fact, the overlap still does show up with the open folder icons in pre-MD. Hmm.
Worse, there's a *vertical* overlap too because of the size of the Harmony focus ring (overlap-vert.png). We can space the treeview items out more to avoid that, but I'm starting to get disenchanted with the Harmony focus rings in this specific instance.
I tried removing the Harmony focus rings altogether and replacing them with a 1pt GoogleBlue700 border instead; results in thin-{non,}md.png. That is probably the right way to go for now - it doesn't look quite like a Harmony textfield, but it avoids distorting the interior layout of the TreeView too much until TreeView gets a full Harmony redesign. I will upload a CL to implement that (and undo the earlier change) shortly.
,
Jan 8 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f1b8b41f59422b2de372cb39ed37b3b46c11c0cb commit f1b8b41f59422b2de372cb39ed37b3b46c11c0cb Author: Elly Fong-Jones <ellyjones@chromium.org> Date: Mon Jan 08 13:09:08 2018 views: don't show focus ring on TreeView's editor This change: 1) Undoes the positioning special case from a1a251f4dd 2) Sets a 1pt GoogleBlue700 solid border on the TreeView's editor Change (2) has the effect of suppressing the focus ring in Harmony mode, while not changing the focused appearance in pre-Harmony. Also, since the TreeView blurs the editor when it itself blurs (and ends editing), this ends up having the same behavior as a regular focus ring but a different appearance. Bug: 788128 Change-Id: I5cf34868a9075478bbfc8ffe4414c93b1f696187 Reviewed-on: https://chromium-review.googlesource.com/852377 Reviewed-by: Scott Violet <sky@chromium.org> Commit-Queue: Elly Fong-Jones <ellyjones@chromium.org> Cr-Commit-Position: refs/heads/master@{#527617} [modify] https://crrev.com/f1b8b41f59422b2de372cb39ed37b3b46c11c0cb/ui/views/controls/tree/tree_view.cc [modify] https://crrev.com/f1b8b41f59422b2de372cb39ed37b3b46c11c0cb/ui/views/controls/tree/tree_view.h
,
Jan 8 2018
Okay, this should now be Really Fixed.
,
Jan 9 2018
Update : Verified above issue on Mac(10.12.6), (10.13.1) & (10.13.3) OS with latest Canary build version #65.0.3316.0 and the issue is no longer reproducible. The fix is working as intended. Kindly review an attached screen-cast. Thank you! |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by vku...@etouch.net
, Nov 23 2017Owner: tapted@chromium.org
Status: Assigned (was: Unconfirmed)