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

Focus outline on Code Snippet is unstyled #2304

Closed
joshblack opened this issue Apr 13, 2019 · 15 comments · Fixed by #5233
Closed

Focus outline on Code Snippet is unstyled #2304

joshblack opened this issue Apr 13, 2019 · 15 comments · Fixed by #5233

Comments

@joshblack
Copy link
Contributor

When clicking in a code snippet, it appears that we have a un-styled focus indicator:

image

This is called (I believe) by tabindex="0" and role="textbox", which I believe is moreso meant for an area where you can freely edit text.

@joshblack
Copy link
Contributor Author

cc @dakahn do you agree on the textbox stuff?

@dakahn
Copy link
Contributor

dakahn commented Apr 15, 2019

Yeah, exactly right. Wrong role. I can bundle these fixes in with some code snippet improvements that have been made recently to CCR. 👍🏽

@stale
Copy link

stale bot commented May 1, 2019

We've marked this issue as stale because there hasn't been any activity for a couple of weeks. If there's no further activity on this issue in the next three days then we'll close it. Thanks for your contributions.

@stale stale bot added the wontfix label May 1, 2019
@dakahn
Copy link
Contributor

dakahn commented May 1, 2019

Looks like this was taken care of 😄

@dakahn dakahn closed this as completed May 1, 2019
@janhassel
Copy link
Member

I was just about to open an issue about the focus style on code snippet and found this issue.
In the current version ([email protected]) I can replicate the screenshot from @joshblack

image

In the single-line codesnippet the focus style beyond the box, in the multi-line it's just unstyled:

image

Using Chrome v79 on macOs 10.14.6

@tw15egan
Copy link
Collaborator

tw15egan commented Jan 28, 2020

Confirmed this is still present. @carbon-design-system/design , what should we do with this outline here?

@laurenmrice
Copy link
Member

Can we do something like this:
Artboard

@tw15egan
Copy link
Collaborator

With the way the component is set up, it doesn't seem like that is possible.

Screen Shot 2020-01-28 at 2 16 02 PM

Screen Shot 2020-01-28 at 2 16 32 PM

My recommendation would be to just remove the focus from the element since it is not interactive anyways. The focus will just go to the copy icon, then to show more.

@laurenmrice
Copy link
Member

laurenmrice commented Jan 28, 2020

ok yeah then lets just do that, have the focus on the copy icon and then the show more only^ .

@tw15egan
Copy link
Collaborator

Focus styles have been removed from multi-line snippet, @emyarod is going to look into a possible cross-browser / os solution to the single line snippet in a separate PR

@emyarod emyarod self-assigned this Jan 29, 2020
@emyarod
Copy link
Member

emyarod commented Jan 29, 2020

just a proof of concept, still WIP but looking for feedback before I continue @laurenmrice @tw15egan

here's a screenshot dump but we can go over this in more detail later on. every category below has 3 images each showing Windows 10, macOS with hidden scrollbars, and macOS with visible scrollbars. the first set is the as-is, the second set is a CSS only fix, and the third set includes a proof of concept that includes JS.

as-is

default

as-is1
as-is1
as-is4

snippet focused

as-is2
as-is2
as-is5

copy button focused

as-is3
as-is3
as-is6

preliminary fix (no height adjustments)

default

p1
p1
p4

snippet focused

p2
p2
p5

copy button focused

p3
p3
p6

dynamically set height based on whether or not scrollbar is present

if the content is short and a scrollbar is not rendered, it will appear the same as the prelim fix screenshots above on macOS

default

z1
p1
z1

snippet focused

z2
p2
z2

copy button focused

z3
p3
z3

@tw15egan
Copy link
Collaborator

@emyarod thanks for taking the time to put these screencaps together 🙏 Makes it very easy to illustrate the issue.

@emyarod
Copy link
Member

emyarod commented Jan 30, 2020

if the "preliminary fix (no height adjustments)" solution is fine then I can continue with that, but the proof of concept with a bit of tweaking might be worth implementing

@tw15egan
Copy link
Collaborator

tw15egan commented Jan 30, 2020

Personally, I think the preliminary fix (no height adjustments) looks great and could be implemented immediately it seems. I like that it stays the same height in both macOS versions.

@laurenmrice
Copy link
Member

agreed, lets go for that ^

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