Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Site-icon's focus ring is too tall, and is clipped beneath the site icon below it #14802

Closed
stephendonner opened this issue Mar 18, 2021 · 8 comments · Fixed by brave/brave-core#8327

Comments

@stephendonner
Copy link

Description

Focus ring is too tall, and is clipped beneath the site icon below it

Steps to Reproduce

  1. switch to the Favorites view in Top Sites via either the context-menu item or via Customize -> Top Sites -> Favorites
  2. add > 6 favorites (up to 12)
  3. start pressing Tab on the first row, and notice the focus ring

Actual result:

The focus ring extends beneath the next site's favicon, and looks awkward due to being clipped

Screen Shot 2021-03-17 at 7 29 06 PM

Expected result:

No clipping into the next site's favicon; in Figma, the focus ring looks to merely comprise the favicon/tile, not the accompanying site text

Screen Shot 2021-03-17 at 7 37 29 PM

Reproduces how often:

100%

Brave version (brave://version info)

Brave 1.24.2 Chromium: 89.0.4389.90 (Official Build) nightly (x86_64)
Revision 62eb262cdaae9ef819aadd778193781455ec7a49-refs/branch-heads/4389@{#1534}
OS macOS Version 11.2.3 (Build 20D91)

Version/Channel Information:

  • Can you reproduce this issue with the current release? no
  • Can you reproduce this issue with the beta channel? yes
  • Can you reproduce this issue with the nightly channel? yes

cc: @karenkliu @rebron @simonhong @bsclifton

@simonhong
Copy link
Member

Hmm, it seems regression. 1.23.x didn't have this issue.

@bsclifton
Copy link
Member

It's possible this regressed with brave/brave-core#8120
cc: @petemill

@GeetaSarvadnya
Copy link

1.21.x - Not Reproducible
1.22.x - Not Reproducible
1.23.x - Reproducible
1.24.x - Reproducible

@simonhong
Copy link
Member

simonhong commented Mar 18, 2021

FYI: My local build is 1.23.33 and that version doesn't have this issue.

@GeetaSarvadnya
Copy link

I have verified in 1.23.43 and the issue is reproducible in this version
Top tiles_1 23 43

@rebron rebron added the priority/P4 Planned work. We expect to get to it "soon". label Mar 19, 2021
@simonhong simonhong self-assigned this Mar 23, 2021
@bsclifton
Copy link
Member

This doesn't seem to be a problem anymore w/ Chromium 90; @simonhong can you verify? Should we still fix this? 1.23.x will be first release w/ Chromium 90 and is also first build with this code; so I don't think we need a fix

@bsclifton
Copy link
Member

bsclifton commented Mar 25, 2021

OK confirmed it still IS an issue; you just need to use TAB key to focus 👍 (per steps in top post). In Chromium 89 it was having the focus ring problem when you clicked - which is what I noticed was not happening anymore 😄

@stephendonner
Copy link
Author

Verified FIXED using

Brave 1.24.26 Chromium: 90.0.4430.30 (Official Build) nightly (x86_64)
Revision 5674335ff855e43f3bccf8cfc29a779bdf0d067f-refs/branch-heads/4430@{#532}
OS macOS Version 11.2.3 (Build 20D91)

The focus ring is now back to spec.

Screen Shot 2021-03-25 at 9 27 51 AM

Screen Shot 2021-03-25 at 9 28 00 AM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants