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

Issue 788128 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jan 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 1
Type: Bug-Regression



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 description

Chrome 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


 
Actual.png
876 KB View Download

Comment 1 by vku...@etouch.net, Nov 23 2017

Labels: hasbisect-per-revision
Owner: tapted@chromium.org
Status: Assigned (was: Unconfirmed)
You are probably looking for a change made after 514116 (known good), but no later than 514117 (first known bad).
CHANGELOG URL:
The script might not always return single CL as suspect as some perf builds might get missing due to failure.
https://chromium.googlesource.com/chromium/src/+log/04f1082efb63369e19ace6dde8b62c8702303b88..6b6e8fe1fcf17e0545c688d1bfda16d6f79fd97e

Suspecting: https://chromium.googlesource.com/chromium/src/+/6b6e8fe1fcf17e0545c688d1bfda16d6f79fd97e
Could you please check whether this is caused with respect to your change, if not please help us in assigning it to the right owner.

Note: Issue not seen on Win & Linux OS.

Comment 2 by vku...@etouch.net, Nov 23 2017

Manual bisect range:
Good Build: 64.0.3260.2
Bad Build: 64.0.3261.0

Comment 3 by tapted@chromium.org, Nov 24 2017

Cc: -nyerramilli@chromium.org bettes@chromium.org tapted@chromium.org
Owner: ellyjo...@chromium.org
+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.
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.
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-02-2018 14-05-18.gif
1.6 MB View Download
Status: Started (was: Assigned)
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.
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.
Project Member

Comment 8 by bugdroid1@chromium.org, 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

Comment 9 by vku...@etouch.net, 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.

Actual_Canary.mov
1.4 MB Download
#9: Thanks for checking again - I'll go check my fix on Mac.
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.
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.
pre-md.png
4.6 KB View Download
post-md.png
4.0 KB View Download
overlap-vert.png
6.1 KB View Download
thin-nonmd.png
5.6 KB View Download
thin-md.png
4.7 KB View Download
Project Member

Comment 13 by bugdroid1@chromium.org, 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

Status: Fixed (was: Started)
Okay, this should now be Really Fixed.

Comment 15 by vku...@etouch.net, Jan 9 2018

Labels: TE-Verified-M65 TE-Verified-65.0.3316.0
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!
Current_Behavior.mov
3.5 MB Download

Sign in to add a comment