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

Closing on escape no longer works when focusTrapDisabled=true #10522

Closed
1 of 6 tasks
Elijbet opened this issue Oct 10, 2024 · 6 comments
Closed
1 of 6 tasks

Closing on escape no longer works when focusTrapDisabled=true #10522

Elijbet opened this issue Oct 10, 2024 · 6 comments
Assignees
Labels
4 - verified Issues that have been released and confirmed resolved. bug Bug reports for broken functionality. Issues should include a reproduction of the bug. Calcite (dev) Issues logged by Calcite developers. estimate - 3 A day or two of work, likely requires updates to tests. impact - p0 - emergency User set priority impact status of p0 - emergency impact - p3 - not time sensitive User set priority impact status of p3 - not time sensitive p - high Issue should be addressed in the current milestone, impacts component or core functionality regression Issues that are caused by changes in a release, but were working before that.

Comments

@Elijbet
Copy link
Contributor

Elijbet commented Oct 10, 2024

Check existing issues

Actual Behavior

Related issue: #6456

Blocked issues: #10682

There is a regression with the focus-trap stacking PR. Closing on escape no longer works when focusTrapDisabled=true.

It may also affect closing on click in some components.

There is also a related failing test:

@esri/calcite-components:test: ● stacked focus-trap components › closes a stack of open components sequentially in visual order
@esri/calcite-components:test:
@esri/calcite-components:test: expect(received).toBe(expected) // Object.is equality
@esri/calcite-components:test:
@esri/calcite-components:test: Expected: "input-time-picker"
@esri/calcite-components:test: Received: "example-modal"
@esri/calcite-components:test:
@esri/calcite-components:test: 93 | ? "sheet-button"
@esri/calcite-components:test: 94 | : focusTrapOrderElements[i].id || document.body.id;
@esri/calcite-components:test: > 95 | expect(activeElementIdAfterTab).toBe(expectedElementId);
@esri/calcite-components:test: | ^
@esri/calcite-components:test: 96 |
@esri/calcite-components:test: 97 | await page.keyboard.press("Escape");
@esri/calcite-components:test: 98 | await page.waitForChanges();
@esri/calcite-components:test:
@esri/calcite-components:test: at testEscapeAndAssertOpenState (src/tests/stackedFocusTrap.e2e.ts:95:43)
@esri/calcite-components:test: at testStackEscapeSequence (src/tests/stackedFocusTrap.e2e.ts:125:7)
@esri/calcite-components:test: at Object. (src/tests/stackedFocusTrap.e2e.ts:128:5)

Expected Behavior

Components should close on escape when focus-trap is true individually or in a stack.

Reproduction Sample

https://codepen.io/jcfranco/pen/PoMbdMe?editors=100

Reproduction Steps

  1. Open the modal
  2. Press escape
  3. Observe the modal not closing as expected

Reproduction Version

2.13.0

Relevant Info

No response

Regression?

2.12.2

Priority impact

impact - p3 - not time sensitive

Impact

No response

Calcite package

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

Esri team

Calcite (dev)

@Elijbet Elijbet 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 Oct 10, 2024
@github-actions github-actions bot added Calcite (dev) Issues logged by Calcite developers. impact - p3 - not time sensitive User set priority impact status of p3 - not time sensitive labels Oct 10, 2024
@geospatialem geospatialem added p - high Issue should be addressed in the current milestone, impacts component or core functionality 1 - assigned Issues that are assigned to a sprint and a team member. estimate - 3 A day or two of work, likely requires updates to tests. and removed 0 - new New issues that need assignment. needs triage Planning workflow - pending design/dev review. labels Oct 10, 2024
@driskull
Copy link
Member

@geospatialem since this is a regression shouldn't it be in a patch?

@geospatialem
Copy link
Member

geospatialem commented Oct 10, 2024

@geospatialem since this is a regression shouldn't it be in a patch?

@driskull @jcfranco reminded me that we merged it in with the October milestone, so we just need to address it prior to the launch of 3.0 in November. 💪🏻

@jcfranco jcfranco added impact - p0 - emergency User set priority impact status of p0 - emergency and removed impact - p3 - not time sensitive User set priority impact status of p3 - not time sensitive labels Oct 21, 2024
@jcfranco
Copy link
Member

Updated the impact label accordingly.

@jcfranco jcfranco added the regression Issues that are caused by changes in a release, but were working before that. label Oct 21, 2024
@github-actions github-actions bot added the impact - p3 - not time sensitive User set priority impact status of p3 - not time sensitive label Nov 12, 2024
@Elijbet Elijbet added 2 - in development Issues that are actively being worked on. and removed 1 - assigned Issues that are assigned to a sprint and a team member. labels Nov 25, 2024
Elijbet added a commit that referenced this issue Nov 27, 2024
…cape when focusTrap is disabled (#10578)

**Related Issue:** #10522

## Summary
Close on `Escape` works when `focusTrap` is disabled.
@Elijbet Elijbet added 3 - installed Issues that have been merged to master branch and are ready for final confirmation. and removed 2 - in development Issues that are actively being worked on. labels Nov 27, 2024
Copy link
Contributor

Installed and assigned for verification.

@geospatialem
Copy link
Member

geospatialem commented Nov 27, 2024

Verified for the input-time-picker and modal components with 3.0.0-next.36 🎉

Observed a few behaviors that may need follow-up cc @Elijbet (assigning back to ya to confirm the following):

Note: the full testing suite of components can be accessed at: https://codepen.io/geospatialem/pen/qEWByZz

@geospatialem geospatialem added 1 - assigned Issues that are assigned to a sprint and a team member. and removed 3 - installed Issues that have been merged to master branch and are ready for final confirmation. labels Nov 27, 2024
@geospatialem geospatialem assigned Elijbet and unassigned geospatialem and DitwanP Nov 27, 2024
@geospatialem geospatialem added 4 - verified Issues that have been released and confirmed resolved. and removed 1 - assigned Issues that are assigned to a sprint and a team member. labels Nov 27, 2024
@geospatialem
Copy link
Member

This is expected. And input-time-picker has an open issue to mitigate towards this behavior in #10639.

sheet: The esc key doesn't seem to close the component when focus-trap-disabled is present unless focus is on the close button, is this expected?: https://codepen.io/geospatialem/pen/VYZwBKY

Updated the sample to fit the use case, and it performs as-expected. ✨

Closing out the above as verified. Nice work, @Elijbet! 💪🏻

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. bug Bug reports for broken functionality. Issues should include a reproduction of the bug. Calcite (dev) Issues logged by Calcite developers. estimate - 3 A day or two of work, likely requires updates to tests. impact - p0 - emergency User set priority impact status of p0 - emergency impact - p3 - not time sensitive User set priority impact status of p3 - not time sensitive p - high Issue should be addressed in the current milestone, impacts component or core functionality regression Issues that are caused by changes in a release, but were working before that.
Projects
None yet
Development

No branches or pull requests

5 participants