Skip to content

Remove unnecessary tabindex attributes from offcanvas docs examples #41524

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

anotherjames
Copy link

@anotherjames anotherjames commented Jun 5, 2025

Description

Adjusts code examples in the offcanvas component's documentation in order to avoid potential bugs in Safari due to its unusual focus handling.

Motivation & Context

Having copied the code examples of the offcanvas component in an implementation for a website, I was surprised to find the presence of the tabindex attribute on the wrapping <div> caused unexpected behaviour in Safari. I had an event listener for the focusin event on an <a> element within the offcanvas component. When clicking that link element, Safari sets the event target to the <div>, unlike Chrome which sets it to the <a>. (Safari has a history of unusual focus handling!) This meant my event listener acted on the wrong element, causing a bug report from my client.

In an early commit of the original Offcanvas component's PR (#29017), the tabindex attribute on the wrapping <div> was being manually adjusted in javascript. But that no longer happens (e.g. since #33865 changed the way keyboard navigation & focus was handled anyway), yet the initial tabindex attribute remained in the code examples in its docs. Given that the wrapping <div> element wouldn't be tabbable without the tabindex attribute anyway, I believe the attribute is unnecessary. I suggest we remove it to save other developers who copy the examples from similar headaches!

Type of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Refactoring (non-breaking change)
  • Breaking change (fix or feature that would change existing 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
  • I have added tests to cover my changes
  • All new and existing tests passed

Live previews

Related issues

In an early commit of the original Offcanvas component's PR (twbs#29017), the tabindex attribute on the wrapping div was manually adjusted in javascript. By the time of that PR's final state, that no longer happened, but the initial tabindex attribute remained in the code examples in its docs.
Since Safari can do unexpected things with div[tabindex] elements, it is worth removing the now-unnecessary attribute from the examples to avoid obscure bugs.
@julien-deramond julien-deramond moved this from Needs review to Review in progress in v5.4.0 Jun 5, 2025
@github-project-automation github-project-automation bot moved this to Needs review in v5.4.0 Jun 5, 2025
@julien-deramond julien-deramond moved this from Review in progress to Needs review in v5.4.0 Jun 5, 2025
@julien-deramond
Copy link
Member

Thanks for reporting this issue and submitting a PR, @anotherjames!
Looping in @patrickhlauke for accessibility feedback 🙏

Thinking out loud: since the offcanvas component behaves similarly to a modal in terms of focus management, if we move forward with this change, we might want to consider whether a similar adjustment should also apply to modals—assuming it doesn't negatively impact accessibility or focus behavior.

@patrickhlauke
Copy link
Member

unfortunately, simply removing the tabindex and not programmatically moving focus at least to or into the offcanvas causes issues.

Video: using Chrome/NVDA, looking at the deploy preview. triggering one of the example offcanvases, we see focus still remains on the trigger button in the underlying page. using cursor up/down to read/navigate in NVDA, we're still going through the page behind the offcanvas. it's only if we hit TAB once the offcanvas is open that we end up inside the offcanvas and "trapped". for comparison, video includes the current behaviour, where focus is programmatically moved to the offcanvas.

bootstrap-offcanvas.mp4

Now, moving focus to the offcanvas itself (as with modals) can indeed have unintended side effects - for instance, you may not want its entire contents to be read out in one big go. A better solution is to programmatically move focus to some element at the top/start of the offcanvas...

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.

3 participants