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

Docs: add focus visible outline to navbar's nav links and toggler #39825

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

julien-deramond
Copy link
Member

@julien-deramond julien-deramond commented Mar 22, 2024

Description

This PR adds a focus visible outline to navbar's nav links and toggler. The outline is the same as the one used when focusing the navbar brand logo for consistency.

Thix fix only works for Chrome and Firefox. Safari's navbar brand and skip links focus visible already have a contrast issue in the main branch; the outline rules not being handled the same way by the browser.

2024-03-22 08 24 43

Motivation & Context

In our current documentation, it's almost impossible to see the focused items in the navbar when navigating with the keyboard.

Before:

2024-03-22 08 24 13

Type of changes

  • Enhancement (non-breaking change which adds functionality)

Checklist

  • I have read the contributing guidelines
  • My code follows the code style of the project (using npm run lint)
  • My change introduces changes to the documentation
  • I have updated the documentation accordingly
  • (N/A) I have added tests to cover my changes
  • All new and existing tests passed

Live previews

Copy link
Member

@louismaximepiton louismaximepiton left a comment

Choose a reason for hiding this comment

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

I'm fine using :focus-visible since it's for the doc only.

Maybe you could find a way to fix it on Safari as well ? Maybe outline-color: white or something similar could do the trick ?

@XhmikosR
Copy link
Member

Actually, using white might look better in all browsers?

@julien-deramond
Copy link
Member Author

The idea of the PR was mainly to be a quick fix: to have the same focus visible rendering for the nav links, the .navbar-brand and the skip links.

This fix only works for Chrome and Firefox.
Safari was not handled because the .navbar-brand and the skip links have already a contrast issue; the outline not being handled the same way for Safari.

2024-03-23 08 04 25

We could tweak the whole rendering obviously for multi-browsers consistency, but it would take more time.

@XhmikosR
Copy link
Member

I'm just not sure the blue color along with white is good. Can't we just use white? Or will not look good in some cases?

@julien-deramond
Copy link
Member Author

I'm just not sure the blue color along with white is good. Can't we just use white? Or will not look good in some cases?

I think we could fine-tune the outline to be a white one-line (maybe with a bigger width) instead of two in the documentation header only, including skip links, since the background colors are known and with a dark tint both in light and dark modes.

Or we could do something more generic in the doc, like what's done in our Boosted documentation: a general rule that uses a double outline (black and white) that works everywhere and doesn't need fine-tuning: see https://boosted.orange.com/. With black and white double-outline for security, we're pretty sure that one color will be contrasted enough whatever the context. It's maybe less nice visually but works well overall.

@patrickhlauke
Copy link
Member

It's been ages since I dived into the various styles, but I'm just wondering what kind of strange overrides we have in place that are causing the issue in Chrome/Firefox in the first place? Would just removing those (rather than adding explicit extra styles) work?

I'm assuming the issue in Safari is that "Apple knows best" and just always goes with its default blue outline, regardless? And yes, could outline-color: white or similar work (as a patch just for the navbar perhaps)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Needs review
Development

Successfully merging this pull request may close these issues.

None yet

4 participants