From 44628a119e957e07d0bdd96f3ad84a3b4d1bbac7 Mon Sep 17 00:00:00 2001 From: Segun Adebayo Date: Thu, 5 Sep 2024 16:58:24 +0100 Subject: [PATCH] refactor(select): prefer click event --- .changeset/few-ducks-yawn.md | 8 ++ .xstate/select.js | 66 +++++----------- e2e/select.e2e.ts | 11 --- examples/next-ts/pages/compositions | 2 +- .../machines/select/src/select.connect.ts | 28 +++---- .../machines/select/src/select.machine.ts | 79 ++++++------------- packages/machines/select/src/select.types.ts | 5 -- 7 files changed, 63 insertions(+), 136 deletions(-) create mode 100644 .changeset/few-ducks-yawn.md diff --git a/.changeset/few-ducks-yawn.md b/.changeset/few-ducks-yawn.md new file mode 100644 index 0000000000..e100e76398 --- /dev/null +++ b/.changeset/few-ducks-yawn.md @@ -0,0 +1,8 @@ +--- +"@zag-js/select": patch +--- + +Refactor opening and selection to be based on click events rather than pointerdown/up cycles. + +This improves the usability and accessibility of the select component. It also fixes an issue where controlled multiple +selects open state behaved unexpectedly. diff --git a/.xstate/select.js b/.xstate/select.js index dcdd9d3684..a6a7c1165f 100644 --- a/.xstate/select.js +++ b/.xstate/select.js @@ -30,14 +30,10 @@ const fetchMachine = createMachine({ "!multiple": false, "!multiple": false, "!multiple": false, - "shouldRestoreFocus": false, "isOpenControlled": false, "isOpenControlled": false, "closeOnSelect && isOpenControlled": false, "closeOnSelect": false, - "shouldRestoreFocus && isOpenControlled": false, - "shouldRestoreFocus": false, - "isOpenControlled": false, "hasHighlightedItem && loop && isLastItemHighlighted": false, "hasHighlightedItem": false, "hasHighlightedItem && loop && isFirstItemHighlighted": false, @@ -81,16 +77,17 @@ const fetchMachine = createMachine({ "CONTROLLED.OPEN": [{ cond: "isTriggerClickEvent", target: "open", - actions: ["highlightFirstSelectedItem"] + actions: ["setInitialFocus", "highlightFirstSelectedItem"] }, { - target: "open" + target: "open", + actions: ["setInitialFocus"] }], "TRIGGER.CLICK": [{ cond: "isOpenControlled", actions: ["invokeOnOpen"] }, { target: "open", - actions: ["invokeOnOpen", "highlightFirstSelectedItem"] + actions: ["invokeOnOpen", "setInitialFocus", "highlightFirstSelectedItem"] }], "TRIGGER.FOCUS": { target: "focused" @@ -100,35 +97,35 @@ const fetchMachine = createMachine({ actions: ["invokeOnOpen"] }, { target: "open", - actions: ["invokeOnOpen"] + actions: ["setInitialFocus", "invokeOnOpen"] }] } }, focused: { tags: ["closed"], - entry: ["focusTriggerEl"], on: { "CONTROLLED.OPEN": [{ cond: "isTriggerClickEvent", target: "open", - actions: ["highlightFirstSelectedItem"] + actions: ["setInitialFocus", "highlightFirstSelectedItem"] }, { cond: "isTriggerArrowUpEvent", target: "open", - actions: ["highlightComputedLastItem"] + actions: ["setInitialFocus", "highlightComputedLastItem"] }, { cond: "isTriggerArrowDownEvent || isTriggerEnterEvent", target: "open", - actions: ["highlightComputedFirstItem"] + actions: ["setInitialFocus", "highlightComputedFirstItem"] }, { - target: "open" + target: "open", + actions: ["setInitialFocus"] }], OPEN: [{ cond: "isOpenControlled", actions: ["invokeOnOpen"] }, { target: "open", - actions: ["invokeOnOpen"] + actions: ["setInitialFocus", "invokeOnOpen"] }], "TRIGGER.BLUR": { target: "idle" @@ -138,28 +135,28 @@ const fetchMachine = createMachine({ actions: ["invokeOnOpen"] }, { target: "open", - actions: ["invokeOnOpen", "highlightFirstSelectedItem"] + actions: ["setInitialFocus", "invokeOnOpen", "highlightFirstSelectedItem"] }], "TRIGGER.ENTER": [{ cond: "isOpenControlled", actions: ["invokeOnOpen"] }, { target: "open", - actions: ["invokeOnOpen", "highlightComputedFirstItem"] + actions: ["setInitialFocus", "invokeOnOpen", "highlightComputedFirstItem"] }], "TRIGGER.ARROW_UP": [{ cond: "isOpenControlled", actions: ["invokeOnOpen"] }, { target: "open", - actions: ["invokeOnOpen", "highlightComputedLastItem"] + actions: ["setInitialFocus", "invokeOnOpen", "highlightComputedLastItem"] }], "TRIGGER.ARROW_DOWN": [{ cond: "isOpenControlled", actions: ["invokeOnOpen"] }, { target: "open", - actions: ["invokeOnOpen", "highlightComputedFirstItem"] + actions: ["setInitialFocus", "invokeOnOpen", "highlightComputedFirstItem"] }], "TRIGGER.ARROW_LEFT": [{ cond: "!multiple && hasSelectedItems", @@ -191,24 +188,19 @@ const fetchMachine = createMachine({ }, open: { tags: ["open"], - entry: ["setInitialFocus"], exit: ["scrollContentToTop"], activities: ["trackDismissableElement", "computePlacement", "scrollToHighlightedItem"], on: { - "CONTROLLED.CLOSE": [{ - cond: "shouldRestoreFocus", + "CONTROLLED.CLOSE": { target: "focused", - actions: ["clearHighlightedItem"] - }, { - target: "idle", - actions: ["clearHighlightedItem"] - }], + actions: ["focusTriggerEl", "clearHighlightedItem"] + }, CLOSE: [{ cond: "isOpenControlled", actions: ["invokeOnClose"] }, { target: "focused", - actions: ["invokeOnClose", "clearHighlightedItem"] + actions: ["invokeOnClose", "focusTriggerEl", "clearHighlightedItem"] }], "TRIGGER.CLICK": [{ cond: "isOpenControlled", @@ -227,24 +219,6 @@ const fetchMachine = createMachine({ }, { actions: ["selectHighlightedItem"] }], - "CONTENT.INTERACT_OUTSIDE": [ - // == group 1 == - { - cond: "shouldRestoreFocus && isOpenControlled", - actions: ["invokeOnClose"] - }, { - cond: "shouldRestoreFocus", - target: "focused", - actions: ["invokeOnClose", "clearHighlightedItem"] - }, - // == group 2 == - { - cond: "isOpenControlled", - actions: ["invokeOnClose"] - }, { - target: "idle", - actions: ["invokeOnClose", "clearHighlightedItem"] - }], "CONTENT.HOME": { actions: ["highlightFirstItem"] }, @@ -299,10 +273,8 @@ const fetchMachine = createMachine({ "isTriggerArrowDownEvent || isTriggerEnterEvent": ctx => ctx["isTriggerArrowDownEvent || isTriggerEnterEvent"], "!multiple && hasSelectedItems": ctx => ctx["!multiple && hasSelectedItems"], "!multiple": ctx => ctx["!multiple"], - "shouldRestoreFocus": ctx => ctx["shouldRestoreFocus"], "closeOnSelect && isOpenControlled": ctx => ctx["closeOnSelect && isOpenControlled"], "closeOnSelect": ctx => ctx["closeOnSelect"], - "shouldRestoreFocus && isOpenControlled": ctx => ctx["shouldRestoreFocus && isOpenControlled"], "hasHighlightedItem && loop && isLastItemHighlighted": ctx => ctx["hasHighlightedItem && loop && isLastItemHighlighted"], "hasHighlightedItem": ctx => ctx["hasHighlightedItem"], "hasHighlightedItem && loop && isFirstItemHighlighted": ctx => ctx["hasHighlightedItem && loop && isFirstItemHighlighted"] diff --git a/e2e/select.e2e.ts b/e2e/select.e2e.ts index a93e48e118..5eb7c3df65 100644 --- a/e2e/select.e2e.ts +++ b/e2e/select.e2e.ts @@ -40,17 +40,6 @@ test.describe("pointer", () => { await I.seeTriggerHasText("Select option") }) - test("should open and select with pointer cycle", async () => { - const albania = "Albania" - - await I.pointerDownTrigger() - await I.hoverItem(albania) - await I.pointerUpItem(albania) - - await I.seeItemIsChecked(albania) - await I.seeTriggerHasText(albania) - }) - test("should highlight on hover", async () => { await I.clickTrigger() await I.hoverItem("Albania") diff --git a/examples/next-ts/pages/compositions b/examples/next-ts/pages/compositions index 270df77023..89312973f0 160000 --- a/examples/next-ts/pages/compositions +++ b/examples/next-ts/pages/compositions @@ -1 +1 @@ -Subproject commit 270df77023ae3be08e73b4d81b09aada7b11ce1c +Subproject commit 89312973f0e153bdb13072b48321d92b28c560d4 diff --git a/packages/machines/select/src/select.connect.ts b/packages/machines/select/src/select.connect.ts index 1592f2ebc1..07ec0918e5 100644 --- a/packages/machines/select/src/select.connect.ts +++ b/packages/machines/select/src/select.connect.ts @@ -1,4 +1,4 @@ -import { getEventKey, isLeftClick, type EventKeyMap } from "@zag-js/dom-event" +import { getEventKey, type EventKeyMap } from "@zag-js/dom-event" import { ariaAttr, dataAttr, @@ -166,19 +166,11 @@ export function connect(userContext: UserDefinedContex collection: ctx.collection ?? collection.empty(), typeahead: getByTypeahead.defaultOptions, fieldsetDisabled: false, - restoreFocus: true, positioning: { placement: "bottom-start", gutter: 8, @@ -92,10 +91,11 @@ export function machine(userContext: UserDefinedContex { guard: "isTriggerClickEvent", target: "open", - actions: ["highlightFirstSelectedItem"], + actions: ["setInitialFocus", "highlightFirstSelectedItem"], }, { target: "open", + actions: ["setInitialFocus"], }, ], "TRIGGER.CLICK": [ @@ -105,7 +105,7 @@ export function machine(userContext: UserDefinedContex }, { target: "open", - actions: ["invokeOnOpen", "highlightFirstSelectedItem"], + actions: ["invokeOnOpen", "setInitialFocus", "highlightFirstSelectedItem"], }, ], "TRIGGER.FOCUS": { @@ -118,7 +118,7 @@ export function machine(userContext: UserDefinedContex }, { target: "open", - actions: ["invokeOnOpen"], + actions: ["setInitialFocus", "invokeOnOpen"], }, ], }, @@ -126,26 +126,26 @@ export function machine(userContext: UserDefinedContex focused: { tags: ["closed"], - entry: ["focusTriggerEl"], on: { "CONTROLLED.OPEN": [ { guard: "isTriggerClickEvent", target: "open", - actions: ["highlightFirstSelectedItem"], + actions: ["setInitialFocus", "highlightFirstSelectedItem"], }, { guard: "isTriggerArrowUpEvent", target: "open", - actions: ["highlightComputedLastItem"], + actions: ["setInitialFocus", "highlightComputedLastItem"], }, { guard: or("isTriggerArrowDownEvent", "isTriggerEnterEvent"), target: "open", - actions: ["highlightComputedFirstItem"], + actions: ["setInitialFocus", "highlightComputedFirstItem"], }, { target: "open", + actions: ["setInitialFocus"], }, ], OPEN: [ @@ -155,7 +155,7 @@ export function machine(userContext: UserDefinedContex }, { target: "open", - actions: ["invokeOnOpen"], + actions: ["setInitialFocus", "invokeOnOpen"], }, ], "TRIGGER.BLUR": { @@ -168,7 +168,7 @@ export function machine(userContext: UserDefinedContex }, { target: "open", - actions: ["invokeOnOpen", "highlightFirstSelectedItem"], + actions: ["setInitialFocus", "invokeOnOpen", "highlightFirstSelectedItem"], }, ], "TRIGGER.ENTER": [ @@ -178,7 +178,7 @@ export function machine(userContext: UserDefinedContex }, { target: "open", - actions: ["invokeOnOpen", "highlightComputedFirstItem"], + actions: ["setInitialFocus", "invokeOnOpen", "highlightComputedFirstItem"], }, ], "TRIGGER.ARROW_UP": [ @@ -188,7 +188,7 @@ export function machine(userContext: UserDefinedContex }, { target: "open", - actions: ["invokeOnOpen", "highlightComputedLastItem"], + actions: ["setInitialFocus", "invokeOnOpen", "highlightComputedLastItem"], }, ], "TRIGGER.ARROW_DOWN": [ @@ -198,7 +198,7 @@ export function machine(userContext: UserDefinedContex }, { target: "open", - actions: ["invokeOnOpen", "highlightComputedFirstItem"], + actions: ["setInitialFocus", "invokeOnOpen", "highlightComputedFirstItem"], }, ], "TRIGGER.ARROW_LEFT": [ @@ -238,21 +238,13 @@ export function machine(userContext: UserDefinedContex open: { tags: ["open"], - entry: ["setInitialFocus"], exit: ["scrollContentToTop"], activities: ["trackDismissableElement", "computePlacement", "scrollToHighlightedItem"], on: { - "CONTROLLED.CLOSE": [ - { - guard: "shouldRestoreFocus", - target: "focused", - actions: ["clearHighlightedItem"], - }, - { - target: "idle", - actions: ["clearHighlightedItem"], - }, - ], + "CONTROLLED.CLOSE": { + target: "focused", + actions: ["focusTriggerEl", "clearHighlightedItem"], + }, CLOSE: [ { guard: "isOpenControlled", @@ -260,7 +252,7 @@ export function machine(userContext: UserDefinedContex }, { target: "focused", - actions: ["invokeOnClose", "clearHighlightedItem"], + actions: ["invokeOnClose", "focusTriggerEl", "clearHighlightedItem"], }, ], "TRIGGER.CLICK": [ @@ -287,28 +279,6 @@ export function machine(userContext: UserDefinedContex actions: ["selectHighlightedItem"], }, ], - "CONTENT.INTERACT_OUTSIDE": [ - // == group 1 == - { - guard: and("shouldRestoreFocus", "isOpenControlled"), - actions: ["invokeOnClose"], - }, - { - guard: "shouldRestoreFocus", - target: "focused", - actions: ["invokeOnClose", "clearHighlightedItem"], - }, - - // == group 2 == - { - guard: "isOpenControlled", - actions: ["invokeOnClose"], - }, - { - target: "idle", - actions: ["invokeOnClose", "clearHighlightedItem"], - }, - ], "CONTENT.HOME": { actions: ["highlightFirstItem"], }, @@ -366,7 +336,6 @@ export function machine(userContext: UserDefinedContex isFirstItemHighlighted: (ctx) => ctx.highlightedValue === ctx.collection.firstValue, isLastItemHighlighted: (ctx) => ctx.highlightedValue === ctx.collection.lastValue, closeOnSelect: (ctx, evt) => !!(evt.closeOnSelect ?? ctx.closeOnSelect), - shouldRestoreFocus: (ctx) => !!ctx.restoreFocus, // guard assertions (for controlled mode) isOpenControlled: (ctx) => !!ctx["open.controlled"], isTriggerClickEvent: (_ctx, evt) => evt.previousEvent?.type === "TRIGGER.CLICK", @@ -387,6 +356,7 @@ export function machine(userContext: UserDefinedContex }, trackDismissableElement(ctx, _evt, { send }) { const contentEl = () => dom.getContentEl(ctx) + let restoreFocus = true return trackDismissableElement(contentEl, { defer: true, exclude: [dom.getTriggerEl(ctx), dom.getClearTriggerEl(ctx)], @@ -394,10 +364,10 @@ export function machine(userContext: UserDefinedContex onPointerDownOutside: ctx.onPointerDownOutside, onInteractOutside(event) { ctx.onInteractOutside?.(event) - ctx.restoreFocus = !event.detail.focusable + restoreFocus = !(event.detail.focusable || event.detail.contextmenu) }, onDismiss() { - send({ type: "CONTENT.INTERACT_OUTSIDE" }) + send({ type: "CLOSE", src: "interact-outside", restoreFocus }) }, }) }, @@ -487,9 +457,12 @@ export function machine(userContext: UserDefinedContex element?.focus({ preventScroll: true }) }) }, - focusTriggerEl(ctx) { + focusTriggerEl(ctx, evt) { + const restoreFocus = evt.restoreFocus ?? evt.previousEvent?.restoreFocus + if (restoreFocus != null && !restoreFocus) return raf(() => { - dom.getTriggerEl(ctx)?.focus({ preventScroll: true }) + const element = dom.getTriggerEl(ctx) + element?.focus({ preventScroll: true }) }) }, selectHighlightedItem(ctx, evt) { diff --git a/packages/machines/select/src/select.types.ts b/packages/machines/select/src/select.types.ts index 930fcbd8fe..88e0ce684d 100644 --- a/packages/machines/select/src/select.types.ts +++ b/packages/machines/select/src/select.types.ts @@ -156,11 +156,6 @@ interface PrivateContext { * Whether the fieldset is disabled */ fieldsetDisabled: boolean - /** - * @internal - * Whether to restore focus to the trigger after the menu closes - */ - restoreFocus?: boolean /** * The highlighted item */