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.

Sign up for GitHub

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

Jump to bottom

feat(dialog, modal, popover, input-date-picker, input-time-picker, sheet): support stacked component sequential closing with escape #9231

Open
wants to merge 71 commits into
base: dev
Choose a base branch
from
Open

feat(dialog, modal, popover, input-date-picker, input-time-picker, sheet): support stacked component sequential closing with escape #9231

wants to merge 71 commits into from

Conversation

Elijbet
Copy link
Contributor

@Elijbet Elijbet commented Apr 30, 2024

Related Issue: #6456

Summary

Support component sequential closing with escape by having focus-trap handle the stacking order.

@github-actions github-actions bot added the enhancement Issues tied to a new feature or request. label Apr 30, 2024
driskull
driskull reviewed May 2, 2024
View reviewed changes
Copy link
Member

@driskull driskull left a comment

Choose a reason for hiding this comment

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

Initial feedbackz

packages/calcite-components/src/components/input-date-picker/input-date-picker.tsx Outdated Show resolved Hide resolved
packages/calcite-components/src/components/input-time-picker/input-time-picker.tsx Outdated Show resolved Hide resolved
packages/calcite-components/src/components/sheet/sheet.e2e.ts Outdated Show resolved Hide resolved
packages/calcite-components/src/demos/sheet.html Outdated Show resolved Hide resolved
packages/calcite-components/src/utils/focusTrapComponent.ts Show resolved Hide resolved
packages/calcite-components/src/utils/focusTrapComponent.ts Outdated Show resolved Hide resolved
packages/calcite-components/src/utils/focusTrapComponent.ts Outdated Show resolved Hide resolved
@Elijbet Elijbet changed the title feat(sheet, modal, popover, tooltip, combobox, dropdown, date-picker): support stacked component sequential closing with escape feat(sheet, modal, popover): support stacked component sequential closing with escape May 2, 2024
@github-actions GitHub Actions
Copy link
Contributor

This PR has been automatically marked as stale because it has not had recent activity. Please close your PR if it is no longer relevant. Thank you for your contributions.

@github-actions github-actions bot added the Stale Issues or pull requests that have not had recent activity. label May 16, 2024
@Elijbet Elijbet removed the Stale Issues or pull requests that have not had recent activity. label May 16, 2024
@github-actions GitHub Actions
Copy link
Contributor

This PR has been automatically marked as stale because it has not had recent activity. Please close your PR if it is no longer relevant. Thank you for your contributions.

@github-actions github-actions bot added the Stale Issues or pull requests that have not had recent activity. label May 24, 2024
@benelan benelan changed the base branch from main to dev June 10, 2024 09:13
@github-actions github-actions bot removed the Stale Issues or pull requests that have not had recent activity. label Jun 11, 2024
@github-actions GitHub Actions
Copy link
Contributor

This PR has been automatically marked as stale because it has not had recent activity. Please close your PR if it is no longer relevant. Thank you for your contributions.

@github-actions github-actions bot added the Stale Issues or pull requests that have not had recent activity. label Jun 19, 2024
@Elijbet Elijbet removed the Stale Issues or pull requests that have not had recent activity. label Jun 20, 2024
@github-actions GitHub Actions
Copy link
Contributor

This PR has been automatically marked as stale because it has not had recent activity. Please close your PR if it is no longer relevant. Thank you for your contributions.

@github-actions GitHub Actions
Copy link
Contributor

This PR has been automatically marked as stale because it has not had recent activity. Please close your PR if it is no longer relevant. Thank you for your contributions.

@github-actions github-actions bot added the Stale Issues or pull requests that have not had recent activity. label Sep 19, 2024
@Elijbet Elijbet removed the Stale Issues or pull requests that have not had recent activity. label Sep 24, 2024
@jcfranco
Copy link
Member

There is an focus-trap/focus-trap#1253 to enhance so it will allow child elements to handle Escape key presses without causing the trap to close. We might have to wait for this to land to pick up the remaining comments.

We recently updated focus-trap, which now allows child elements to handle Escape key presses without closing the trap, but I noticed that dialog doesn't have logic to ignore canceled Escape key presses within focus-trap. Can you check that this is handled? Any code handling Escape like this should be changed to go through focus-trap, similar to this example.

@jcfranco
Copy link
Member

