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

Merged
merged 78 commits into from
Oct 4, 2024
Merged

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

Show file tree
Hide file tree
Changes from 53 commits
Commits
Show all changes
78 commits
Select commit Hold shift + click to select a range
b9b684d
set up demo for testing component sequential closing behavior
Elijbet Apr 30, 2024
281619e
WIP
driskull Apr 30, 2024
2ad2c8e
WIP
Elijbet May 1, 2024
c95e3aa
WIP
Elijbet May 1, 2024
c54cab1
WIP
Elijbet May 1, 2024
e3823ea
add docs
Elijbet May 1, 2024
1187a3a
simplified HTML for Demo and e2e test to reflect the demo workflow
Elijbet May 1, 2024
fadbb0d
cleanup
Elijbet May 2, 2024
0622da1
stacked focus-trap components test
Elijbet May 2, 2024
00e576b
relocate test for closing of stacked focus-trap components to globalS…
Elijbet May 8, 2024
55fbb07
fix failing tests
Elijbet May 8, 2024
0af108e
add wait for events open/close to input-date-picker for additional check
Elijbet Jun 20, 2024
47cf21d
add wait for events open/close to input-time-picker for additional check
Elijbet Jun 20, 2024
6a1ad45
merge dev
Elijbet Jul 30, 2024
8ca1dc5
docs
Elijbet Jul 30, 2024
d2324b2
merge dev
Elijbet Jul 30, 2024
5861f82
fix timing issues with modal and sheet
Elijbet Aug 2, 2024
95af390
remove focus-trapping logic and let popover handle it
Elijbet Aug 2, 2024
69dbd5a
WIP
Elijbet Aug 2, 2024
ba39a7f
WIP
Elijbet Aug 2, 2024
3e4b38e
enable focus trap in input-time-picker and fix failing tests
Elijbet Aug 4, 2024
85f4ef2
Helper to manage focusTrap by determining if a click event occurred o…
Elijbet Aug 8, 2024
7418a25
WIP
Elijbet Aug 8, 2024
a9dd5c0
WIP
Elijbet Aug 8, 2024
58622a4
WIP test
Elijbet Aug 9, 2024
0376e14
add focus trap to dialog
Elijbet Aug 9, 2024
f11fb0f
WIP
Elijbet Aug 9, 2024
31764b2
WIP
Elijbet Aug 9, 2024
a7cebb6
WIP should not toggle popovers with triggerDisabled
Elijbet Aug 9, 2024
c993e02
merge dev
jcfranco Aug 19, 2024
327aa38
WIP popover
Elijbet Aug 20, 2024
785fed5
WIP popover
Elijbet Aug 20, 2024
84f881e
WIP popover
Elijbet Aug 21, 2024
84c20d8
WIP input-date-picker
Elijbet Aug 21, 2024
9db785e
merge dev
Elijbet Aug 21, 2024
06570cd
merge dev
Elijbet Aug 21, 2024
c3e5da4
test cleanup
Elijbet Aug 21, 2024
ef5ad2a
cleanup
Elijbet Aug 21, 2024
16564f1
merge dev
jcfranco Aug 22, 2024
f7dd170
WIP
Elijbet Aug 23, 2024
a493275
WIP
Elijbet Aug 23, 2024
6d3ed51
WIP
Elijbet Aug 23, 2024
d8d0412
WIP
Elijbet Aug 23, 2024
9f18ccc
WIP
Elijbet Aug 24, 2024
1e67bf8
Utilities for implementing FocusTrapComponent are no longer necessary…
Elijbet Aug 25, 2024
3e94a4d
WIP
Elijbet Aug 25, 2024
16c7f1a
WIP
Elijbet Aug 25, 2024
5d5b90b
WIP
Elijbet Aug 26, 2024
20cde10
make clickOutsideDeactivates depend on outsideCloseDisabled value
Elijbet Sep 1, 2024
249081a
merge dev
Elijbet Sep 4, 2024
829d93b
remove clickOutsideDeactivates option from focus-trap since scrim han…
Elijbet Sep 6, 2024
d8acece
merge dev
Elijbet Sep 24, 2024
76a4cf7
cleanup
Elijbet Sep 24, 2024
766ec1e
merge dev
Elijbet Sep 26, 2024
f720d10
change logic to handle ignoring an Escape key press in focus-trap
Elijbet Sep 26, 2024
6bf5a5f
change logic to handle ignoring an Escape key press in focus-trap on …
Elijbet Sep 26, 2024
7f110b6
adjust test timing to accomodate changes
Elijbet Sep 26, 2024
0b582b8
adjust timing on modal tests
Elijbet Sep 26, 2024
7afdb64
remove closable from panel so it doesn't prevent shell from closing
Elijbet Sep 26, 2024
d8f34c5
this.effectiveReferenceElement
Elijbet Sep 26, 2024
87f4f56
cleanup
Elijbet Sep 27, 2024
b645dea
remove redundant connectFocusTrap(this);
Elijbet Sep 30, 2024
353ec39
sub waiting for timeout to waiting for event
Elijbet Oct 1, 2024
79ddd20
seperate stackedFocusTrap.e2e.ts
Elijbet Oct 1, 2024
337342b
cleanup stackedFocusTrap test
Elijbet Oct 1, 2024
212f6bc
clean up test
Elijbet Oct 1, 2024
0f8c4e2
assert on focus component
Elijbet Oct 2, 2024
fd53ff4
remove redundant components from modal, add dialog, account for focus…
Elijbet Oct 2, 2024
ee5d656
add camelCase util and target sheet-button id instead of sheet as it …
Elijbet Oct 3, 2024
7e3dda8
tab out of focus-retaining input components to assert on focus return…
Elijbet Oct 3, 2024
06e68ff
add comments to focusTrap options and cleanu up tests
Elijbet Oct 3, 2024
cfe719d
remove focusTrap component properties and pass options to connectFocu…
Elijbet Oct 3, 2024
b7f7411
cleanup
Elijbet Oct 3, 2024
b75baee
move anonymous focusTrap option functions to private methods on the c…
Elijbet Oct 3, 2024
fc85f74
remove stylistic changes and group helpers
Elijbet Oct 4, 2024
5097b0b
cleanup
Elijbet Oct 4, 2024
2c40889
clickOutsideDeactivates option on sheet and scrim comment, remove ext…
Elijbet Oct 4, 2024
5117688
explainer to sheet-button assertion
Elijbet Oct 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions packages/calcite-components/src/components/dialog/dialog.e2e.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -444,7 +444,7 @@ describe("calcite-dialog", () => {

await page.keyboard.press("Escape");
await page.waitForChanges();
await page.waitForChanges();

expect(mockCallBack).toHaveBeenCalledTimes(2);
expect(await page.find(`calcite-dialog >>> .${CSS.containerOpen}`)).toBeNull();
});
Expand Down Expand Up @@ -716,17 +716,15 @@ describe("calcite-dialog", () => {
const dialog = await page.find("calcite-dialog");
await page.waitForChanges();
expect(dialog).toHaveAttribute("open");
expect(dialog).toHaveAttribute("open");
await page.waitForChanges();
Elijbet marked this conversation as resolved.
Show resolved Hide resolved

await page.keyboard.press("Escape");
await page.waitForChanges();
expect(dialog).not.toHaveAttribute("open");
expect(dialog).not.toHaveAttribute("open");

dialog.setProperty("open", true);
await page.waitForChanges();
expect(dialog).toHaveAttribute("open");
expect(dialog).toHaveAttribute("open");
});

