Drupal.tabbingManager does not constrain tab navigation to the browser.

Really nice detective work!

All of my feedback was addressed. Thank you!

Having a magic boolean is not ideal, even for BC if we want to change that later on.

I updated the MR, so something like this is possible:

          Drupal.tabbingManager.constrain(
            document.querySelector('#tabbingmanager-test-container'),
            { trapFocus: true },
          );

forgot the default value, should be fine now.

Thank you @bnjmnm and @nod_! I think this still needs a CR informing developers about the new configuration option.

Committed ab86b36 and pushed to 9.3.x. Thanks!

I've just noticed the change record via RSS.

I'm very wary of this, and disappointed that the change was rushed through with so little discussion.

From the issue summary:

Discovered in #2158943: Replace jQuery UI dialog with supported library or polyfill, as the dialog replacement will require Drupal-provided focus management on dialogs. This was previously handled by jQuery UI.

Firstly (just in case) let's beware of conflating tabbingManager with dialogs. Dialogs are a mature pattern, which include other behaviours apart from tabbing constraint. Our tabbingManager is an oddity, with a mixed bag of uses, none of which are widespread, mature, or standardized.

Tabbingmanager successfully makes only a specified set of elements tabbable within the browser window. However, when the final element in the constrained test is focused then tabbed, focus leaves the browser entirely.

Also avoid conflating "reduce the set of tabbable controls" with "constrain tabbing inside a container". One doesn't imply a need for the other.

The issue summary doesn't mention any use cases for the new trapFocus option. Have you discovered any apart from dialogs?

focus leaves the browser entirely.

This is a bit vague. I think you mean the document, or perhaps the Window? (I'm unclear which of these objects myself.)

It doesn't leave the browser; it moves to one of the browser's built-in controls. This is very common behaviour, and it's been that way a long time, on both desktop and mobile browsers. Which control depends on which browser, browser configuration, whether any other browser tabs are open, and the keyboard tabbing direction. Focus may end up on the the browser's tab bar, back button, home button, address bar, bookmarks toolbar, status bar, or conceivably something else. A few of these have a keyboard shortcut to focus or activate them, but most don't. I myself frequently reach Firefox's bookmarks toolbar by shift-tabbing backwards out of a document.

The change record for the addition of tabbingManager seems to suggest this was the intended functionality. I suspect this wasn't spotted as tabbingManager is currently only used by contextual in a way where it's less likely to run into the bug.

I don't think it does, and I'm not inclined to call it a bug. In the case of the contextual module, my recollection is that we wanted to make it quicker to reach a particular block configuration, so the tabbing manager's job was to remove extraneous tab stops between the contextual buttons. There wasn't any need to prevent the user from leaving the document entirely, or constrain tabbing to any particular container. Rather, the tabbing manager is filtering out some controls from the tabbing order. We've since done a similar thing in layout builder, by disabling interactive elements in the block content.

I acknowledge there may be interest in making the focus trap optional.

That's a good call :-)

In between comments #6 and #7:

Are there valid use cases for using this without trapping focus? Wondering if we should have a follow-up to deprecate and remove this.

Yes. Tabbing outside of the document to reach the browser controls is long-standing behaviour in lots of browsers. We mustn't interfere with normal browser behaviour without an extraordinarily good reason. In particular, we should avoid putting obstacles in the way of reaching the browser's back button.

Trapping focus shouldn't be undertaken lightly. Generally, it's only called for when an operation requires modal behaviour, i.e. situations where the application needs some specific information or decision(s) from the user before allowing anything else to happen. This typically means modal dialogs, or acknowledging some alerts, but conceivably other "choose now, or abort" components too.

In any case, such components must also offer an mechanism for the user to abort the operation which trapped focus. The new trapFocus option doesn't (or can't) ensure this. Dialogs typically contain a close and/or cancel button (for pointers, switch access, speech control, keyboard), and can be dismissed with the escape key (if you can use a keyboard). These are the sort of "standard exit methods" referred to by WCAG SC 2.1.2 No Keyboard Trap.

Used carelessly, I think the trapFocus option could easily lead to failures of SC 2.1.2, which is serious. The API documentation and change record overlook this.

This makes #3230930: Consider making Tabbing Manager default to trapping focus a no-go in my view. I'll comment there too.

Note that many components (like a <select>, radio button group, or an ARIA tablist) involve managing focus or selection, but they don't prevent a user from tabbing away to somewhere outside the component.

Currently in core the only uses of tabbing manager are contextual links, and quickedit. Neither of these warrant confining focus within the document, in my view. They don't have the "choose something here, or abort" urgency or exclusivity of a modal dialog.

Stop trying to manage the user. It isn't a disaster if they merely overstep into their browser toolbar. This frequently happens when aiming for some header/footer links, and it's easy enough to just tab back into the page.

From the commit:

   *   When true, focus is trapped within the tabbable elements, i.e. focus will
   *   remain within the browser.

       *   When true, tabbing is trapped within the set of elements and can't
       *   leave the browser. 

When false, it is possible to tab out of the browser window

These documentation comments are misleading. I think it means the document, not the browser.

I spent the day chewing my thumbs, wondering whether this commit should be reverted. On balance, I don't think so...

• The trapFocus option looks like it might be very handy, but this issue doesn't provide many use cases. I think it would be fine for a dialog library to have tabbingManager as a dependency, on the understanding that the dialog library provides the required exit mechanisms for the user.
• However the trapFocus option is potentially very dangerous, and the tabbingManager is a public API. We ought to advise developers of this.

Next steps...

• Close #3230930: Consider making Tabbing Manager default to trapping focus as a won't-fix.
• Follow-up: Add some warnings to the API documentation. Not to be used lightly, provide a means of aborting, and refer to WCAG SC 2.1.2 No Keyboard Trap.
• Add similar advice to the change record for this issue. (Setting status to needs-work for this.)

Aside: I'd forgotten that tabbingManager had a facility for stacking contexts! That's something a lot of dialog libraries have overlooked, but it's part of the HTML dialog spec. I wonder how well it works?

Thank you for the thoughtful feedback @andrewmacpherson!

I've updated the issue summary and added credit for the review from @andrewmacpherson. I also opened follow-up for adding the same documentation to the API documentation: #3240554: Follow-up to #3229828: Add documentation for risks of using trapFocus option. Moving back to fixed, let's continue the discussion on the follow-up.