[Modal] Focus isn't trapped in the modal when the primary action is disabled

[acmertz]

Issue summary

If a Modal has a primary action with disabled: true, focus isn't trapped in the modal when tabbing over focusable elements.

Expected behavior

Focus wraps around to the first focusable element inside the Modal when tabbing away from the last focusable element.

Actual behavior

User can tab out of the Modal if the primary action is disabled.

https://screenshot.click/polaris-modal-broken-focus-trap.mp4

Steps to reproduce the problem

1. Create a Modal dialog with a disabled primary action
2. Open the modal. Focus should automatically move to the first focusable item within the modal.
3. Start hitting the Tab key on your keyboard. Once the last focusable item before the disabled primary input receives focus, pressing Tab again will move focus outside of the modal to the next focusable item on the rest of the page.

Reduced test case

Specifications

• Are you using the React components? (Y/N): Y
• Polaris version number: v10.10.1
• Browser: Chrome v107
• Device: MacBook Pro
• Operating System: macOS Ventura 13.0

[acmertz]

I traced this down to the findLastKeyboardFocusableNode() helper function in focus.ts, which uses this selector to identify keyboard-focusable elements:

a,frame,iframe,input:not([type=hidden]):not(:disabled),select:not(:disabled),textarea:not(:disabled),button:not(:disabled),*[tabindex]:not([tabindex="-1"])

I discovered that the Polaris <Button> component does not set the disabled attribute on the underlying <button> element when the disabled prop is passed. This causes disabled buttons to be considered keyboard-focusable by findLastKeyboardFocusableNode() because they match the button:not(:disabled) portion of the above selector. TrapFocus only redirects focus to the first focusable element in the modal when receiving a "tab" keyboard event from the last focusable element, but since tabindex="-1" is set on the button it can never receive focus - causing it to be skipped in the tab order and allowing focus to move outside the Modal.

I can see two potential solutions to this issue:

1. Add the disabled attribute to disabled <Button> components (preferred)

or:

2. Update the selector used in focus.ts so it stops matching buttons that aren't disabled but still have tabindex="-1".