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

Popover - When clicked outside of a focus trap, focus is not returned to the element prior to the focus trap #10682

Open
2 of 5 tasks
annelfitz opened this issue Nov 4, 2024 · 7 comments
Assignees
Labels
1 - assigned Issues that are assigned to a sprint and a team member. a11y Issues related to Accessibility fixes or improvements. ArcGIS Maps SDK for JavaScript Issues logged by ArcGIS SDK for JavaScript team members. bug Bug reports for broken functionality. Issues should include a reproduction of the bug. calcite-components Issues specific to the @esri/calcite-components package. estimate - 5 A few days of work, definitely requires updates to tests. impact - p3 - not time sensitive User set priority impact status of p3 - not time sensitive p - medium Issue is non core or affecting less that 60% of people using the library spike Issues that need quick investigations for time estimations, prioritization, or a quick assessment.

Comments

@annelfitz
Copy link

annelfitz commented Nov 4, 2024

Check existing issues

Summary

When the esc key is pressed when the calcite popover is open, it will always close the popover, even if the focus is elsewhere in the web page.

When clicked outside of the focus trap (like on the map for example), we would expect the esc key to no longer affect the popover, at least until the focus is returned to that element.

Ideal behavior:

  • When escape is pressed, focus is returned to the element prior to the focus trap
  • When clicked outside of a focus trap, focus is not returned to the element prior to the focus trap

Currently, in both of the scenarios above, focus is returned to the element prior to the focus trap. It seems like we may want different things depending on if the focus trap is deactivated due to a click vs escape key press.

Actual Behavior

The esc key will always close the popover, even if the focus is no longer on the popover.
The focus will always return to the element prior to the focus trap, even if it is deactivated due to a click elsewhere.

Expected Behavior

When clicked outside of a focus trap, the focus should not be returned to the element prior to the focus trap.

Reproduction Sample

https://codepen.io/driskull/pen/zYgaNQP

Reproduction Steps

  1. open the popover
  2. press escape
  3. notice the button is focused
  4. open the popover
  5. click outside of the popover
  6. notice the button is focused
  7. click again outside of the popover and the focus on the button is now gone.

Reproduction Version

2.13.2

Working W3C Example/Tutorial

No response

Relevant Info

related JS SDK issue: https://devtopia.esri.com/WebGIS/arcgis-js-api/issues/66350#issuecomment-5110811

Regression?

No response

Priority impact

impact - p3 - not time sensitive

Calcite package

  • @esri/calcite-components
  • @esri/calcite-components-react
  • @esri/calcite-design-tokens
  • @esri/eslint-plugin-calcite-components

Esri team

ArcGIS Maps SDK for JavaScript

@annelfitz annelfitz added 0 - new New issues that need assignment. a11y Issues related to Accessibility fixes or improvements. bug Bug reports for broken functionality. Issues should include a reproduction of the bug. needs triage Planning workflow - pending design/dev review. p - high Issue should be addressed in the current milestone, impacts component or core functionality labels Nov 4, 2024
@github-actions github-actions bot added ArcGIS Maps SDK for JavaScript Issues logged by ArcGIS SDK for JavaScript team members. calcite-components Issues specific to the @esri/calcite-components package. impact - p3 - not time sensitive User set priority impact status of p3 - not time sensitive labels Nov 4, 2024
@driskull
Copy link
Member

driskull commented Nov 5, 2024

The way our focus trapping works is that when the focus trap is deactivated via click outside or escape, focus is returned to the element focused prior to opening the focus-trap. In this case, this would be the expand button.

I think what Maps SDK is asking for is that clicks outside deactivate the focus trap but do not return focus to the element prior to focus trapping. But when hitting escape, it does return focus to the element previously focused.

I think we may need a spike and do some research here.

@hcampos-professional
Copy link

I think what Maps SDK is asking for is that clicks outside deactivate the focus trap but do not return focus to the element prior to focus trapping

This could be conditional on whether the focus was explicitly moved to a different element, if we can detect or signal that. In the case of the JSAPI, the problem happens when the focus is moved to the view but then reset back to the expand button. If there was a way to detect that the focus switched to the view, then we could stop the default focus trap behavior of going back to expand button.

@driskull
Copy link
Member

driskull commented Nov 7, 2024

This could be conditional on whether the focus was explicitly moved to a different element

Yeah we can probably do this.

@geospatialem
Copy link
Member

May be resolved by #10522, blocking to confirm after the efforts are completed.

@geospatialem geospatialem added blocked This issue is blocked by another issue. and removed needs triage Planning workflow - pending design/dev review. labels Nov 12, 2024
@geospatialem geospatialem added this to the Stalled milestone Nov 12, 2024
Copy link
Contributor

Issue #10522 has been closed, this issue is ready for re-evaluation.

cc @geospatialem,@DitwanP

@github-actions github-actions bot removed the blocked This issue is blocked by another issue. label Nov 27, 2024
@geospatialem
Copy link
Member

Issue #10522 has been closed, this issue is ready for re-evaluation.

cc @geospatialem,@DitwanP

This was not addressed in the fix mentioned above, will work towards a resolution in an upcoming milestone.

@geospatialem geospatialem added the needs triage Planning workflow - pending design/dev review. label Nov 27, 2024
@geospatialem geospatialem removed this from the Stalled milestone Nov 27, 2024
@geospatialem geospatialem added p - medium Issue is non core or affecting less that 60% of people using the library estimate - 5 A few days of work, definitely requires updates to tests. needs milestone Planning workflow - pending milestone assignment, has priority and/or estimate. and removed p - high Issue should be addressed in the current milestone, impacts component or core functionality needs triage Planning workflow - pending design/dev review. labels Dec 18, 2024
@DitwanP DitwanP removed the needs milestone Planning workflow - pending milestone assignment, has priority and/or estimate. label Jan 13, 2025
jcfranco added a commit that referenced this issue Feb 6, 2025
…cus-trap (#11453)

**Related Issue:** #11345 

## Summary

Adds `focusTrapOptions` option to allow additional customization of
focus-trapping components. It supports the following options from
https://github.com/focus-trap/focus-trap#createoptions:

* `initialFocus` – would replace popover's internal
initialFocusTrapFocus prop
* `allowOutsideClick` – supports
#10682
* `returnFocusOnDeactivate` – supports
#10682

And the following custom prop:

* `extraContainers` – allows specifying extra elements (nodes or
selectors) to focus trap (e.g., anything appending to the body, etc)
when creating/activating the trap. **Note**: if specified, elements must
exist when the focus trap is activated, if extra containers are created
afterwards, users can use `updateFocusTrapElements(extraContainers)`

This also enhances `updatesFocusTrapElements()` to accept extra
containers to allow in the focus trap if these are created after the
trap is activated.

### Notes

* A subset of https://github.com/focus-trap/focus-trap#createoptions
options are exposed as certain configurations might break component
functionality.
* `extraContainers` gets used both when creating and updating the focus
trap target containers
* Tidies up types
benelan pushed a commit that referenced this issue Feb 8, 2025
…cus-trap (#11453)

**Related Issue:** #11345 

## Summary

Adds `focusTrapOptions` option to allow additional customization of
focus-trapping components. It supports the following options from
https://github.com/focus-trap/focus-trap#createoptions:

* `initialFocus` – would replace popover's internal
initialFocusTrapFocus prop
* `allowOutsideClick` – supports
#10682
* `returnFocusOnDeactivate` – supports
#10682

And the following custom prop:

* `extraContainers` – allows specifying extra elements (nodes or
selectors) to focus trap (e.g., anything appending to the body, etc)
when creating/activating the trap. **Note**: if specified, elements must
exist when the focus trap is activated, if extra containers are created
afterwards, users can use `updateFocusTrapElements(extraContainers)`

This also enhances `updatesFocusTrapElements()` to accept extra
containers to allow in the focus trap if these are created after the
trap is activated.

### Notes

* A subset of https://github.com/focus-trap/focus-trap#createoptions
options are exposed as certain configurations might break component
functionality.
* `extraContainers` gets used both when creating and updating the focus
trap target containers
* Tidies up types
@geospatialem
Copy link
Member

The above might be addressed, or partially addressed with component focus trapping completed with #11453 and #10522. Reassigning as a spike to confirm and/or address next steps. cc @DitwanP

@geospatialem geospatialem added the spike Issues that need quick investigations for time estimations, prioritization, or a quick assessment. label Feb 21, 2025
@geospatialem geospatialem added 1 - assigned Issues that are assigned to a sprint and a team member. and removed 0 - new New issues that need assignment. labels Feb 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1 - assigned Issues that are assigned to a sprint and a team member. a11y Issues related to Accessibility fixes or improvements. ArcGIS Maps SDK for JavaScript Issues logged by ArcGIS SDK for JavaScript team members. bug Bug reports for broken functionality. Issues should include a reproduction of the bug. calcite-components Issues specific to the @esri/calcite-components package. estimate - 5 A few days of work, definitely requires updates to tests. impact - p3 - not time sensitive User set priority impact status of p3 - not time sensitive p - medium Issue is non core or affecting less that 60% of people using the library spike Issues that need quick investigations for time estimations, prioritization, or a quick assessment.
Projects
None yet
Development

No branches or pull requests

5 participants