Add shift-tab keyboard support for dialogs (modal & Offcanvas components)

[RyanBerliner]

Fixes #28481

• Share focus trapping behavior between modal and Offcanvas components
• Allow shift-tab keyboard navigation to wrap around backwards (go to the last focusable element)

There are a couple issues (primarily #33804) that aim to fix inconsistent screen reader navigation or announcements. The work I've done here only concerns tab navigation.

TODO:

• Test and revise the focusableChildren selector
• Fix isVisible utility function (tracking isVisible utility function returning false positives #33914)
• Fix BrowserStack tests

Preview Modal
Preview Offcanvas

[GeoSot]

@RyanBerliner can you rebase your MR please?

[RyanBerliner]

@GeoSot I've rebased. It looks like #33327 introduced a dependency of selector-engine.js for util/index.js. Combine this with my previous work on this PR and it's throwing circular dependency warning between the util index and selector engine files. I'm open to any number of resolutions, though IMO it seems that the getElement() in the util index file might be best suited living in the selector engine file? Let me know what you'd prefer to see done.

[GeoSot]

though IMO it seems that the getElement() in the util index file might be best suited living in the selector engine file?

Yes, seems right as it has a foreign dependency. Can you handle it ? (maybe isElement need to follow getElement)
I suppose is better on a separate MR

/cc @rohit2sharma95 , @alpadev

[RyanBerliner]

I hadn't realized how dependent other utility functions were on getElement() and isElement()... so moving either of those to selector engine would likely turn into some larger, and questionable refactoring. To avoid this I've decided instead to move focusableChildren() to the utility file. I think this can be justified since focusableChildren() does have some more opinionated logic in it that isn't as straight forward as the other selector engine functions.

[GeoSot]

Seems good to my eyes.

CC: @twbs/js-review

changes cuased of #34441

[GeoSot]

@RyanBerliner , can we update this MR (and resolve the suggestion)?

[XhmikosR]

@patrickhlauke: LGTY?

[XhmikosR]

FYI This doesn't pass on BrowserStack + there are some warnings: https://github.com/twbs/bootstrap/runs/3125887723

We should get those sorted before landing this :)

[RyanBerliner]

@XhmikosR the warnings seem simple enough, I can handle them. The failed tests on the other hand seem to be related to differences in how the <area> tag is selected. This came up in #33960, and we decided to just drop special handling of this element... I'm wondering if you all think it may be most practical to drop it from the selector here as well since it's used so little. Otherwise, happy to troubleshoot... just want to make sure everyone believes it's worth it since it's a funky element. I'm leaning towards dropping it.

[GeoSot]

I will support the 'drop' case, for one more time. However try to leave a comment about it, for future reference

[patrickhlauke]

Testing the behaviour, this now seems to behave correctly. Not looked at the code side, but from a functional point of view, this looks good to me

[RyanBerliner]

@XhmikosR I'm not sure I have the permission to re-run browserstack tests, however I believe everything should be resolved. If there are still warnings or errors that you'd like me to revisit lmk.

[XhmikosR]

All good @RyanBerliner thanks!

Also, thanks to @GeoSot everything seems to be good to go.