it("closes and allows re-opening when Close button is clicked", async () => {
Expand Down
9 changes: 9 additions & 0 deletions packages/calcite-components/src/components/dialog/dialog.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,11 @@ export class Dialog
this.mutationObserver?.observe(this.el, { childList: true, subtree: true });
connectLocalized(this);
connectMessages(this);
connectFocusTrap(this, {
focusTrapOptions: {
clickOutsideDeactivates: false,
Elijbet marked this conversation as resolved.
Show resolved Hide resolved
},
});
connectFocusTrap(this);
this.setupInteractions();
}
Expand Down Expand Up @@ -482,6 +487,10 @@ export class Dialog
deactivateFocusTrap(this);
}

onFocusTrapDeactivate(): void {
Copy link
Member

Choose a reason for hiding this comment

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

I noticed components close on blur or when clicking outside or on another element (e.g., scrim). Can you confirm all component-closing onFocusTrapDeactivate calls are needed?

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 think in dialog/modal we also have to consider the value of outsideCloseDisabled.

this.open = false;
}

@Watch("open")
toggleDialog(value: boolean): void {
if (this.ignoreOpenChange) {
Expand Down
23 changes: 20 additions & 3 deletions packages/calcite-components/src/components/input-date-picker/input-date-picker.e2e.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -373,19 +373,18 @@ describe("calcite-input-date-picker", () => {

it("toggles the date picker when clicked", async () => {
let calendar = await page.find(`calcite-input-date-picker >>> .${CSS.calendarWrapper}`);
Elijbet marked this conversation as resolved.
Show resolved Hide resolved

expect(await calendar.isVisible()).toBe(false);

await inputDatePicker.click();
await page.waitForChanges();
calendar = await page.find(`calcite-input-date-picker >>> .${CSS.calendarWrapper}`);

calendar = await page.find(`calcite-input-date-picker >>> .${CSS.calendarWrapper}`);
expect(await calendar.isVisible()).toBe(true);

await inputDatePicker.click();
await page.waitForChanges();
calendar = await page.find(`calcite-input-date-picker >>> .${CSS.calendarWrapper}`);

Elijbet marked this conversation as resolved.
Show resolved Hide resolved
calendar = await page.find(`calcite-input-date-picker >>> .${CSS.calendarWrapper}`);
expect(await calendar.isVisible()).toBe(false);
});

Expand Down Expand Up @@ -429,6 +428,8 @@ describe("calcite-input-date-picker", () => {
}

it("toggles the date picker when clicked", async () => {
const openSpy = await inputDatePicker.spyOnEvent("calciteInputDatePickerOpen");
const closeSpy = await inputDatePicker.spyOnEvent("calciteInputDatePickerClose");
const calendar = await page.find(`calcite-input-date-picker >>> .${CSS.calendarWrapper}`);

expect(await isCalendarVisible(calendar, "start")).toBe(false);
Expand All @@ -453,11 +454,13 @@ describe("calcite-input-date-picker", () => {
await resetFocus(page);
await startInput.click();
await page.waitForChanges();
expect(openSpy).toHaveReceivedEventTimes(1);
Elijbet marked this conversation as resolved.
Show resolved Hide resolved

expect(await isCalendarVisible(calendar, "start")).toBe(true);

await startInput.click();
await page.waitForChanges();
expect(closeSpy).toHaveReceivedEventTimes(1);

expect(await isCalendarVisible(calendar, "start")).toBe(false);

Expand All @@ -466,11 +469,13 @@ describe("calcite-input-date-picker", () => {
await resetFocus(page);
await startInputToggle.click();
await page.waitForChanges();
expect(openSpy).toHaveReceivedEventTimes(2);

expect(await isCalendarVisible(calendar, "start")).toBe(true);

await startInputToggle.click();
await page.waitForChanges();
expect(closeSpy).toHaveReceivedEventTimes(2);

expect(await isCalendarVisible(calendar, "start")).toBe(false);

Expand All @@ -479,11 +484,13 @@ describe("calcite-input-date-picker", () => {
await resetFocus(page);
await endInput.click();
await page.waitForChanges();
expect(openSpy).toHaveReceivedEventTimes(3);

expect(await isCalendarVisible(calendar, "end")).toBe(true);

await endInput.click();
await page.waitForChanges();
expect(closeSpy).toHaveReceivedEventTimes(3);

expect(await isCalendarVisible(calendar, "end")).toBe(false);

Expand All @@ -492,11 +499,13 @@ describe("calcite-input-date-picker", () => {
await resetFocus(page);
await endInputToggle.click();
await page.waitForChanges();
expect(openSpy).toHaveReceivedEventTimes(4);

expect(await isCalendarVisible(calendar, "end")).toBe(true);

await endInputToggle.click();
await page.waitForChanges();
expect(closeSpy).toHaveReceivedEventTimes(4);

expect(await isCalendarVisible(calendar, "end")).toBe(false);

Expand All @@ -505,11 +514,13 @@ describe("calcite-input-date-picker", () => {
await resetFocus(page);
await startInput.click();
await page.waitForChanges();
expect(openSpy).toHaveReceivedEventTimes(5);

expect(await isCalendarVisible(calendar, "start")).toBe(true);

await startInputToggle.click();
await page.waitForChanges();
expect(closeSpy).toHaveReceivedEventTimes(5);

expect(await isCalendarVisible(calendar, "start")).toBe(false);

Expand All @@ -518,11 +529,13 @@ describe("calcite-input-date-picker", () => {
await resetFocus(page);
await startInputToggle.click();
await page.waitForChanges();
expect(openSpy).toHaveReceivedEventTimes(6);

expect(await isCalendarVisible(calendar, "start")).toBe(true);

await startInput.click();
await page.waitForChanges();
expect(closeSpy).toHaveReceivedEventTimes(6);

expect(await isCalendarVisible(calendar, "start")).toBe(false);

Expand All @@ -531,11 +544,13 @@ describe("calcite-input-date-picker", () => {
await resetFocus(page);
await endInput.click();
await page.waitForChanges();
expect(openSpy).toHaveReceivedEventTimes(7);

expect(await isCalendarVisible(calendar, "end")).toBe(true);

await endInputToggle.click();
await page.waitForChanges();
expect(closeSpy).toHaveReceivedEventTimes(7);

expect(await isCalendarVisible(calendar, "end")).toBe(false);

Expand All @@ -544,11 +559,13 @@ describe("calcite-input-date-picker", () => {
await resetFocus(page);
await endInputToggle.click();
await page.waitForChanges();
expect(openSpy).toHaveReceivedEventTimes(8);

expect(await isCalendarVisible(calendar, "end")).toBe(true);

await endInput.click();
await page.waitForChanges();
expect(closeSpy).toHaveReceivedEventTimes(8);

expect(await isCalendarVisible(calendar, "end")).toBe(false);

Expand Down
10 changes: 6 additions & 4 deletions packages/calcite-components/src/components/input-date-picker/input-date-picker.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -880,6 +880,10 @@ export class InputDatePicker
this.datePickerEl.reset();
}

onFocusTrapDeactivate(): void {
this.open = false;
}

syncHiddenFormInput(input: HTMLInputElement): void {
syncHiddenFormInput("date", this, input);
}
Expand Down Expand Up @@ -963,10 +967,6 @@ export class InputDatePicker
this.open = true;
this.focusOnOpen = true;
event.preventDefault();
} else if (key === "Escape") {
this.open = false;
event.preventDefault();
jcfranco marked this conversation as resolved.
Show resolved Hide resolved
this.restoreInputFocus();
}
};

Expand Down Expand Up @@ -998,6 +998,8 @@ export class InputDatePicker
connectFocusTrap(this, {
focusTrapEl: el,
focusTrapOptions: {
allowOutsideClick: true,
clickOutsideDeactivates: false,
jcfranco marked this conversation as resolved.
Show resolved Hide resolved
initialFocus: false,
setReturnFocus: false,
},
Expand Down
8 changes: 8 additions & 0 deletions packages/calcite-components/src/components/input-time-picker/input-time-picker.e2e.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -933,37 +933,45 @@ describe("calcite-input-time-picker", () => {

it("toggles the time picker when clicked", async () => {
let popover = await page.find("calcite-input-time-picker >>> calcite-popover");
const openSpy = await inputTimePicker.spyOnEvent("calciteInputTimePickerOpen");
const closeSpy = await inputTimePicker.spyOnEvent("calciteInputTimePickerClose");

expect(await popover.isVisible()).toBe(false);

await inputTimePicker.click();
await page.waitForChanges();
expect(openSpy).toHaveReceivedEventTimes(1);
popover = await page.find("calcite-input-time-picker >>> calcite-popover");

expect(await popover.isVisible()).toBe(true);

await inputTimePicker.click();
await page.waitForChanges();
expect(closeSpy).toHaveReceivedEventTimes(1);
popover = await page.find("calcite-input-time-picker >>> calcite-popover");

expect(await popover.isVisible()).toBe(false);
});

it("toggles the time picker when using arrow down/escape key", async () => {
let popover = await page.find("calcite-input-time-picker >>> calcite-popover");
const openSpy = await inputTimePicker.spyOnEvent("calciteInputTimePickerOpen");
const closeSpy = await inputTimePicker.spyOnEvent("calciteInputTimePickerClose");

expect(await popover.isVisible()).toBe(false);

await inputTimePicker.callMethod("setFocus");
await page.waitForChanges();
await page.keyboard.press("ArrowDown");
await page.waitForChanges();
expect(openSpy).toHaveReceivedEventTimes(1);
popover = await page.find("calcite-input-time-picker >>> calcite-popover");

expect(await popover.isVisible()).toBe(true);

await page.keyboard.press("Escape");
await page.waitForChanges();
expect(closeSpy).toHaveReceivedEventTimes(1);
popover = await page.find("calcite-input-time-picker >>> calcite-popover");

expect(await popover.isVisible()).toBe(false);
Expand Down
Loading
Loading