focus-trap / tabbable Goto Github PK

Use discussion on #136 as a basis.

Raised as a result of focus-trap/focus-trap#172 since that repo has the same build system (pretty much copy/paste, so same issue...).

This can be fixed by changing line 16 of index.js from..

|| (candidate.tagName === 'A' && !candidate.href && !candidate.tabIndex)

to..

|| (candidate.tagName === 'A' && !candidate.href && (!candidateIndex && candidateIndex !== 0))

I won't be using tabbable this way, but someone might be.

In IE11, a SVG with tabindex="0" does not have a tabIndex property so gets added to the orderedTabbables array no matter where it is in the DOM.

candidateIndex = parseInt(candidate.getAttribute('tabindex'), 10) || candidate.tabIndex;

Always triggers the OR condition if tabindex="0", which then assumes tabIndex is a valid property. SVGs return undefined for the tabIndex property.

I don't know how to get around this problem.

Could suggest in the README that people set focusable="false" on all SVGs and use tabindex if they need it to be focusable (which they probably do not, most of the time).

Description

When focus-trap is activated via .activate() on an element containing a form with radio inputs, an error is thrown:

Upon inspecting getCheckedRadio, it appears that the nodes parameter is not iterable, although it has a length property:

Environment:

Safari 15.4
MacOS Monterey 12.3.1

I am trying to use the library in internet explorer 11

in the transpiled version there is an arrow function
I can see it coming from line 49

let tabbableNodes = orderedTabbables
    .sort(sortOrderedTabbables)
    .map((a) => a.node)
    .concat(regularTabbables);

https://github.com/focus-trap/tabbable/blob/master/src/index.js#L49

We have two options, we can use a traditional function expression in the index.js, or we can configure the babel settings to have a target of internet explorer 9 to match the version support in the docs

https://drafts.csswg.org/css-box-3/#visibility-prop

Child elements of a visibility: hidden element can themselves be appeared by explicitly setting visibility: visible. In this case, any descendants are part of the tab order. tabbable currently excludes these.

The Drupal project is considering adding this library as one of our dependencies and so we're performing a standard stability review. We're looking into adopting this in https://www.drupal.org/project/drupal/issues/3113649. I fully acknowledge the likelihood of security issues tabbable are very low, so I particularly appreciate the time taken to answer these.

Since there isn't a policy at https://github.com/focus-trap/tabbable/security I'm curious if you have any official policies documented somewhere regarding:

Security releases
For example, does more than one version receive security fixes, or only the current version? It looks like 5.x is the first after the maintainer switch, so this may not apply currently, but would like to know if it would be applicable with a >=6.x release.
Release windows/cadence
For example, do they happen as necessary on any given day, or on a set schedule after a certain passage of time (e.g. once a month)? Looking at all the recent releases (which is great to see!), I can probably make some assumptions, but would like to confirm.
Backwards compatibility guarantees
Tabbable uses semver, so I assume the major version promises not to break BC. Are there any guarantees that a geven version will be supported for some period of time (an LTS version, for example), also with the understanding that things possibly changed between 4 and 5?

Thanks, I'm pleased to see all the recent activity on this library!

who can tell me how to use the library fix this error.

I have a situation where a TH element has both scope="col" and tabindex="0" attributes, which is causing Tabbable to break with "Cannot read properties of undefined (reading 'forEach').

index.js:425 Uncaught TypeError: Cannot read properties of undefined (reading 'forEach')
at e (index.js:425:14)
at index.js:429:32
at Array.forEach ()
at e (index.js:425:14)
at e.tabbable (index.js:473:10)

Tabbable appears to be evaluating !! item.scope as true because the attribute is present, and assumes there is a ShadowRoot to process.

A negative value (usually tabindex="-1") means that the element should be focusable, but should not be reachable via sequential keyboard navigation. Mostly useful to create accessible widgets with JavaScript.
https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/tabindex

This could be solved by changing '[tabindex]', to '[tabindex]:not([tabindex*="-"])',

Elements that are marked with inert or are nested in another inert element shouldn't be tabbable/focusable. Since this is just supported behind browsers flags at the moment, maybe tabbable can add support with a default false flag for now.

We removed Karma in #226 but that removed browser-based testing. tabbable, focus-trap, and focus-trap-react now all use the @testing-library and focus-trap/react use Cypress. Let's add Cypress tests to tabbable.