Could you also coordinate input-date-picker changes w/ @anveshmekala? Additional coverage was added all date-picker-related test suites (see https://github.com/Esri/calcite-design-system/pull/8402/files).

@Elijbet
Copy link
Contributor Author

Elijbet commented Sep 26, 2024

Could you also coordinate input-date-picker changes w/ @anveshmekala? Additional coverage was added all date-picker-related test suites (see #8402 (files)).

Checked with him and there are no conflicts so far.

driskull
driskull approved these changes Sep 26, 2024
View reviewed changes
Copy link
Member

@driskull driskull left a comment

Choose a reason for hiding this comment

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

This looks good to me 👍

packages/calcite-components/src/components/dialog/dialog.e2e.ts Outdated Show resolved Hide resolved
packages/calcite-components/src/components/dialog/dialog.e2e.ts Outdated Show resolved Hide resolved
packages/calcite-components/src/components/dialog/dialog.e2e.ts Outdated Show resolved Hide resolved
packages/calcite-components/src/components/modal/modal.e2e.ts Outdated Show resolved Hide resolved
packages/calcite-components/src/components/modal/modal.e2e.ts Outdated Show resolved Hide resolved
packages/calcite-components/src/components/modal/modal.e2e.ts Outdated
await page.keyboard.press("Escape");
await page.waitForChanges();
await page.waitForTimeout(delayInMilliseconds);
Copy link
Member

Choose a reason for hiding this comment

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

maybe await waitForAnimationFrame() would work instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the only place where this substitution works.

Copy link
Member

Choose a reason for hiding this comment

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

Interesting. I think we could live with the delay but I'm curious on what its waiting for exactly.

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 had to add them after this commit: 6bf5a5f (#9231)

packages/calcite-components/src/demos/input-date-picker.html Outdated Show resolved Hide resolved
jcfranco
jcfranco requested changes Oct 1, 2024
View reviewed changes
Copy link
Member

@jcfranco jcfranco left a comment

Choose a reason for hiding this comment

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

Great job, @Elijbet! Just one more round of changes and I think we'll be good to land. 🛬

packages/calcite-components/src/components/dialog/dialog.e2e.ts Outdated Show resolved Hide resolved
packages/calcite-components/src/tests/globalStyles.e2e.ts Outdated Show resolved Hide resolved
packages/calcite-components/src/tests/globalStyles.e2e.ts Outdated Show resolved Hide resolved
packages/calcite-components/src/tests/globalStyles.e2e.ts Outdated Show resolved Hide resolved
packages/calcite-components/src/tests/globalStyles.e2e.ts Outdated Show resolved Hide resolved
packages/calcite-components/src/utils/focusTrapComponent.ts
@@ -58,9 +71,10 @@ export function connectFocusTrap(component: FocusTrapComponent, options?: Connec
}

const focusTrapOptions: FocusTrapOptions = {
clickOutsideDeactivates: true,
escapeDeactivates: false,
clickOutsideDeactivates: component.clickOutsideDeactivates ?? true,
Copy link
Member

Choose a reason for hiding this comment

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

This setup seems a bit confusing to me. We’re passing options when setting up the focus trap, but some are also defined on the component itself. This could could cause confusion or issues (e.g., onDeactivate being set in both component.onFocusTrapDeactivate and options.onFocusTrapDeactivate). Can we roll it back to just rely on the component passing options for now? I'd like us to revisit this approach later.

packages/calcite-components/src/components/input-time-picker/input-time-picker.tsx
@@ -432,6 +412,11 @@ export class InputTimePicker
/** Fires when the component is open and animation is complete. */
@Event({ cancelable: false }) calciteInputTimePickerOpen: EventEmitter<void>;

@Listen("calcitePopoverClose")
Copy link
Member

Choose a reason for hiding this comment

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

Can you use the JSX event property instead of using @Listen?

packages/calcite-components/src/components/input-date-picker/input-date-picker.e2e.ts Outdated Show resolved Hide resolved
packages/calcite-components/src/components/popover/popover.tsx Outdated Show resolved Hide resolved
packages/calcite-components/src/components/dialog/dialog.tsx Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@driskull driskull driskull approved these changes

@jcfranco jcfranco jcfranco requested changes

@benelan benelan Awaiting requested review from benelan

Requested changes must be addressed to merge this pull request.

Assignees

@Elijbet Elijbet

Labels
enhancement Issues tied to a new feature or request. pr ready for visual snapshots Adding this label will run visual snapshot testing.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Elijbet @jcfranco @driskull