Skip to content

Commit 6c2acaf

Browse files
RajdeepcRajdeep Chandra
Rajdeepc
and
Rajdeep Chandra
authored
fix(picker): Escape key behavior in modal overlays containing picker components (#5672)
* fix(picker): added open attribute to handle escape key in overlay closing * chore: added storybook * chore: added tests * chore: added changeset --------- Co-authored-by: Rajdeep Chandra <[email protected]>
1 parent 6cbd438 commit 6c2acaf

File tree

4 files changed

+169
-17
lines changed

4 files changed

+169
-17
lines changed

‎.changeset/fix-picker-escape-modal.md

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
---
2+
'@spectrum-web-components/picker': minor
3+
---
4+
5+
**Fixed** escape key behavior in modal overlays containing picker components. Previously, pressing the Escape key when a picker was open inside a modal overlay would not properly close the modal, instead moving focus to the picker. Now, the escape key correctly closes the picker first (if open), then closes the modal overlay on subsequent escape key presses.
6+
7+
This fix adds a check for `this.open` in the picker's `handleEscape` method to ensure proper modal overlay closure behavior.

‎packages/picker/src/Picker.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -285,7 +285,7 @@ export class PickerBase extends SizedMixin(SpectrumElement, {
285285
protected handleEscape = (
286286
event: MenuItemKeydownEvent | KeyboardEvent
287287
): void => {
288-
if (event.key === 'Escape') {
288+
if (event.key === 'Escape' && this.open) {
289289
event.stopPropagation();
290290
event.preventDefault();
291291
this.toggle(false);

‎packages/picker/stories/picker.stories.ts

Lines changed: 73 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,9 @@ import '@spectrum-web-components/menu/sp-menu-item.js';
2323
import '@spectrum-web-components/picker/sp-picker.js';
2424
import '@spectrum-web-components/tooltip/sp-tooltip.js';
2525
import '@spectrum-web-components/overlay/sp-overlay.js';
26+
import '@spectrum-web-components/overlay/overlay-trigger.js';
2627
import '@spectrum-web-components/popover/sp-popover.js';
28+
import '@spectrum-web-components/dialog/sp-dialog.js';
2729
import { ifDefined } from 'lit-html/directives/if-defined.js';
2830
import { spreadProps } from '../../../test/lit-helpers.js';
2931
import '../../overlay/stories/index.js';
@@ -793,25 +795,80 @@ BackgroundClickTest.swc_vrt = {
793795
skip: true,
794796
};
795797

796-
export const PickerInModal = (): TemplateResult => {
798+
export const PickerInModalOverlay = (): TemplateResult => {
797799
return html`
798-
<sp-button id="trigger">Overlay Trigger</sp-button>
799-
<sp-overlay trigger="trigger@click" placement="bottom">
800-
<sp-popover>
801-
<sp-picker
802-
label="What would you like to do?"
803-
value="item-2"
804-
id="picker-icons"
805-
style="margin: 20px"
800+
<div>
801+
<div>
802+
<h3>Picker in sp-overlay (non-modal)</h3>
803+
<sp-button id="trigger">Overlay Trigger</sp-button>
804+
<sp-overlay trigger="trigger@click" placement="bottom">
805+
<sp-popover>
806+
<sp-picker
807+
label="What would you like to do?"
808+
value="item-2"
809+
id="picker-icons"
810+
style="margin: 20px"
811+
>
812+
<sp-menu-item>Save</sp-menu-item>
813+
<sp-menu-item>Finish</sp-menu-item>
814+
<sp-menu-item>Review</sp-menu-item>
815+
</sp-picker>
816+
</sp-popover>
817+
</sp-overlay>
818+
</div>
819+
820+
<div>
821+
<h3>
822+
Picker in overlay-trigger (modal) - Test Escape key behavior
823+
</h3>
824+
<overlay-trigger
825+
type="modal"
826+
id="modal-trigger"
827+
placement="top"
806828
>
807-
<sp-menu-item>Save</sp-menu-item>
808-
<sp-menu-item>Finish</sp-menu-item>
809-
<sp-menu-item>Review</sp-menu-item>
810-
</sp-picker>
811-
</sp-popover>
812-
</sp-overlay>
829+
<sp-button
830+
variant="primary"
831+
slot="trigger"
832+
style="position:absolute;bottom:50px"
833+
>
834+
Button popover
835+
</sp-button>
836+
<sp-popover slot="click-content" tip>
837+
<sp-dialog no-divider class="options-popover-content">
838+
<sp-picker
839+
label="Select a Country with a very long label, too long in fact"
840+
value="item-2"
841+
id="picker-value"
842+
>
843+
<sp-menu-item value="item-1">
844+
Deselect
845+
</sp-menu-item>
846+
<sp-menu-item value="item-2">
847+
Select inverse
848+
</sp-menu-item>
849+
<sp-menu-item value="item-3">
850+
Feather...
851+
</sp-menu-item>
852+
<sp-menu-item value="item-4">
853+
Select and mask...
854+
</sp-menu-item>
855+
<sp-menu-item value="item-5">
856+
Save selection
857+
</sp-menu-item>
858+
<sp-menu-item disabled value="item-6">
859+
Make work path
860+
</sp-menu-item>
861+
</sp-picker>
862+
</sp-dialog>
863+
</sp-popover>
864+
<sp-tooltip slot="hover-content" placement="right">
865+
I'm a tooltip in a different direction
866+
</sp-tooltip>
867+
</overlay-trigger>
868+
</div>
869+
</div>
813870
`;
814871
};
815-
PickerInModal.swc_vrt = {
872+
PickerInModalOverlay.swc_vrt = {
816873
skip: true,
817874
};

‎packages/picker/test/index.ts

Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,10 @@ import '@spectrum-web-components/menu/sp-menu-group.js';
3030
import '@spectrum-web-components/menu/sp-menu-item.js';
3131
import '@spectrum-web-components/menu/sp-menu.js';
3232
import '@spectrum-web-components/picker/sp-picker.js';
33+
import '@spectrum-web-components/overlay/overlay-trigger.js';
34+
import '@spectrum-web-components/popover/sp-popover.js';
35+
import '@spectrum-web-components/dialog/sp-dialog.js';
36+
import '@spectrum-web-components/button/sp-button.js';
3337
import { SAFARI_FOCUS_RING_CLASS } from '@spectrum-web-components/picker/src/InteractionController.js';
3438
import { isFirefox, isWebKit } from '@spectrum-web-components/shared';
3539
import '@spectrum-web-components/shared/src/focus-visible.js';
@@ -64,6 +68,8 @@ import {
6468
slottedLabel,
6569
tooltip,
6670
} from '../stories/picker.stories.js';
71+
import { OverlayTrigger } from '@spectrum-web-components/overlay';
72+
import { Button } from '@spectrum-web-components/button';
6773

6874
export type TestablePicker = { optionsMenu: Menu };
6975

@@ -2264,4 +2270,86 @@ export function runPickerTests(): void {
22642270
expect(displayedIconSrcAfter).to.equal(newSrc);
22652271
});
22662272
});
2273+
2274+
it('closes modal overlay immediately when escape is pressed on closed picker in modal', async function () {
2275+
const test = await fixture<HTMLDivElement>(html`
2276+
<sp-theme scale="medium" color="light" system="spectrum">
2277+
<overlay-trigger
2278+
type="modal"
2279+
id="modal-trigger"
2280+
placement="top"
2281+
>
2282+
<sp-button
2283+
variant="primary"
2284+
slot="trigger"
2285+
style="position:absolute;bottom:50px"
2286+
>
2287+
Open Modal
2288+
</sp-button>
2289+
<sp-popover slot="click-content" tip>
2290+
<sp-dialog no-divider class="options-popover-content">
2291+
<sp-picker
2292+
label="Select a Country"
2293+
value="item-2"
2294+
id="picker-value"
2295+
>
2296+
<sp-menu-item value="item-1">
2297+
Deselect
2298+
</sp-menu-item>
2299+
<sp-menu-item value="item-2">
2300+
Select inverse
2301+
</sp-menu-item>
2302+
<sp-menu-item value="item-3">
2303+
Feather...
2304+
</sp-menu-item>
2305+
<sp-menu-item value="item-4">
2306+
Select and mask...
2307+
</sp-menu-item>
2308+
<sp-menu-item value="item-5">
2309+
Save selection
2310+
</sp-menu-item>
2311+
<sp-menu-item disabled value="item-6">
2312+
Make work path
2313+
</sp-menu-item>
2314+
</sp-picker>
2315+
</sp-dialog>
2316+
</sp-popover>
2317+
</overlay-trigger>
2318+
</sp-theme>
2319+
`);
2320+
2321+
const overlayTrigger = test.querySelector(
2322+
'overlay-trigger'
2323+
) as OverlayTrigger;
2324+
const button = test.querySelector('sp-button') as Button;
2325+
const picker = test.querySelector('sp-picker') as Picker;
2326+
2327+
// Open the modal overlay
2328+
button.click();
2329+
await elementUpdated(overlayTrigger);
2330+
2331+
// Wait for the overlay to open
2332+
await waitUntil(
2333+
() => overlayTrigger.open === 'click',
2334+
'overlay should be open'
2335+
);
2336+
2337+
// Focus on the picker (but don't open it)
2338+
picker.focus();
2339+
await elementUpdated(picker);
2340+
2341+
// Verify picker is closed
2342+
expect(picker.open, 'picker should be closed initially').to.be.false;
2343+
2344+
// Press escape - this should close the modal overlay immediately since picker is not open
2345+
const modalClosed = oneEvent(overlayTrigger, 'sp-closed');
2346+
await sendKeys({ press: 'Escape' });
2347+
await modalClosed;
2348+
2349+
// Verify modal overlay is closed
2350+
expect(
2351+
overlayTrigger.open,
2352+
'modal overlay should be closed after escape'
2353+
).to.be.undefined;
2354+
});
22672355
}

0 commit comments

Comments
 (0)