https://developer.mozilla.org/en-US/docs/Web/HTML/Element/summary

summary should be added to the list:
https://github.com/davidtheclark/tabbable/blob/c5ad06c85e596fea038c8a59a97324627eaa2784/index.js#L1-L11

So download is smaller.

Change next major version to swap the default behavior of displayCheck='full'/undefined to reflect the fact that detached nodes are not actually tabbable/focusable, with an option to ignore isHidden on detached nodes.

Originally posted by @idoros in #664 (comment)

A form element in a <fieldset disabled> should not be focusable:

https://codepen.io/MartijnCuppens/pen/abWRZNY?editors=1011

Hi,

aa2a699#diff-bfe9874d239014961b1ae4e89875a6155667db834a410aaaa2ebe3cf89820556R284 seems to have broken IE support as IE doesn't implement the contains method on HTMLDocument. I'm seeing this in IE11 (but according to the README for this project, IE9+ is supported).

From Googling around a workaround might be to fall back to node.ownerDocument.body.contains() if node.ownerDocument.contains() is undefined.

Thanks

When an element has tabindex="-1 it's not tabbable, unless you use Javascript to set the focus. If an element with tabindex="-1" has focus, it would be nice to include it, at least with an option.

The change to use getClientRects().length completely breaks this library for us: #604

We use tabbable when initializing dialogs, to find candidate element(s) to auto-focus when the dialog appears. These dialogs are React components and the call to tabbable() occurs during the componentDidMount lifecycle method. At that moment, the DOM elements we want to traverse using tabbable are fully formed, but they are not yet attached to the document. So getClientRects().length always returns 0, and thus every single one of our calls to tabbable fails. (At this library level, we have no way to control when the dialog is actually attached to the document.)

Could this change be reverted, or can we scope it to only be used when document.body.contains(node)?

In the README it states to import this module you need to use a named import instead of a default import.

import { tabbable } from 'tabbable';

tabbable is undefined.

You my want to update it to reflect the need for a default import.

It would be great if tabbable could identify the tab ordering between input fields across multiple web components.

Unfortunately since query selectors can't span shadow roots, it may require a very different approach.

I'm trying to use focus-trap in a StencilJS component and the various checks that rely on getComputedStyle fail when it hits a shadow root.

Pretty sure this could be fixed with a simple check, I'll take a crack at a PR. Thanks!

7vfqzmxv.entry.js:formatted:2 Uncaught TypeError: Failed to execute 'getComputedStyle' on 'Window': parameter 1 is not of type 'Element'.
    at p.hasDisplayNone (7vfqzmxv.entry.js:formatted:2)
    at p.hasDisplayNone (7vfqzmxv.entry.js:formatted:2)
    at p.hasDisplayNone (7vfqzmxv.entry.js:formatted:2)
    at p.hasDisplayNone (7vfqzmxv.entry.js:formatted:2)
    at p.isUntouchable (7vfqzmxv.entry.js:formatted:2)
    at l (7vfqzmxv.entry.js:formatted:2)
    at s (7vfqzmxv.entry.js:formatted:2)
    at i (7vfqzmxv.entry.js:formatted:2)
    at w (7vfqzmxv.entry.js:formatted:2)
    at Object.activate (7vfqzmxv.entry.js:formatted:2)

element.matches not working in ie 11. (src/index.js)

isDisabledFromFieldset is supposed to tell whether a node is in the first <legend> of the top-most disabled <fieldset>, but is actually assessing whether a node is in the first <legend> of the closest ancestor <fieldset>, which is not necessarily the top-most <fieldset> containing the node.

Right now only nodes within a container get tested, but not a container itself. I have use case when i want a container to be possibly included in resulted array.

In current state i may get what i need, by retrieving tabbables of a parent of a container and then filtering results by inclusion of container. My concern is that of performance. Siblings of a container can be big, while container itself may be small or empty, it may result in much of unnecessary work.

Another way is to add option which will tell to prepend a container to candidates list if it matches one of selectors.

What are your opinion on adding such option? It is ok or is it out of scope of this library?

I've been shifting my focus away from UI development, so don't plan on addressing new issues myself. If you use this library and want to see development continue, you can make that happen by becoming a co-maintainer — with permissions to triage issues, merge PRs, cut releases, etc.

Please comment below if you're interested!

Another possibility is for a dedicated owner to fork this code and create a new package. I'd be happy to link to that library from the README of this one.

When you tab through a radio set (radio inputs sharing a name), only one of them is actually tabbable — one of the following:

• The one that has been selected.
• If none are selected and you're moving forward with tab, the first one.
• If none are selected and you're moving backward with shift-tab, the last one.

This library doesn't take that into account: it treats radio inputs like other inputs, assuming they're all tabbable.

This would be a tricky problem to fix without adding a fair amount of delicate complexity. The best solution might be similar to the iframe problem: #23 (comment) — only hijack tab and shift+tab if you're leaving the focus trap. However, the big blocking failing of this solution remains to be solved: I think the approach will only work if the first and last tabbable elements in the container are recognizable by the library. For example, if the first element that's actually tabbable (according to the browser) is in an iframe or the shadow dom or is a selected radio button that's not the first one of it's group — then how is this library going to know that that's what should receive focus?

Open to solutions.

cc @doodirock

First of all, thank you.

A couple questions; Is it possible to add <summary>/<details> ? What about hidden html attribute ?

An effort among user-agent is underway to make scroll container tabbable. You can find whatwg/html#2851 and https://www.chromestatus.com/feature/5231964663578624 on this topic. On macOS Firefox 83.0, scroll containers are focusable, on Ubuntu 20.04 Chrome 87 too. Is this something you plan to support?

https://github.com/focus-trap/tabbable/blob/master/src/index.js#L193-L202

In concert with focus-trap/focus-trap#121

Blocked by dperini/nwsapi#83

Relates to #982

Would revert #995

