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(ComposedModal): fix focus trap and refocus to trigger element when closed #11418

Conversation

annawen1
Copy link
Member

@annawen1 annawen1 commented May 16, 2022

Closes

Gets changes from #11350 and #11390 into the v10 branch.

RPReplay_Final1652723745.mov

Changelog

Changed

  • wrap children in another div to get focus wrapping to work
  • set aria-hidden when modal is closed to prevent VO from reading out modal content when closed
  • pass logic to re-focus to trigger element onClose in the ComposedModal story

Testing / Reviewing

test using VO on iOS

  • go to ComposedModal, and swipe through modal content, making sure focus is trapped within modal
  • also to go State Manager story to ensure when modal is closed, focus goes back to the "Launch composed modal" button

@annawen1 annawen1 marked this pull request as ready for review May 16, 2022 17:58
@annawen1 annawen1 requested a review from a team as a code owner May 16, 2022 17:58
@annawen1 annawen1 requested review from tay1orjones and joshblack and removed request for a team May 16, 2022 17:58
@@ -108,6 +108,10 @@
@include breakpoint(xlg) {
width: 48%;
}

.#{$prefix}--modal-container-body {
Copy link
Member Author

@annawen1 annawen1 May 16, 2022

Choose a reason for hiding this comment

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

@tay1orjones should the new styles be added to both packages/styles/scss/components/modal/_modal.scss and packages/components/src/components/modal/_modal.scss?

Copy link
Contributor

Choose a reason for hiding this comment

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

You may be able to get away with only packages/components in v10 at this point since the styles package is private, curious what you think @tay1orjones 👀 Definitely doesn't hurt having it in both places though (especially if that makes porting things back easier)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, same thought. Having the same in both spots is preferred/nice but not necessary.

@@ -106,6 +106,10 @@
@include carbon--breakpoint(xlg) {
width: 48%;
}

.#{$prefix}--modal-container-body {
display: contents;
Copy link
Contributor

Choose a reason for hiding this comment

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

Just wanted to double-check, do we need to offer any fallbacks for IE11 support or should this work as expected?

Copy link
Member Author

Choose a reason for hiding this comment

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

@joshblack oh I didn't think about that, are you guys still supporting IE11? the dotcom team has dropped IE from our supported browser list.

Copy link
Contributor

Choose a reason for hiding this comment

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

@annawen1 it shouldn't be a problem in v11 but technically we do still support it for v10. If the file is already using non-IE11 supported features though it's definitely okay to ship as-is which is why I was curious if that was already in place

Copy link
Member

@tay1orjones tay1orjones left a comment

Choose a reason for hiding this comment

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

Looks good pending the comments from @joshblack 👍

Once merged, does this need to go out in a v10 patch now, or can it wait until our release next week?

@annawen1
Copy link
Member Author

Looks good pending the comments from @joshblack 👍

Once merged, does this need to go out in a v10 patch now, or can it wait until our release next week?

@tay1orjones I think we can sit on it till the release next week!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants