Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

deprecate focusTrapDisabled property #8982

Closed
Elijbet opened this issue Mar 21, 2024 · 7 comments
Closed

deprecate focusTrapDisabled property #8982

Elijbet opened this issue Mar 21, 2024 · 7 comments
Labels
0 - new New issues that need assignment. docs Issues relating to documentation updates only. needs milestone Planning workflow - pending milestone assignment, has priority and/or estimate. refactor Issues tied to code that needs to be significantly reworked. spike complete Issues that have a research spike completed and dev work can proceed

Comments

@Elijbet
Copy link
Contributor

Elijbet commented Mar 21, 2024

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

@Elijbet Elijbet added docs Issues relating to documentation updates only. 0 - new New issues that need assignment. needs triage Planning workflow - pending design/dev review. labels Mar 21, 2024
@Elijbet Elijbet self-assigned this Mar 22, 2024
@Elijbet Elijbet added 1 - assigned Issues that are assigned to a sprint and a team member. and removed 0 - new New issues that need assignment. labels Mar 22, 2024
@Elijbet Elijbet changed the title docs(modal): deprecate focusTrapDisabled property bug(modal): deprecate focusTrapDisabled property Mar 22, 2024
@macandcheese
Copy link
Contributor

Should we do the same for Sheet?

@Elijbet
Copy link
Contributor Author

Elijbet commented Mar 22, 2024

Should we do the same for Sheet?

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

@Elijbet Elijbet changed the title bug(modal): deprecate focusTrapDisabled property bug(modal, sheet): deprecate focusTrapDisabled property Mar 22, 2024
@driskull
Copy link
Member

driskull commented Apr 9, 2024

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
Copy link
Member

driskull commented Jun 10, 2024

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.

@jcfranco jcfranco added the refactor Issues tied to code that needs to be significantly reworked. label Jul 16, 2024
@jcfranco jcfranco changed the title bug(modal, sheet): deprecate focusTrapDisabled property deprecate focusTrapDisabled property Jul 16, 2024
@geospatialem geospatialem added the spike Issues that need quick investigations for time estimations, prioritization, or a quick assessment. label Sep 26, 2024
@geospatialem
Copy link
Member

Spike needed to determine any refinements and considerations across components and the design system.

@geospatialem geospatialem added needs milestone Planning workflow - pending milestone assignment, has priority and/or estimate. and removed needs triage Planning workflow - pending design/dev review. labels Sep 26, 2024
@Elijbet
Copy link
Contributor Author

Elijbet commented Nov 5, 2024

As per group discussion we won't be addressing focusTrapDisabled on modal, as it's up for deprecation.

It also makes sense to offer support for focusTrapDisabled for sheet, as the intent of the components could differ. At least until we can collect more concrete examples or use cases pro or against that approach.

@Kitty @franco @skye @aaron @driskull

Also shared here: fix(input-date-picker, modal, sheet): closes on escape when focusTrap is disabled #10578

Next steps:

  • dialog with modal: no support for focusTrapDisabled

    • Technically we don't offer this in dialog currently so this is and once modal is removed we're good to go
  • dialog: offer focusTrapDisabled depending on workflows

    • New attr/prop, not enabled by default

    • Example of where this is useful, a cookie page dialog that mentions cookie tracking, but the user can still interact with the page without acknowledgement/acceptance

  • input-date-picker / input-time-picker: offer focusTrapDisabled depending on workflows

    • Not enabled by default

    • We already offer this, so no action needed if we agree on the approach

    • It could make sense depending on what the user is implementing in their UI/Ux

    • #10690 is a follow-up issue to determine next steps for input-date-picker and input-time-picker with regards to offering the ability to disable focusTrap.

@Elijbet Elijbet added spike complete Issues that have a research spike completed and dev work can proceed and removed spike Issues that need quick investigations for time estimations, prioritization, or a quick assessment. labels Nov 5, 2024
@github-actions github-actions bot added 0 - new New issues that need assignment. and removed 1 - assigned Issues that are assigned to a sprint and a team member. labels Nov 5, 2024
Copy link
Contributor

github-actions bot commented Nov 5, 2024

cc @geospatialem, @brittneytewks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0 - new New issues that need assignment. docs Issues relating to documentation updates only. needs milestone Planning workflow - pending milestone assignment, has priority and/or estimate. refactor Issues tied to code that needs to be significantly reworked. spike complete Issues that have a research spike completed and dev work can proceed
Projects
None yet
Development

No branches or pull requests

5 participants