Might want to make sure package-lock.json is also now using the nwsapi version that has the fix instead of being still configured for v2.2.2 (which is why #982 isn't visible unless you regenerate the lock file and NPM picks the current-latest problematic version of nwsapi).

Hi guys, sorry for leaving this here but I can't find any solution. I'm trying to mock tabbable to use with jest and I followed the instructions that you leaved but I'm getting an error "TypeError: tabbable.tabbable is not a function" when test runs.

My root for jest is defined in '/packages/core', so that is why I placed mock folder there. Any ideas where could be the issue?

It's possible for radio button names to contain characters that are invalid when used in a CSS selector (e.g. "). Below is an example that causes the tabbable function to throw an error due to an invalid selector constructed at:

HTML:

<div>
  <input type="radio" id="huey" name="&quot;duck&quot;" value="huey"
         checked>
  <label for="huey">Huey</label>
</div>

<div>
  <input type="radio" id="dewey" name="&quot;duck&quot;" value="dewey">
  <label for="dewey">Dewey</label>
</div>

JS:

import { tabbable } from 'tabbable';
// Throws DOMException: Document.querySelectorAll: 'input[type="radio"][name=""duck""]' is not a valid selector
tabbable(document.body);

It's also possible to inject values into the selector with a name like:

<input type="radio" id="dewey" name="&quot;] .some-class[data-attr=&quot;some-attribute" value="dewey">

A potential solution is CSS.escape, but it does require a polyfill for IE.

Hi David,
just a brief remark: According to your README file you are supporting IE9+. I noticed that since version 1.0.3 you are now using the Array.prototype.find Method in [1]. This will break in IE <= 11.

[1] index.js#L53

@stefcameron Still interested in re-writing the test suite with DOM Testing Library and Jest? (https://testing-library.com/docs/dom-testing-library/intro/)

I was going to jump on this soon if you still think it'd be a good idea. Let me know!

If you regenerate package-lock and run tabbable's unit tests, you get the error SyntaxError: 'slot):not([inert]' is not a valid selector
I've narrowed this down to the update of one of jsdom's dependencies: [email protected] to [email protected]
This breaks unit tests for any consumers of tabbable who install latest and use jsdom.

I'm not sure if the fix is re-writing the [tabindex]:not(slot):not([inert]) selector, or if this is a bug that nwsapi needs to fix. Please advise.

These come to mind: area, object.

I see contenteditable is impossible to implement correctly.

Tabbable does not select iframes it as candidate, and thus it is impossible to tab into them. This is needed in some scenarios, for instance when you have a reCaptcha (which is constructed as an iframe) in a form. One solution could be to just add 'iframe' to the candidateSelectors, but perhaps there is some issues with that?

It'd be nice to be able to pass more than one element to tabbable. The current tabbable doesn't allow us to easily find the relative tab orders of elements in multiple containers, if we were to call tabbable on each container.

Would it be reasonable to add this feature to tabbable? Happy to send a pull request if that's okay.

Example:

Suppose the numbers following "el" represent the tab orders of focusable the elements...

container1: el1, el4, el3
container2: el2, el5, el6

Calling tabbable on container1 and container2 would give us...

container1: el1, el3, el4
container2: el2, el5, el6

However, we wouldn't know how to combine the two lists of els, or know that el2 from container2 should be tabbed to before el3 from container1.

Proposal

Allow tabbable to accept multiple elements: tabbable(el1, el2, ...). tabbable returns an array of tabbable nodes in tab order from all of the passed elements/nodes.

Use cases

• Composite modals (ie. modals with multiple "dialogs").
• Non-fullscreen modals/overlays (e.g. overlay does not cover a top nav bar).

It is possible to assess whether an element, or one of its ancestors, have display: none without having to traverse the DOM.

This condition can be simplified to:

if (!displayCheck || displayCheck === 'full') {
  return !node.getClientRects().length;
}

I actually don't know if checking for getClientRects().length is really equivalent to that expensive while loop.

According to the specs, the getClientRects() method returns an empty list

If the element on which it was invoked does not have an associated layout box

What exactly are all the cases in which an element does not have an associated layout box? I am not sure.

But the e2e tests currently present in the project are all passing.
However I didn't go through the tests myself and I don't know what they do, I only checked this very small set of possible edge cases and it looks like the method should be good enough.

May I open a PR?

By the way, since the getBoundingClientRect() method internally uses getClientRects(), the "full" option would become faster than the "non-zero-area" one. But I guess that the latter would still be useful for accessibility purpose.

While it's possible to type callable signature with static properties in TS it would be better to change exports shape to default export + 2 named exports (which I mention also in #39 ).

It would also be better to use mixed exports, even if it means having to use require('tabbable').default by CJS consumers - because then TS typings would be accurate. Faking default with module.exports = _default is a can of worms and might cause issues in webpack.

Now that the test suite is converted over to use Jest and DOM Testing Library, what do you think about adding a code coverage badge to the README to show off the code coverage?

I see in the focus-trap-react repo we used Codecov along with GitHub Actions for the CI. In this tabbable repo it's also using GitHub Actions for the CI, so I assume we can generate the code coverage report there and then feed the results to Codecov, right? Or maybe even generate the badge without using Codecov if that's not something you want to do, and we could just use GitHub Actions directly?

Thoughts?

Dependencies haven't been updated in a while, and focus-trap, which depends on this module, uses Webpack now after focus-trap/focus-trap#124, so I'm also going to change the build system to Webpack.

I hope to address #51 while I'm at it.

Also along with focus-trap/focus-trap#124, and along with focus-trap/focus-trap-react#78, I'll be updating the ESLint and Prettier configuration to match. This way, all 3 repos now number the https://github.com/focus-trap "roof" will use the same stack and the same linting/formatting rules.

The project will move to Yarn since the other 2 repos use Yarn also.

Finally, I'll switch the CI to a GitHub Workflow, just like the other 2 repos are using now.

Hi,

I am not sure if this is related to a recent change of Firefox, but today I got this strange error:

After checking this relates to this line: https://github.com/focus-trap/tabbable/blob/master/src/index.js#L405

Apparently, Firefox seems to throw an exception whenever you access the disabled attribute on an element that does not implement it.

Chrome does not experience any issue, but this should probably be changed, by checking the attribute instead.

Thanks :)

jQuery UI isn't picking up on it either, but I think contenteditable elements should be tabbable. You can try here: codepen

The fix is easy, I think; just add [contenteditable]:not([contenteditable="false"]) to list of candidate selectors.

I'm building a UI library which exports a bunch of stuff, only some of my components rely on this package so it would be desirable that if a consumer of my package doesn't use component relying on tabbable it would be tree shaken out of the final bundle. However it's not the case right now.

Let's focus on 2 common tree-shaking deopts:

• no ESM bundle (this package) exports only CJS format
• top-level static properties, as in module.exports = tabbable; tabbable.isTabbable = isTabbable;

Action items:

• migrate source to ESM + build CJS format from it, point to CJS from package.json#main and to ESM from package.json#module so bundlers can pick appropriate file
• change exports shape to export default tabbable; export { isTabbable, isFocusable };

Intent to implement - yes (was already implemented in #38 )