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

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

Merged

Conversation

Elijbet
Copy link
Contributor

@Elijbet Elijbet commented Oct 22, 2024

Related Issue: #10522

Summary

Close on Escape works when focusTrap is disabled.

@github-actions github-actions bot added the bug Bug reports for broken functionality. Issues should include a reproduction of the bug. label Oct 22, 2024
@Elijbet Elijbet changed the title fix(modal): close on escape works regardless of focus-trap fix(input-date-picker, modal, sheet): close on escape works regardless of focus-trap Oct 22, 2024
@Elijbet Elijbet changed the title fix(input-date-picker, modal, sheet): close on escape works regardless of focus-trap fix(input-date-picker, modal, sheet): close on escape works when of focusTrap is disabled Oct 22, 2024
@Elijbet Elijbet marked this pull request as ready for review October 23, 2024 00:16
@Elijbet Elijbet changed the title fix(input-date-picker, modal, sheet): close on escape works when of focusTrap is disabled fix(input-date-picker, modal, sheet): close on escape works when focusTrap is disabled Oct 23, 2024
@Elijbet Elijbet changed the title fix(input-date-picker, modal, sheet): close on escape works when focusTrap is disabled fix(input-date-picker, modal, sheet): closes on escape when focusTrap is disabled Oct 23, 2024
//
//--------------------------------------------------------------------------

@Listen("keydown", { target: "window" })
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of creating this event on the window, can we add it on the host of the component? That way, it won't close for an escape that happens outside of the component.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same thing with the modal component. Although less important since the modal is deprecated :D

Copy link
Contributor Author

@Elijbet Elijbet Oct 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I initially had it on host but @jcfranco thinks we should close the modal/sheet regardless of where esc occurs.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. However, when we refactor sheet to support #10364, this would need to move to the host when non modal. It might be better to just have it on sheets host for now. I'll defer to @jcfranco

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When a sheet can be non modal, we wouldn't want escape closing it from outside of the sheet.

Copy link
Member

@driskull driskull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Contributor

github-actions bot commented Nov 4, 2024

This PR has been automatically marked as stale because it has not had recent activity. Please close your PR if it is no longer relevant. Thank you for your contributions.

@github-actions github-actions bot added the Stale Issues or pull requests that have not had recent activity. label Nov 4, 2024
@Elijbet
Copy link
Contributor Author

Elijbet commented Nov 4, 2024

As per group discussion. @geospatialem @jcfranco @SkyeSeitz @ashetland @driskull

  • 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
  • sheet: offers focusTrapDisabled

    • Not enabled by default
    • We already offer this, so no action needed

@driskull
Copy link
Member

driskull commented Nov 4, 2024

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

I think for the future we should plan to deprecate focusTrapDisabled at some point and have these components no longer support any focus trapping. They should act like a single component and be controlled via arrow keys only so tabbing past them is easier for an end user.

Copy link
Contributor

This PR has been automatically marked as stale because it has not had recent activity. Please close your PR if it is no longer relevant. Thank you for your contributions.

@github-actions github-actions bot added the Stale Issues or pull requests that have not had recent activity. label Nov 15, 2024
@Elijbet Elijbet changed the title fix(input-date-picker, modal, sheet): closes on escape when focusTrap is disabled fix(input-date-picker, input-time-picker, modal, sheet): closes on escape when focusTrap is disabled Nov 22, 2024
@github-actions github-actions bot removed the Stale Issues or pull requests that have not had recent activity. label Nov 23, 2024
@Elijbet Elijbet added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Nov 26, 2024
Copy link
Member

@jcfranco jcfranco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, @Elijbet! Once comments are addressed, this is good to go! ✨🚀✨

@Elijbet Elijbet 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 27, 2024
@Elijbet
Copy link
Contributor Author

Elijbet commented Nov 27, 2024

Note: With this change it will only be closing on Escape when focus trap is disabled if the event is emitted within the component (i.e., Escape won't close if focus is in another, separate, element).

@Elijbet Elijbet 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 27, 2024
@Elijbet Elijbet merged commit 5acda5b into dev Nov 27, 2024
13 checks passed
@Elijbet Elijbet deleted the elijbet/10522-close-on-escape-works-regardless-of-focus-trap branch November 27, 2024 04:18
@github-actions github-actions bot added this to the 2024-12-17 - Dec Milestone milestone Nov 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug reports for broken functionality. Issues should include a reproduction of the bug. pr ready for visual snapshots Adding this label will run visual snapshot testing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants