refactor(modal): Update modal to use focus-trap module. #5676

driskull · 2022-11-01T19:20:05Z

Related Issue: #1169 #5270

Summary

refactor(modal): Update modal to use focus-trap module.

Replaces outdated @a11y/focus-trap module with more popular and supported focus-trap module.
https://www.npmjs.com/package/focus-trap
https://www.npmjs.com/package/@a11y/focus-trap

Precursor to #2133

driskull · 2022-11-01T19:21:47Z

@macandcheese @geospatialem @jcfranco could you play around with this and let me know if its working correctly?

src/components/modal/modal.tsx

macandcheese · 2022-11-03T17:24:04Z

Seems to work well! The "tab fencing" demo doesn't seem to work as described, but that may just be out of date sample content. Is it still possible to specify an element to focus first?

driskull · 2022-11-03T17:25:09Z

Is it still possible to specify an element to focus first?

Yes, that's possible with the module. Do we need to support that for modal? I didn't see anything like that.

https://github.com/focus-trap/focus-trap#createoptions initialFocus.

macandcheese · 2022-11-03T17:32:29Z

Yes, that's possible with the module. Do we need to support that for modal? I didn't see anything like that.

https://github.com/focus-trap/focus-trap#createoptions initialFocus.

Just going off the existing content in the "tab fencing" example modal. That may just be out of date demo - we already have setFocus available

driskull · 2022-11-03T23:05:54Z

From what I understand, it's ideal to focus on the first focusable element when a component's focus/setFocus method is called.

In most cases, this will be the close button but if that is disabled it would go to the next focusable element which could be some modal content or if no focusable modal content then any footer focusable content.

If we're following the rules, we shouldn't prioritize modal "content" over whatever the first focusable element is.

So that demo wording should be updated.

Ideally, we remove any options on setFocus. A user shouldn't be allowed or have to worry about focusing a specific element within the component. The component should handle that by focusing the first focusable element. In some components this will be a ranging focus element like the last selected radio button.

jcfranco · 2022-11-04T17:19:45Z

@driskull We should also confirm if this impacts the a11y/design of things. IMO, for closable dialogs with interactive content, as a keyboard user I would expect to be able to interact with the content without an extra keypress to move focus. Note that I'm not vouching for setFocus options, but we'll need to find another approach if focusing the close button in all cases isn't ideal and we don't want to provide focus IDs and keep the same design. cc @ashetland @SkyeSeitz @geospatialem

FWIW, @benelan was looking into this topic for #5147.

If we're following the rules, we shouldn't prioritize modal "content" over whatever the first focusable element is.

Which rules are you referring to here?

ashetland · 2022-11-04T17:31:53Z

@driskull We should also confirm if this impacts the a11y/design of things. IMO, for closable dialogs with interactive content, as a keyboard user I would expect to be able to interact with the content without an extra keypress to move focus. Note that I'm not vouching for setFocus options, but we'll need to find another approach if focusing the close button in all cases isn't ideal and we don't want to provide focus IDs and keep the same design. cc @ashetland @SkyeSeitz @geospatialem

FWIW, @benelan was looking into this topic for #5147.

If we're following the rules, we shouldn't prioritize modal "content" over whatever the first focusable element is.

Which rules are you referring to here?

I agree with @jcfranco here regarding interactive content. I like to think of this in terms of primary action. When the dialog appears, what's my primary action? If there is interactive content, my primary action would be to interact with that content before I close the dialog.

driskull · 2022-11-04T18:04:50Z

Which rules are you referring to here?

W3C guidelines for modal, but they are all pretty similar regarding focus.

https://www.w3.org/WAI/ARIA/apg/example-index/dialog-modal/dialog

Note that I'm not vouching for setFocus options, but we'll need to find another approach if focusing the close button in all cases isn't ideal and we don't want to provide focus IDs and keep the same design

I agree its not ideal in all cases, but in those cases the user is slotting the content and the footer buttons so they can always focus on one of those once the modal is opened. I don't think we need setFocus options because of that. I don't think there are any modal shadowRoot elements that need to specifically be focused.

I agree with @jcfranco here regarding interactive content. I like to think of this in terms of primary action. When the dialog appears, what's my primary action? If there is interactive content, my primary action would be to interact with that content before I close the dialog.

For these cases, we wouldn't be able to use any setFocus options to determine a primary action anyway. The author of the modal would have to focus on the primary action once the modal is opened. Which they can do because they are slotting the primary actions and those actions are in light DOM.

driskull · 2022-11-04T18:17:57Z

Just to be clear, the only thing changing is we focus the close-button by default now because that's what the first focusable element in the component is.

…his already.

driskull · 2022-11-04T21:09:47Z

If we want to provide an option in the future for which element to focus by default, we could do that with setFocus by passing in the element or some selector.

https://github.com/focus-trap/focus-trap#what-it-does

When a focus trap is activated, this is what should happen:

Some element within the focus trap receives focus. By default, this will be the first element in the focus trap's tab order (as determined by tabbable). Alternately, you can specify an element that should receive this initial focus.

benelan · 2022-11-07T18:08:19Z

It seems like focus-trap could be another option. Although I'd prefer to go with delegatesFocus if everything pans out, since it's spec.

driskull · 2022-11-07T18:16:44Z

It seems like focus-trap could be another option. Although I'd prefer to go with delegatesFocus if everything pans out, since it's spec.

delegatesFocus and focus-trap are two different topics.

delegatesFocus: when a non-focusable part of the shadow DOM is clicked, the first focusable part is given focus, and the shadow host is given any available :focus styling.
focusTrap: Trap focus within a DOM node.

benelan · 2022-11-07T18:25:54Z

Ah, thanks for clarifying. I thought focus-trap was being suggested as an alternative.

driskull · 2022-11-07T18:41:38Z

Ah, thanks for clarifying. I thought focus-trap was being suggested as an alternative.

Nope! but ideally we should be consistent with how delegatesFocus works (by focusing first focusable element). Which focus-trap does.

geospatialem · 2022-11-07T18:46:35Z

This looks great, @driskull! 🌟 Concur with your thoughts on the :focus placement for the close button, in particular around a11y.

If folks think the ordering seems odd with the close button being the first focusable element we could consider moving the close button to the footer by default. It would be a new pattern for the component so we'd need some design buy in for it, but perhaps an option if folks would rather see focusable elements from the content in focus first.

benelan · 2022-11-07T21:37:03Z

If folks think the ordering seems odd with the close button being the first focusable element we could consider moving the close button to the footer by default.

I'm not design, so take this with a grain of salt, but I don't think we should do this. If we do, we would probably need to change the "X" to "Close". I don't think I've ever seen a close "X" button in the footer, they are also in the top right. Imo changing that would cause more confusion/annoyance for users than whatever the first focusable element is.

macandcheese · 2022-11-07T21:43:21Z

Many Modals won't have footers either, so the close button as currently situated makes the most sense IMO. I think the focus on the close button should be expected as we've done that for awhile.

driskull · 2022-11-07T21:56:38Z

Many Modals won't have footers either, so the close button as currently situated makes the most sense IMO. I think the focus on the close button should be expected as we've done that for awhile.

Correct. We've gotten feedback from a11y team that focusing close button on panel first is correct. This should align with that.

jcfranco · 2022-11-15T20:25:55Z

I think the focus on the close button should be expected as we've done that for awhile.
Correct. We've gotten feedback from a11y team that focusing close button on panel first is correct. This should align with that.

Fair enough. As long as this is design/a11y-approved and we are consistent.

Just curious, but is there an a11y example out in the wild that has a close button similar to where we place ours?

If we want to provide an option in the future for which element to focus by default, we could do that with setFocus by passing in the element or some selector.
https://github.com/focus-trap/focus-trap#what-it-does

I like this. We can revisit supporting this later based on user feedback.

jcfranco

Awesome stuff!

jcfranco · 2022-11-15T20:28:27Z

src/components/modal/modal.tsx

-    return focusElement(
-      focusId === "close-button" ? closeButton : getFocusableElements(this.el)[0] || closeButton
-    );
+    if (closeButtonEl && focusId === "close-button") {


Since close-button is the only supported ID, I think you could just check for closeButtonEl being present or not. Passing any other ID would technically be invalid, right?

Correct. I think its nicer to check and fallback incase the user entered an invalid ID>

jcfranco · 2022-11-15T20:29:39Z

src/tests/commonTests.ts

+  /**
+   * Used to delay setFocus call in ms.
+   */
+  delay?: number;


If this is for the open/close transition, I'd suggest using the skipAnimation helper instead of introducing a delay argument. If it's something specific to focus-trap behavior, can you add more info about this to the documentation?

@jcfranco the problem is the delayInitialFocus is set to true.

I tried delayInitialFocus: !Build.isTesting, but the problem is that its not returning true.

{"testing":false,"browser":true,"dev":false,"server":false} Location: http://localhost:3333/build/p-371d7086.entry.js:15:2306

src/utils/focusTrapComponent.ts

# Conflicts: # package-lock.json

* master: (24 commits) 1.0.0-next.632 feat(pick-list, value-list): Add calciteListFilter event, filteredItems prop, filterText prop and filteredData prop. (#5681) fix(block, date-picker, list-item-group, panel, pick-list-group, popover, tip, tip-manager)!: Set default internal heading to a div. (#5728) refactor(modal): Update modal to use focus-trap module. (#5676) 1.0.0-next.631 fix(input-date-picker): restores mouse clicks on date-picker popup (#5760) build(deps): bump loader-utils from 1.4.1 to 1.4.2 (#5764) ci(product-label): fix version syntax and use os agnostic newline char (#5762) 1.0.0-next.630 ci(pr-bot): don't label dependabot PRs (#5759) build(deps): bump chromatic from 6.7.1 to 6.11.4 (#5756) fix(combobox): Wrap and break text on long items (#5672) build(deps): bump @storybook/addon-interactions from 6.5.9 to 6.5.13 (#5753) build(deps): bump type-fest from 3.1.0 to 3.2.0 (#5752) build(deps): bump @storybook/addon-a11y from 6.5.12 to 6.5.13 (#5754) build(deps): bump postcss from 8.4.18 to 8.4.19 (#5755) ci: add chromatic (#5733) build(docs): generate docs-json for afd usage (#5748) 1.0.0-next.629 fix(inline-editable): Add text-ellipsis when not editing (#5679) ...

refactor(modal): Update modal to use focus-trap module.

2355312

github-actions bot assigned driskull Nov 1, 2022

driskull commented Nov 1, 2022

View reviewed changes

src/components/modal/modal.tsx Outdated Show resolved Hide resolved

driskull added 6 commits November 1, 2022 13:14

cleanup

b31109c

cleanup

4a6b6c0

fix test

104492f

cleanup

462d81a

cleanup

91bf8f3

Merge branch 'master' into dris0000/modal-focus-trap-npm

599fdc4

driskull marked this pull request as ready for review November 3, 2022 16:55

driskull requested a review from a team as a code owner November 3, 2022 16:55

remove previousActiveElement internal prop. focus utility handles t…

2a98931

…his already.

set fallbackFocus element.

b51bfda

github-actions bot added the refactor Issues tied to code that needs to be significantly reworked. label Nov 9, 2022

driskull added 2 commits November 9, 2022 14:35

cleanup

ba6c2e8

cleanup

886cde8

driskull requested review from alisonailea, anveshmekala, eriklharper and benelan November 14, 2022 18:44

Merge branch 'master' into dris0000/modal-focus-trap-npm

cd3ca2d

driskull added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Nov 14, 2022

jcfranco approved these changes Nov 15, 2022

View reviewed changes

driskull added 3 commits November 15, 2022 13:31

review feedback

6a607e4

add spec test

5c75164

Merge branch 'master' into dris0000/modal-focus-trap-npm

464e287

# Conflicts: # package-lock.json

driskull added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Nov 15, 2022

driskull added 6 commits November 15, 2022 16:08

cleanup

bdb7c4b

cleanup

533c136

Merge branch 'master' into dris0000/modal-focus-trap-npm

8dd9662

cleanup

9f6e99e

cleanup

4b17306

cleanup

1abe776

driskull merged commit 059d6ee into master Nov 16, 2022

driskull deleted the dris0000/modal-focus-trap-npm branch November 16, 2022 17:34

github-actions bot added this to the Sprint 2022/11/07 - 2022/11/18 milestone Nov 16, 2022

nwhittaker mentioned this pull request Dec 2, 2022

Infinite loop when calling setFocus on new calcite-input #5882

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(modal): Update modal to use focus-trap module. #5676

refactor(modal): Update modal to use focus-trap module. #5676

driskull commented Nov 1, 2022 •

edited

Loading

driskull commented Nov 1, 2022

macandcheese commented Nov 3, 2022

driskull commented Nov 3, 2022 •

edited

Loading

macandcheese commented Nov 3, 2022

driskull commented Nov 3, 2022 •

edited

Loading

jcfranco commented Nov 4, 2022

ashetland commented Nov 4, 2022

driskull commented Nov 4, 2022

driskull commented Nov 4, 2022

driskull commented Nov 4, 2022

benelan commented Nov 7, 2022

driskull commented Nov 7, 2022 •

edited

Loading

benelan commented Nov 7, 2022

driskull commented Nov 7, 2022

geospatialem commented Nov 7, 2022

benelan commented Nov 7, 2022

macandcheese commented Nov 7, 2022 •

edited

Loading

driskull commented Nov 7, 2022

jcfranco commented Nov 15, 2022

jcfranco left a comment

jcfranco Nov 15, 2022

driskull Nov 15, 2022

jcfranco Nov 15, 2022

driskull Nov 15, 2022

refactor(modal): Update modal to use focus-trap module. #5676

refactor(modal): Update modal to use focus-trap module. #5676

Conversation

driskull commented Nov 1, 2022 • edited Loading

Summary

driskull commented Nov 1, 2022

macandcheese commented Nov 3, 2022

driskull commented Nov 3, 2022 • edited Loading

macandcheese commented Nov 3, 2022

driskull commented Nov 3, 2022 • edited Loading

jcfranco commented Nov 4, 2022

ashetland commented Nov 4, 2022

driskull commented Nov 4, 2022

driskull commented Nov 4, 2022

driskull commented Nov 4, 2022

benelan commented Nov 7, 2022

driskull commented Nov 7, 2022 • edited Loading

benelan commented Nov 7, 2022

driskull commented Nov 7, 2022

geospatialem commented Nov 7, 2022

benelan commented Nov 7, 2022

macandcheese commented Nov 7, 2022 • edited Loading

driskull commented Nov 7, 2022

jcfranco commented Nov 15, 2022

jcfranco left a comment

Choose a reason for hiding this comment

jcfranco Nov 15, 2022

Choose a reason for hiding this comment

driskull Nov 15, 2022

Choose a reason for hiding this comment

jcfranco Nov 15, 2022

Choose a reason for hiding this comment

driskull Nov 15, 2022

Choose a reason for hiding this comment

driskull commented Nov 1, 2022 •

edited

Loading

driskull commented Nov 3, 2022 •

edited

Loading

driskull commented Nov 3, 2022 •

edited

Loading

driskull commented Nov 7, 2022 •

edited

Loading

macandcheese commented Nov 7, 2022 •

edited

Loading