From bd7a2e436ab315d8afa16bc7081ea2ad28bfc817 Mon Sep 17 00:00:00 2001 From: Segun Adebayo Date: Fri, 10 May 2024 15:00:17 +0100 Subject: [PATCH] refactor(tabs): prefer click for pointer interaction --- .changeset/config.json | 1 + .changeset/wicked-papayas-hang.md | 6 + .xstate/tabs.js | 69 +++++----- e2e/models/tabs.model.ts | 41 ++++++ e2e/tabs.e2e.ts | 145 ++++++++++++--------- examples/vanilla-ts/package.json | 2 +- packages/machines/tabs/src/tabs.connect.ts | 26 ++-- packages/machines/tabs/src/tabs.machine.ts | 98 +++++++------- packages/machines/tabs/src/tabs.types.ts | 15 +-- 9 files changed, 234 insertions(+), 169 deletions(-) create mode 100644 .changeset/wicked-papayas-hang.md create mode 100644 e2e/models/tabs.model.ts diff --git a/.changeset/config.json b/.changeset/config.json index bfdfc0c8c1..b53711f91a 100644 --- a/.changeset/config.json +++ b/.changeset/config.json @@ -23,6 +23,7 @@ "nuxt-ts", "preact-ts", "svelte-ts", + "vanilla-ts", "website" ] } diff --git a/.changeset/wicked-papayas-hang.md b/.changeset/wicked-papayas-hang.md new file mode 100644 index 0000000000..189bc682e2 --- /dev/null +++ b/.changeset/wicked-papayas-hang.md @@ -0,0 +1,6 @@ +--- +"@zag-js/tabs": minor +--- + +When using the pointer, prefer click based selection when using `activationMode=automatic` over focus triggering +selection. For keyboard, selection follows focus as usual. diff --git a/.xstate/tabs.js b/.xstate/tabs.js index ef80cd228a..e9dc8b7a9f 100644 --- a/.xstate/tabs.js +++ b/.xstate/tabs.js @@ -13,12 +13,10 @@ const fetchMachine = createMachine({ initial: "idle", context: { "selectOnFocus": false, - "isHorizontal": false, - "isHorizontal": false, - "isVertical": false, - "isVertical": false, - "!selectOnFocus": false, - "selectOnFocus": false + "selectOnFocus": false, + "selectOnFocus": false, + "selectOnFocus": false, + "!selectOnFocus": false }, entry: ["checkRenderedElements", "syncIndicatorRect", "setContentTabIndex"], exit: ["cleanupObserver"], @@ -41,14 +39,10 @@ const fetchMachine = createMachine({ states: { idle: { on: { - TAB_FOCUS: [{ - cond: "selectOnFocus", - target: "focused", - actions: ["setFocusedValue", "setValue"] - }, { + TAB_FOCUS: { target: "focused", actions: "setFocusedValue" - }], + }, TAB_CLICK: { target: "focused", actions: ["setFocusedValue", "setValue"] @@ -61,38 +55,37 @@ const fetchMachine = createMachine({ target: "focused", actions: ["setFocusedValue", "setValue"] }, - ARROW_LEFT: { - cond: "isHorizontal", - actions: "focusPrevTab" - }, - ARROW_RIGHT: { - cond: "isHorizontal", - actions: "focusNextTab" - }, - ARROW_UP: { - cond: "isVertical", + ARROW_PREV: [{ + cond: "selectOnFocus", + actions: ["focusPrevTab", "selectFocusedTab"] + }, { actions: "focusPrevTab" - }, - ARROW_DOWN: { - cond: "isVertical", + }], + ARROW_NEXT: [{ + cond: "selectOnFocus", + actions: ["focusNextTab", "selectFocusedTab"] + }, { actions: "focusNextTab" - }, - HOME: { + }], + HOME: [{ + cond: "selectOnFocus", + actions: ["focusFirstTab", "selectFocusedTab"] + }, { actions: "focusFirstTab" - }, - END: { + }], + END: [{ + cond: "selectOnFocus", + actions: ["focusLastTab", "selectFocusedTab"] + }, { actions: "focusLastTab" - }, + }], ENTER: { cond: "!selectOnFocus", - actions: "setValue" + actions: "selectFocusedTab" + }, + TAB_FOCUS: { + actions: ["setFocusedValue"] }, - TAB_FOCUS: [{ - cond: "selectOnFocus", - actions: ["setFocusedValue", "setValue"] - }, { - actions: "setFocusedValue" - }], TAB_BLUR: { target: "idle", actions: "clearFocusedValue" @@ -110,8 +103,6 @@ const fetchMachine = createMachine({ }, guards: { "selectOnFocus": ctx => ctx["selectOnFocus"], - "isHorizontal": ctx => ctx["isHorizontal"], - "isVertical": ctx => ctx["isVertical"], "!selectOnFocus": ctx => ctx["!selectOnFocus"] } }); \ No newline at end of file diff --git a/e2e/models/tabs.model.ts b/e2e/models/tabs.model.ts new file mode 100644 index 0000000000..dfae098881 --- /dev/null +++ b/e2e/models/tabs.model.ts @@ -0,0 +1,41 @@ +import { expect, type Page } from "@playwright/test" +import { a11y, testid } from "../_utils" +import { Model } from "./model" + +export class TabsModel extends Model { + constructor(public page: Page) { + super(page) + } + + checkAccessibility() { + return a11y(this.page) + } + + goto() { + return this.page.goto("/tabs") + } + + private getTabTrigger = (id: string) => { + return this.page.locator(testid(`${id}-tab`)) + } + + private getTabContent = (id: string) => { + return this.page.locator(testid(`${id}-tab-panel`)) + } + + clickTab = async (id: string) => { + await this.getTabTrigger(id).click() + } + + seeTabContent = async (id: string) => { + await expect(this.getTabContent(id)).toBeVisible() + } + + dontSeeTabContent = async (id: string) => { + await expect(this.getTabContent(id)).not.toBeVisible() + } + + setTabIsFocused = async (id: string) => { + await expect(this.getTabTrigger(id)).toBeFocused() + } +} diff --git a/e2e/tabs.e2e.ts b/e2e/tabs.e2e.ts index 514158aada..2320834ecd 100644 --- a/e2e/tabs.e2e.ts +++ b/e2e/tabs.e2e.ts @@ -1,82 +1,111 @@ -import { expect, test } from "@playwright/test" -import { a11y, testid } from "./_utils" +import { test } from "@playwright/test" +import { TabsModel } from "./models/tabs.model" -const item = (id: string) => ({ - tab: testid(`${id}-tab`), - panel: testid(`${id}-tab-panel`), -}) - -const nils = item("nils") -const agnes = item("agnes") -const joke = item("joke") +let I: TabsModel test.describe("tabs", () => { test.beforeEach(async ({ page }) => { - await page.goto("/tabs") + I = new TabsModel(page) + await I.goto() + }) + + test("should have no accessibility violation", async () => { + await I.checkAccessibility() }) - test("should have no accessibility violation", async ({ page }) => { - await a11y(page) + test("on home key, select first tab", async () => { + await I.clickTab("agnes") + await I.pressKey("Home") + + await I.setTabIsFocused("nils") + await I.seeTabContent("nils") }) - test.describe("in automatic mode", () => { - test("should select the correct tab on click", async ({ page }) => { - await page.click(nils.tab) - await expect(page.locator(nils.panel)).toBeVisible() + test("on end key, select last tab", async () => { + await I.clickTab("agnes") + await I.pressKey("End") + + await I.setTabIsFocused("joke") + await I.seeTabContent("joke") + }) - await page.click(agnes.tab) - await expect(page.locator(agnes.panel)).toBeVisible() + test("click tab, select tab", async () => { + await I.clickTab("agnes") + await I.seeTabContent("agnes") + }) - await page.click(joke.tab) - await expect(page.locator(joke.panel)).toBeVisible() - }) + test("automatic: should select the correct tab on click", async () => { + await I.clickTab("nils") + await I.seeTabContent("nils") - test("on `ArrowRight`: should select & focus the next tab", async ({ page }) => { - await page.focus(nils.tab) - await page.keyboard.press("ArrowRight") + await I.clickTab("agnes") + await I.seeTabContent("agnes") - await expect(page.locator(agnes.tab)).toBeFocused() - await expect(page.locator(agnes.panel)).toBeVisible() + await I.clickTab("joke") + await I.seeTabContent("joke") + }) - await page.keyboard.press("ArrowRight") - await expect(page.locator(joke.tab)).toBeFocused() - await expect(page.locator(joke.panel)).toBeVisible() + test("automatic: on arrow right, select + focus next tab", async () => { + await I.clickTab("nils") + await I.pressKey("ArrowRight") - await page.keyboard.press("ArrowRight") - await expect(page.locator(nils.tab)).toBeFocused() - await expect(page.locator(nils.panel)).toBeVisible() - }) + await I.setTabIsFocused("agnes") + await I.seeTabContent("agnes") + }) - test("on `ArrowLeft`: should select & focus the previous tab", async ({ page }) => { - await page.focus(nils.tab) - await page.keyboard.press("ArrowLeft") + test("automatic: on arrow right, loop focus + selection", async () => { + await I.clickTab("nils") + await I.pressKey("ArrowRight", 3) - await expect(page.locator(joke.tab)).toBeFocused() - await expect(page.locator(joke.panel)).toBeVisible() + await I.setTabIsFocused("nils") + await I.seeTabContent("nils") + }) - await page.keyboard.press("ArrowLeft") - await expect(page.locator(agnes.tab)).toBeFocused() - await expect(page.locator(agnes.panel)).toBeVisible() + test("automatic: on arrow left, select + focus the previous tab", async () => { + await I.clickTab("joke") + await I.pressKey("ArrowLeft") - await page.keyboard.press("ArrowLeft") - await expect(page.locator(nils.tab)).toBeFocused() - await expect(page.locator(nils.panel)).toBeVisible() - }) + await I.setTabIsFocused("agnes") + await I.seeTabContent("agnes") + }) - test("on `Home` should select first tab", async ({ page }) => { - await page.click(joke.tab) - await page.keyboard.press("Home") + test("manual: on arrow right, focus but not select tab", async () => { + await I.controls.select("activationMode", "manual") + + await I.clickTab("nils") + await I.pressKey("ArrowRight") + + await I.setTabIsFocused("agnes") + await I.dontSeeTabContent("agnes") + }) + + test("manual: on home key, focus but not select tab", async () => { + await I.controls.select("activationMode", "manual") + + await I.clickTab("agnes") + await I.pressKey("Home") + + await I.setTabIsFocused("nils") + await I.dontSeeTabContent("nils") + }) + + test("manual: on navigate, select on enter", async () => { + await I.controls.select("activationMode", "manual") + + await I.clickTab("nils") + await I.pressKey("ArrowRight") + await I.pressKey("Enter") + + await I.setTabIsFocused("agnes") + await I.seeTabContent("agnes") + }) - await expect(page.locator(nils.tab)).toBeFocused() - await expect(page.locator(nils.panel)).toBeVisible() - }) + test("loopFocus=false", async () => { + await I.controls.bool("loopFocus", false) - test("on `End` should select last tab", async ({ page }) => { - await page.focus(nils.tab) - await page.keyboard.press("End") + await I.clickTab("joke") + await I.pressKey("ArrowRight") - await expect(page.locator(joke.tab)).toBeFocused() - await expect(page.locator(joke.panel)).toBeVisible() - }) + await I.setTabIsFocused("joke") }) }) diff --git a/examples/vanilla-ts/package.json b/examples/vanilla-ts/package.json index 4286e574fc..972025a22f 100644 --- a/examples/vanilla-ts/package.json +++ b/examples/vanilla-ts/package.json @@ -1,5 +1,5 @@ { - "name": "vanilla", + "name": "vanilla-ts", "private": true, "version": "0.0.0", "type": "module", diff --git a/packages/machines/tabs/src/tabs.connect.ts b/packages/machines/tabs/src/tabs.connect.ts index 48eef675b8..78e3a93c36 100644 --- a/packages/machines/tabs/src/tabs.connect.ts +++ b/packages/machines/tabs/src/tabs.connect.ts @@ -9,6 +9,9 @@ export function connect(state: State, send: Send, normalize const translations = state.context.translations const focused = state.matches("focused") + const isVertical = state.context.orientation === "vertical" + const isHorizontal = state.context.orientation === "horizontal" + function getTriggerState(props: TriggerProps): TriggerState { return { selected: state.context.value === props.value, @@ -48,24 +51,30 @@ export function connect(state: State, send: Send, normalize "data-focus": dataAttr(focused), "aria-orientation": state.context.orientation, "data-orientation": state.context.orientation, - "aria-label": translations.listLabel, + "aria-label": translations?.listLabel, onKeyDown(event) { if (event.defaultPrevented) return const evt = getNativeEvent(event) + if (!isSelfTarget(evt)) return + if (evt.isComposing) return const keyMap: EventKeyMap = { ArrowDown() { - send("ARROW_DOWN") + if (isHorizontal) return + send({ type: "ARROW_NEXT", key: "ArrowDown" }) }, ArrowUp() { - send("ARROW_UP") + if (isHorizontal) return + send({ type: "ARROW_PREV", key: "ArrowUp" }) }, ArrowLeft() { - send("ARROW_LEFT") + if (isVertical) return + send({ type: "ARROW_PREV", key: "ArrowLeft" }) }, ArrowRight() { - send("ARROW_RIGHT") + if (isVertical) return + send({ type: "ARROW_NEXT", key: "ArrowRight" }) }, Home() { send("HOME") @@ -74,7 +83,7 @@ export function connect(state: State, send: Send, normalize send("END") }, Enter() { - send({ type: "ENTER", value: state.context.focusedValue }) + send({ type: "ENTER" }) }, } @@ -105,7 +114,7 @@ export function connect(state: State, send: Send, normalize "aria-selected": triggerState.selected, "data-selected": dataAttr(triggerState.selected), "data-focus": dataAttr(triggerState.focused), - "aria-controls": dom.getContentId(state.context, value), + "aria-controls": triggerState.selected ? dom.getContentId(state.context, value) : undefined, "data-ownedby": dom.getListId(state.context), id: dom.getTriggerId(state.context, value), tabIndex: triggerState.selected ? 0 : -1, @@ -162,8 +171,7 @@ export function connect(state: State, send: Send, normalize transitionProperty: "var(--transition-property)", transitionDuration: state.context.canIndicatorTransition ? "var(--transition-duration, 150ms)" : "0ms", transitionTimingFunction: "var(--transition-timing-function)", - [state.context.orientation === "horizontal" ? "left" : "top"]: - state.context.orientation === "horizontal" ? "var(--left)" : "var(--top)", + [isHorizontal ? "left" : "top"]: isHorizontal ? "var(--left)" : "var(--top)", }, }), } diff --git a/packages/machines/tabs/src/tabs.machine.ts b/packages/machines/tabs/src/tabs.machine.ts index 9a51e1b84a..0319f5aa30 100644 --- a/packages/machines/tabs/src/tabs.machine.ts +++ b/packages/machines/tabs/src/tabs.machine.ts @@ -20,20 +20,14 @@ export function machine(userContext: UserDefinedContext) { orientation: "horizontal", activationMode: "automatic", value: null, - focusedValue: null, indicatorRect: { left: "0px", top: "0px", width: "0px", height: "0px" }, loopFocus: true, - translations: {}, ...ctx, + focusedValue: null, canIndicatorTransition: false, isIndicatorRendered: false, }, - computed: { - isHorizontal: (ctx) => ctx.orientation === "horizontal", - isVertical: (ctx) => ctx.orientation === "vertical", - }, - entry: ["checkRenderedElements", "syncIndicatorRect", "setContentTabIndex"], exit: ["cleanupObserver"], @@ -59,17 +53,10 @@ export function machine(userContext: UserDefinedContext) { states: { idle: { on: { - TAB_FOCUS: [ - { - guard: "selectOnFocus", - target: "focused", - actions: ["setFocusedValue", "setValue"], - }, - { - target: "focused", - actions: "setFocusedValue", - }, - ], + TAB_FOCUS: { + target: "focused", + actions: "setFocusedValue", + }, TAB_CLICK: { target: "focused", actions: ["setFocusedValue", "setValue"], @@ -82,39 +69,49 @@ export function machine(userContext: UserDefinedContext) { target: "focused", actions: ["setFocusedValue", "setValue"], }, - ARROW_LEFT: { - guard: "isHorizontal", - actions: "focusPrevTab", - }, - ARROW_RIGHT: { - guard: "isHorizontal", - actions: "focusNextTab", - }, - ARROW_UP: { - guard: "isVertical", - actions: "focusPrevTab", - }, - ARROW_DOWN: { - guard: "isVertical", - actions: "focusNextTab", - }, - HOME: { - actions: "focusFirstTab", - }, - END: { - actions: "focusLastTab", - }, - ENTER: { - guard: not("selectOnFocus"), - actions: "setValue", - }, - TAB_FOCUS: [ + ARROW_PREV: [ + { + guard: "selectOnFocus", + actions: ["focusPrevTab", "selectFocusedTab"], + }, + { + actions: "focusPrevTab", + }, + ], + ARROW_NEXT: [ + { + guard: "selectOnFocus", + actions: ["focusNextTab", "selectFocusedTab"], + }, + { + actions: "focusNextTab", + }, + ], + HOME: [ + { + guard: "selectOnFocus", + actions: ["focusFirstTab", "selectFocusedTab"], + }, + { + actions: "focusFirstTab", + }, + ], + END: [ { guard: "selectOnFocus", - actions: ["setFocusedValue", "setValue"], + actions: ["focusLastTab", "selectFocusedTab"], + }, + { + actions: "focusLastTab", }, - { actions: "setFocusedValue" }, ], + ENTER: { + guard: not("selectOnFocus"), + actions: "selectFocusedTab", + }, + TAB_FOCUS: { + actions: ["setFocusedValue"], + }, TAB_BLUR: { target: "idle", actions: "clearFocusedValue", @@ -125,12 +122,15 @@ export function machine(userContext: UserDefinedContext) { }, { guards: { - isVertical: (ctx) => ctx.isVertical, - isHorizontal: (ctx) => ctx.isHorizontal, selectOnFocus: (ctx) => ctx.activationMode === "automatic", }, actions: { + selectFocusedTab(ctx) { + raf(() => { + ctx.value = ctx.focusedValue + }) + }, setFocusedValue(ctx, evt) { set.focusedValue(ctx, evt.value) }, diff --git a/packages/machines/tabs/src/tabs.types.ts b/packages/machines/tabs/src/tabs.types.ts index 0812814c67..a5fb66ac6a 100644 --- a/packages/machines/tabs/src/tabs.types.ts +++ b/packages/machines/tabs/src/tabs.types.ts @@ -37,7 +37,7 @@ interface PublicContext extends DirectionProperty, CommonProperties { /** * Specifies the localized strings that identifies the accessibility elements and their states */ - translations: IntlTranslations + translations?: IntlTranslations /** * Whether the keyboard navigation will loop from last tab to first, and vice versa. * @default true @@ -75,18 +75,7 @@ interface PublicContext extends DirectionProperty, CommonProperties { export type UserDefinedContext = RequiredBy -type ComputedContext = Readonly<{ - /** - * @computed - * Whether the tab is in the horizontal orientation - */ - isHorizontal: boolean - /** - * @computed - * Whether the tab is in the vertical orientation - */ - isVertical: boolean -}> +type ComputedContext = Readonly<{}> interface PrivateContext { /**