search

Esri / calcite-design-system

A monorepo containing the packages for Esri's Calcite Design System
https://developers.arcgis.com/calcite-design-system/
Other
282 stars 77 forks source link

deprecate `focusTrapDisabled` property #8982

Open Elijbet opened 5 months ago

Elijbet commented 5 months ago

Description

Deprecate the focusTrapDisabled property.

Modal and Sheet add a scrim that makes it difficult to see where we are tabbing outside of these components. Removing the prop would help avoid this issue.

The prop was introduced for consistency, but there seems to be no clear reason why the focus-trap would need to be disabled on modal or sheet.

Related to: 6456

Which Component

modal, sheet

Resources

No response

macandcheese commented 5 months ago

Should we do the same for Sheet?

Elijbet commented 5 months ago

Should we do the same for Sheet?

Looks like it's the same visibility issue with the scrim, so yeah.

driskull commented 5 months ago

I think we should do the same for popover, and components that uses popover internally. Ideally, all of our components use focus trapping.

If we deprecate this property, we can rely on focus-trap (trapStack) to handle the stack for our "open" components. We could also use the escapeDeactivates property on focus-trap to handle closing the correct component when the escape key is pressed or alternatively we can use the stack to handle closing the correct component ourselves. Since our component stack is already a public config, it allows other component libraries to share the same stack.

cc @jcfranco

driskull commented 3 months ago

Actually, we still need focusTrapDisabled for popover because that component is used internally for the action-menu in cases where its using active-descendant and shouldn't receive focus. So there are still some internal use cases for disabling the focus trap.

Maybe we should keep this prop for any component that would be used inside another.