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

Modal/ Flyout - Add returnFocusTo argument for consumer-side focus management on close #2497

Merged
merged 5 commits into from
Oct 14, 2024

Conversation

didoo
Copy link
Contributor

@didoo didoo commented Oct 11, 2024

📌 Summary

While working on the HdsPlus::ChallengeModal one argument that is missing from our Hds::Modal, looking at the implementation of similar components in our consumers' codebases, is the returnFocusTo (see here).

As you can see from the attached video, the focus could be "lost" once the <dialog> element is closed (it's returned to the body) if the original element that opened the modal is deleted from the DOM (eg. think of a modal opened via a dropdown interactive item).

The intent of this PR is to introduce, in our Modal and Flyout components, a similar functionality to allow consumers to control where the focus is returned, if the element that initiated the event is not in the DOM anymore once closed.

🛠️ Detailed description

In this PR I have:

  • added returnToFocus argument and logic to Modal and Flyout components
  • added demos to Modal and Flyout showcase pages where the modal is opened via Dropdown
    • added ember-set-helper as dev-dependency for the showcase
  • added more focus-management integration tests for Modal and Flyout components

Notice: I'll add the documentation update in a separate PR, if we agree this is the way forward.

👀 Previews:

🎬 Video

You can see how the focus is returned to the body once the Modal is closed (look at the document.activeElement live variable in the devtools on the right)

focus.mov

👀 Component checklist

  • Percy was checked for any visual regression
  • A changeset was added

💬 Please consider using conventional comments when reviewing this PR.

@didoo didoo requested review from alex-ju, MelSumner and a team October 11, 2024 14:35
Copy link

vercel bot commented Oct 11, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
hds-showcase ✅ Ready (Inspect) Visit Preview Oct 11, 2024 2:48pm
hds-website ✅ Ready (Inspect) Visit Preview Oct 11, 2024 2:48pm


// Return focus to a specific element (if provided)
if (this.args.returnFocusTo) {
const initiator = document.getElementById(this.args.returnFocusTo);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am using getElementById which means that if consumers want to use the returnFocusTo argument, they have to assign an id to the element they want to return the focus to.

The reasoning for not being less "strict" and use something like querySelector is that one would be tempted to use a CSS class name (which may not be unique in the page) or a data attribute (but if you can assign a data attribute, you can also assign an id)

Can you think of cases where they would not be able to assign an ID to the target element, or the ID is dynamically generated?

Copy link
Contributor

Choose a reason for hiding this comment

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

I also prefer the approach of using an id here. We can have folks try it out, I'm sure they'll tell us if they can't, but I don't know of any immediate reason why they wouldn't be able to do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can have folks try it out, I'm sure they'll tell us if they can't

That's what they are using in Cloud UI, so I think it should work OK: https://github.com/hashicorp/cloud-ui/blob/main/addons/core/package/src/components/modal-dialog.ts#L236-L241

Copy link
Contributor

@MelSumner MelSumner left a comment

Choose a reason for hiding this comment

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

This is a great addition and will be very helpful for our consumers. 👍

@didoo
Copy link
Contributor Author

didoo commented Oct 11, 2024

This is a great addition and will be very helpful for our consumers. 👍

@MelSumner thanks! can you check in the showcase pages that it meets the accessibility requirements and expectations? thanks 🙏

@MelSumner
Copy link
Contributor

Visit Preview

Ahh yes, I should have said; I did visit the showcase page and the focus with the return is doing what it should be doing. 👍

Copy link
Contributor

@KristinLBradley KristinLBradley 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. Seems like a worthwhile and useful update to me.

@didoo
Copy link
Contributor Author

didoo commented Oct 14, 2024

@alex-ju I'll wait for you OK/green light before merging (don't know if/when we want to release these changes. Or even better, please merge it yourself if/when it's OK for you.

@alex-ju
Copy link
Member

alex-ju commented Oct 14, 2024

@alex-ju I'll wait for you OK/green light before merging (don't know if/when we want to release these changes. Or even better, please merge it yourself if/when it's OK for you.

we already have a feature in the pipeline, so we can get this one in anytime

@alex-ju alex-ju merged commit 6b6c22e into main Oct 14, 2024
14 checks passed
@alex-ju alex-ju deleted the modal-flyout-return-focus-to branch October 14, 2024 09:11
@hashibot-hds hashibot-hds mentioned this pull request Oct 14, 2024
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.

6 participants