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

Provide context to modal setFocus method where focus is set on the close button #5856

Closed
nwhittaker opened this issue Nov 30, 2022 · 5 comments
Assignees
Labels
4 - verified Issues that have been released and confirmed resolved. ArcGIS Field Apps Issues logged by ArcGIS Field Apps team members. docs Issues relating to documentation updates only.

Comments

@nwhittaker
Copy link
Contributor

Actual Behavior

When opening a modal with disable-close-button disabled, the close button now always gains the initial focus.

If disable-close-button is enabled, the content slot is checked for focusable elements before the button slots, even if the buttons appear before the content slot in the DOM.

Expected Behavior

When opening a modal, the close button only gains the initial focus if there's nothing to focus in the content slot and/or there're no button slots to focus. The order in which slots are checked is dependent on the order in which they appear in the DOM.

Reproduction Sample

https://codepen.io/nwhittaker-esri/pen/jOKvKry

Reproduction Steps

  1. Visit the code sample
  2. Notice the close button gains initial focus despite the modal containing focusable content and buttons
  3. Enable disabled-close-button on the modal and see the input gains focus instead of the primary button

Reproduction Version

next.647

Relevant Info

Introduced by #5270.

Regression?

next.631

Impact

This regression is breaking auto-focus behaviors in our modals and causing integration tests to fail. Specifically in modals that we want to have a close button, but also want to initially auto-focus a button or some element in the content slot.

Esri team

ArcGIS Field Apps

@nwhittaker nwhittaker added bug Bug reports for broken functionality. Issues should include a reproduction of the bug. 0 - new New issues that need assignment. needs triage Planning workflow - pending design/dev review. labels Nov 30, 2022
@github-actions github-actions bot added the ArcGIS Field Apps Issues logged by ArcGIS Field Apps team members. label Nov 30, 2022
@nwhittaker nwhittaker added the regression Issues that are caused by changes in a release, but were working before that. label Nov 30, 2022
@benelan
Copy link
Member

benelan commented Dec 1, 2022

The outcome of #5147 will likely impact this issue. We are looking into using delegatesFocus to ensure more consistent behavior in line with web standards and a11y. I'd have to double check, but I believe the first focusable element in Modal is the close button, which would make this expected behavior. If you want to focus the slotted content, can you do that on your end since it is in light DOM?

cc @geospatialem for any a11y insights

@geospatialem
Copy link
Member

I'd have to double check, but I believe the first focusable element in Modal is the close button, which would make this expected behavior.

@benelan's note above is correct on the modal's close button as the first focusable element.

The change is a result of the #5270 introduction mentioned above, aligned with a11y standards and recommendations as the first focusable content in the component per WCAG Success Criterion 2.4.3: Focus Order.

The success criterion listed as Level A indicates the minimum set of a11y requirements to meet as many users will find it difficult or impossible to access information if not fulfilled.

If you want to focus the slotted content, can you do that on your end since it is in light DOM?

This is not a recommended approach since it would bypass the first focusable item and wouldn't align with what is depicted visually in the component.

Of note, there is also a timing mechanism associated with the setFocus to make it difficult for developers to set content as the focus.

@nwhittaker
Copy link
Contributor Author

The change is a result of the #5270 introduction mentioned above, aligned with a11y standards and recommendations as the first focusable content in the component per WCAG Success Criterion 2.4.3: Focus Order.

The success criterion listed as Level A indicates the minimum set of a11y requirements to meet as many users will find it difficult or impossible to access information if not fulfilled.

Agreed, but #5270 is a backwards compatibility break of the API because it deviates from the modal's documented setFocus() behavior: "By default, tries to focus on focusable content. If there is none, it will focus on the close button."1 On a related note, manually calling setFocus() on the modal no longer does anything unless the focusId is passed in.2

Understandable if this is more of a documentation clean-up issue, rather than a bug fix issue, though.

Footnotes

  1. https://developers.arcgis.com/calcite-design-system/components/modal/#component-api-methods-setFocus

  2. https://codepen.io/nwhittaker-esri/pen/QWxZdwO

@geospatialem geospatialem added docs Issues relating to documentation updates only. 1 - assigned Issues that are assigned to a sprint and a team member. and removed bug Bug reports for broken functionality. Issues should include a reproduction of the bug. 0 - new New issues that need assignment. needs triage Planning workflow - pending design/dev review. regression Issues that are caused by changes in a release, but were working before that. labels Dec 29, 2022
@geospatialem geospatialem self-assigned this Dec 29, 2022
@geospatialem geospatialem added this to the 2023 January Priorities milestone Dec 29, 2022
@geospatialem geospatialem changed the title Regression where modal close button is now always initially focused Provide context to modal setFocus method where focus is set on the close button Dec 29, 2022
geospatialem added a commit that referenced this issue Dec 29, 2022
**Related Issue:** #5856

## Summary
Add context to the `modal`s `setFocus` method where the focus will be
set on the component's "close" button.
@geospatialem geospatialem added 3 - installed Issues that have been merged to master branch and are ready for final confirmation. and removed 1 - assigned Issues that are assigned to a sprint and a team member. labels Dec 29, 2022
@github-actions
Copy link
Contributor

Installed and assigned for verification.

@geospatialem geospatialem added 4 - verified Issues that have been released and confirmed resolved. and removed 3 - installed Issues that have been merged to master branch and are ready for final confirmation. labels Jan 5, 2023
@geospatialem
Copy link
Member

Verified on the master branch's modal readme, which will be included in the January 2023 (1.0) release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4 - verified Issues that have been released and confirmed resolved. ArcGIS Field Apps Issues logged by ArcGIS Field Apps team members. docs Issues relating to documentation updates only.
Projects
None yet
Development

No branches or pull requests

3 participants