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

v5/docs: reintroduce outline for docs code samples, buttons when :not(:focus-visible) #36507

Merged
merged 3 commits into from
Sep 1, 2022

Conversation

patrickhlauke
Copy link
Member

@patrickhlauke patrickhlauke commented Jun 5, 2022

closes #36506

@patrickhlauke
Copy link
Member Author

patrickhlauke commented Jun 5, 2022

Fixes the issues found in #36506 - need testing more thoroughly throughout though to check that the change to buttons.scss don't have any horrid side effects (though noting that this will be a notable change for keyboard users, as it will reenable the default focus indication in the browser for them, which I'd say is not a bad thing since we've long suffered contrast issues with those fancy button styles)

bootstrap-5-visible-focus-indication-fixed.mp4

@patrickhlauke patrickhlauke marked this pull request as ready for review June 5, 2022 15:59
@patrickhlauke patrickhlauke requested a review from a team as a code owner June 5, 2022 15:59
@patrickhlauke
Copy link
Member Author

@mdo et al, any thoughts on this?

@patrickhlauke patrickhlauke requested a review from ffoodd June 14, 2022 17:27
@patrickhlauke
Copy link
Member Author

any chance we could get this looked at @twbs/css-review @mdo ?

@julien-deramond
Copy link
Member

Focus indication on <pre> code samples is now visible. Great!

Screenshot from 2022-07-24 08-48-26

Note: We should improve the rendering because the focus area is under the copy button


Regarding the buttons, the rendering on Chrome is good. On Firefox, however, the box shadow + red outline combination hurts the eyes a little bit. Maybe just because I'm not used to it yet.

Screenshot from 2022-07-24 08-56-43
Screenshot from 2022-07-24 09-16-25

Screenshot from 2022-07-24 08-58-27
Screenshot from 2022-07-24 08-59-12

Screenshot from 2022-07-24 09-00-12


Off-topic: while playing with the documentation, focus indication for carousels controls; not visible enough IMO.

@patrickhlauke
Copy link
Member Author

in Chrome, there seems to be a tiny bit of overlap as well ... i can have a look at nudging something by a few pixels to compensate.

image

in my Firefox/Win, it uses blue rather than red for some reason. maybe we should explicitly define the outline colour as well at some point (part of the larger reevaluation of focus/focus-visible styles perhaps)

image

@julien-deramond
Copy link
Member

Let's do it step by step, no problem. In other PRs:

  • nudging something by a few pixels to compensate (if too complicated for this PR)
  • defining the outline colour to be coherent between browsers (part of the larger reevaluation of focus/focus-visible styles)

Copy link
Member

@julien-deramond julien-deramond left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! @mdo I let you check this as well.

@patrickhlauke
Copy link
Member Author

I'll have a look at the subtle nudging now as part of this PR, so hold off merging until I had a crack at it

@julien-deramond julien-deramond assigned mdo and unassigned mdo Jul 24, 2022
@patrickhlauke
Copy link
Member Author

Done...added an extra bit of explicit margin on the right-hand side of the pre.

In Firefox now:

image

In Chrome:

image

As said, leaving the outline color thing for another time (as it will likely need to be done on a section basis, so the outline will be a light colour in the header, and a dark one in the main section, etc)

So this PR is now ready for final review/merging

@patrickhlauke
Copy link
Member Author

@mdo et al, any news on this?

Sighted keyboard users rely on knowing where their focus is. If the `<pre>` receives focus (so that it can be scrolled by keyboard users, for instance) then it's essential that they know this is the case
avoids having the focus outline awkwardly clipped by the copy button
@mdo mdo force-pushed the patrickhlauke-issue36506 branch from 08ca108 to 7765c1e Compare September 1, 2022 01:23
@mdo mdo merged commit cda901f into main Sep 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

v5 docs: missing (clear) visible focus indication for version switch button and code samples
4 participants