Add focus visibility on tabs with pills

[mayralgr]

Fixes #31427

Proposal: add the scss variables for the focus

Preview: https://deploy-preview-32357--twbs-bootstrap.netlify.app/docs/5.0/components/navs-tabs/#javascript-behavior

[patrickhlauke]

I think this should specifically target only the pills nav links, as this PR in its current form affects all nav links - see, for instance, the top navigation in the header (Home, Docs, Examples ...) https://deploy-preview-32357--twbs-bootstrap.netlify.app/

[mayralgr]

@patrickhlauke changes where made, let me know if another change is needed

[mdo]

We shouldn't be setting focus styles (the box-shadow) on hover. Will review in more depth likely after beta 2.

[mayralgr]

@mdo I left it just with the focus. Let me know if it is the correct approach.

[patrickhlauke]

We shouldn't be setting focus styles (the box-shadow) on hover. Will review in more depth likely after beta 2.

well, it was just replicating what buttons do...i personally wouldn't have minded.

[patrickhlauke]

beyond that though, https://deploy-preview-32357--twbs-bootstrap.netlify.app/docs/5.0/components/navs-tabs/ looks good to me

[mayralgr]

@mdo is the anything else needed ? Thank you!

[patrickhlauke]

I think the doubling up I saw was because outline wasn't reset when box-shadow was added.

ah gotcha, we can fix it just for the pill ones, yes

[mayralgr]

Hi guys I did this minor change but I am having trouble with the tests, as I left it I think I need a rebase or something ( I try but still gives an error ) any help ? 😅

[patrickhlauke]

i think you forgot to git pull changes that were made to your branch here (as a result of rebasing/merging in changes here on github) before making the latest change, so your PR suddenly included lots of reverts of things that blew the tests up. i think i managed to manually fix it

[XhmikosR]

Haven't looked at it too close, but it seems like the outline results in duplicate borders for active tabs and -1px or so for inactive ones.

[patrickhlauke]

Haven't looked at it too close, but it seems like the outline results in duplicate borders for active tabs and -1px or so for inactive ones.

not sure i'm following what you're seeing. which browser? which example specifically? got a screenshot/pointer to it?

[XhmikosR]

Sorry, for the late reply. I probably mixed the examples and I was looking at the nav-tabs example, not the nav-pills one.

It works, but the focus style seems to overlap with the other nav-items.

Edit: also browsers seem to have their own focus styles already, and with this patch we will have an inconsistent style in nav-tabs?

[patrickhlauke]

Sorry, for the late reply. I probably mixed the examples and I was looking at the nav-tabs example, not the nav-pills one.

It works, but the focus style seems to overlap with the other nav-items.

Yeah I think there's no clean way around this. As that overlapping focus style only shows for keyboard users as they navigate through, and momentarily (before either the link is followed or, in case of dynamic tabs, when that tab then becomes active) it's something I think is not completely deal-breaking (otherwise we have to start moving to inner shadows or something rather that box shadows)

Edit: also browsers seem to have their own focus styles already,

If you go to https://getbootstrap.com/docs/5.0/components/navs-tabs/#pills with Firefox (at least on Windows), there's no visible default focus indication. See this animation where I go back and forth with Tab and note how on the active one nothing can be seen when it gets focus:

With this change, it's at least consistent across all browsers.

Leaving it up to browser default in this case is not a good idea, because of this variability (while yes, Chrome for instance has very clear default focus outline).

and with this patch we will have an inconsistent style in nav-tabs?

You mean inconsistency between the regular .nav, .nav-tabs, and .nav-pills ? yeah, i'd say that can be tackled in a separate PR though? (fwiw in those, the default browser outlines, even while being different across browsers, work ok as they don't disappear for active elements, so this PR tackled specifically the one case where this was an issue, and followed @mdo's suggestion)

[XhmikosR]

All right, let's wait for @twbs/css-review to have a look. I'd swear there was some kind of CSS property we recently used in another RP .

[patrickhlauke]

All right, let's wait for @twbs/css-review to have a look. I'd swear there was some kind of CSS property we recently used in another RP .

ah, you might be thinking of https://developer.mozilla.org/en-US/docs/Web/CSS/isolation ... hmm, this may help yes, but not sure how the semi-transparent box shadow over the active pill would show, if at all

[ffoodd]

@XhmikosR @patrickhlauke Just tried, it doesn't help there since our box-shadow is the same color than our background, with a transparency. Either we change this color (and drop transparency, but that sounds inconsistent with other styles) to make it noticeable, or we consider adding space between pills?

There might be other approaches but I'm unable to find one for now.

[XhmikosR]

I'd vote to just keep it simple and consistent for the time being :)

[patrickhlauke]

This has slipped a bit off our radar... @mdo and @twbs/css-review in general, what are your feelings on this? it's probably not perfect for all circumstances, but does patch a gap in accessibility we currently have.

[ffoodd]

LGTM, not perfect but a good patch for now 👌

Focus styles are a whole topic deserving closer attention.

[GeoSot]

@twbs/css-review may we put this on a following project or close it?

[mdo]

Focus styles coming in #33125.