From 150a4e48f2fabe8b487db680e3690d6518bf71f6 Mon Sep 17 00:00:00 2001 From: George Berezhnoy Date: Sat, 29 Aug 2026 18:55:09 +0100 Subject: [PATCH 1/4] Keep search results and nested-level keys in sync on mobile Two gaps reported while reviewing the a11y work, left out of that branch to keep it reviewable. Search results went stale whenever the item list changed underneath them: addItem()/removeItemByName() updated the searchable set but never re-ran the query, so a newcomer joined the results whether it matched or not and the announced count went with it. SearchInput.reapplyQuery() re-runs whatever is already typed, and PopoverDesktop calls it after either mutation. The mobile popover ignored children.isFlippable. It renders nested levels into the same panel rather than into a popover of their own, so the Flipper navigating the root list carried on claiming the arrows and Enter after drilling in - an item built around a text input could never receive them. The flag now travels with the level through PopoverStatesHistory, and a level that opted out leaves the Flipper deactivated, its items becoming individual stops of the panel's Tab trap instead. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01F3JEisQDVD82VYAp55Lhcp --- .../e2e/fixtures/mobile-nested-input.html | 70 +++++++++++++++++++ .../e2e/tests/header-and-search.spec.ts | 32 ++++++++- .../ui-kit/e2e/tests/mobile-dialog.spec.ts | 23 ++++++ packages/ui-kit/e2e/tests/utils.ts | 1 + .../components/search-input/search-input.ts | 18 +++++ .../ui-kit/src/popover/popover-desktop.ts | 2 + packages/ui-kit/src/popover/popover-mobile.ts | 52 +++++++++++--- .../popover/utils/popover-states-history.ts | 17 +++++ 8 files changed, 204 insertions(+), 11 deletions(-) create mode 100644 packages/ui-kit/e2e/fixtures/mobile-nested-input.html diff --git a/packages/ui-kit/e2e/fixtures/mobile-nested-input.html b/packages/ui-kit/e2e/fixtures/mobile-nested-input.html new file mode 100644 index 0000000..963272a --- /dev/null +++ b/packages/ui-kit/e2e/fixtures/mobile-nested-input.html @@ -0,0 +1,70 @@ + + + + + + UI Kit e2e — mobile with a non-flippable nested level + + +
+ +
+ +
+
+ + + + diff --git a/packages/ui-kit/e2e/tests/header-and-search.spec.ts b/packages/ui-kit/e2e/tests/header-and-search.spec.ts index 2e7f43c..c8f355a 100644 --- a/packages/ui-kit/e2e/tests/header-and-search.spec.ts +++ b/packages/ui-kit/e2e/tests/header-and-search.spec.ts @@ -1,5 +1,5 @@ import { expect, test } from '@playwright/test'; -import { hidePopover, showPopover } from './utils'; +import { addItem, hidePopover, showPopover } from './utils'; test.describe('search input', () => { test.beforeEach(async ({ page }) => { @@ -94,6 +94,36 @@ test.describe('search input', () => { await expect(page.getByRole('menuitem', { name: 'Simple Item' })).toBeFocused(); }); + test('an item added while a query is typed is filtered by that query', async ({ page }) => { + /** + * Changing the item list leaves the results describing a list that no longer exists: the + * newcomer used to show up among the matches whether it matched or not, and the announced + * count went with it + */ + await page.getByRole('searchbox', { name: 'Search' }).fill('Align'); + await expect(page.getByRole('menuitemradio')).toHaveCount(2); + + await addItem(page, { + title: 'Align Right', + name: 'align-right', + toggle: 'align', + }); + + const matchesAfterAdding = 3; + + await expect(page.getByRole('menuitemradio')).toHaveCount(matchesAfterAdding); + await expect(page.getByRole('status').first()).toHaveText(`${matchesAfterAdding} results`); + + await addItem(page, { + title: 'Strikethrough', + name: 'strike', + }); + + /** Does not match, so it stays out of the results rather than joining them */ + await expect(page.getByRole('menuitem', { name: 'Strikethrough' })).toHaveCount(0); + await expect(page.locator('[data-item-name="strike"]')).toBeHidden(); + }); + test('arrow navigation after clicking a result stays within the matches', async ({ page }) => { await page.getByRole('searchbox', { name: 'Search' }).fill('Align'); diff --git a/packages/ui-kit/e2e/tests/mobile-dialog.spec.ts b/packages/ui-kit/e2e/tests/mobile-dialog.spec.ts index 4f164fc..c2c727b 100644 --- a/packages/ui-kit/e2e/tests/mobile-dialog.spec.ts +++ b/packages/ui-kit/e2e/tests/mobile-dialog.spec.ts @@ -135,6 +135,29 @@ test.describe('mobile popover', () => { await expect(back).toBeFocused(); }); + test('a nested level with isFlippable false leaves its keys to its own controls', async ({ page }) => { + /** + * Nested levels render into the same panel as the root one, so the Flipper that navigates + * the root list would carry on claiming the arrows and Enter here too - and an item built + * around a text input needs both for itself + */ + await showPopover(page, 'mobileNestedInput'); + + await page.keyboard.press('ArrowDown'); + await page.keyboard.press('Enter'); + + const input = page.getByRole('textbox', { name: 'Nested input' }); + + await expect(input).toBeVisible(); + + await input.fill('editorjs'); + await page.keyboard.press('ArrowDown'); + + /** The Flipper would have moved the focus off to the next item by now */ + await expect(input).toBeFocused(); + await expect(input).toHaveValue('editorjs'); + }); + test('Enter drills into a nested item', async ({ page }) => { await showPopover(page, 'mobile'); diff --git a/packages/ui-kit/e2e/tests/utils.ts b/packages/ui-kit/e2e/tests/utils.ts index 0794b10..32a1369 100644 --- a/packages/ui-kit/e2e/tests/utils.ts +++ b/packages/ui-kit/e2e/tests/utils.ts @@ -16,6 +16,7 @@ export const fixtures = { confirmationToggle: '/e2e/fixtures/confirmation-toggle.html', mobilePlain: '/e2e/fixtures/mobile-plain.html', mobileEmpty: '/e2e/fixtures/mobile-empty.html', + mobileNestedInput: '/e2e/fixtures/mobile-nested-input.html', } as const; /** diff --git a/packages/ui-kit/src/popover/components/search-input/search-input.ts b/packages/ui-kit/src/popover/components/search-input/search-input.ts index 730847b..ae50eb9 100644 --- a/packages/ui-kit/src/popover/components/search-input/search-input.ts +++ b/packages/ui-kit/src/popover/components/search-input/search-input.ts @@ -106,6 +106,24 @@ export class SearchInput extends EventsDispatcher { this.items = items; } + /** + * Runs the query that is already typed in against the item list again. + * + * Adding or removing an item leaves the results describing a list that no longer exists: + * the newcomer shows up among the matches whether it matches or not, and the reported count + * is off. Re-running the query brings both back in sync without the user retyping it + */ + public reapplyQuery(): void { + if (this.searchQuery === undefined || this.searchQuery === '') { + return; + } + + this.emit(SearchInputEvent.Search, { + query: this.searchQuery, + items: this.foundItems, + }); + } + /** * Returns search field element */ diff --git a/packages/ui-kit/src/popover/popover-desktop.ts b/packages/ui-kit/src/popover/popover-desktop.ts index 2519584..5e1af3d 100644 --- a/packages/ui-kit/src/popover/popover-desktop.ts +++ b/packages/ui-kit/src/popover/popover-desktop.ts @@ -259,6 +259,7 @@ export class PopoverDesktop extends PopoverAbstract { if (this.search !== undefined) { this.search.updateItems(this.itemsDefault); + this.search.reapplyQuery(); } } @@ -271,6 +272,7 @@ export class PopoverDesktop extends PopoverAbstract { if (this.search !== undefined) { this.search.updateItems(this.itemsDefault); + this.search.reapplyQuery(); } } diff --git a/packages/ui-kit/src/popover/popover-mobile.ts b/packages/ui-kit/src/popover/popover-mobile.ts index 53f193d..d5c7ebb 100644 --- a/packages/ui-kit/src/popover/popover-mobile.ts +++ b/packages/ui-kit/src/popover/popover-mobile.ts @@ -57,6 +57,13 @@ export class PopoverMobile extends PopoverAbstract { */ private previouslyFocusedElement: HTMLElement | null = null; + /** + * Whether the items currently on screen take part in keyboard navigation. + * Nested levels opt out of it via children.isFlippable, and since they are rendered into the + * same panel rather than into a popover of their own, the flag has to travel with the level + */ + private isLevelFlippable = true; + /** * Construct the instance * @param params - popover params object @@ -123,6 +130,14 @@ export class PopoverMobile extends PopoverAbstract { this.history.push({ items: params.items }); } + /** + * The items share a single Tab stop only while the level they belong to is navigable. + * A level that opted out is walked by Tab like a plain list instead + */ + protected override get hasRovingTabindex(): boolean { + return this.isLevelFlippable && super.hasRovingTabindex; + } + /** * Open popover */ @@ -143,7 +158,10 @@ export class PopoverMobile extends PopoverAbstract { this.scrollLocker.lock(); - this.flipper?.activate(this.flippableElements); + if (this.isLevelFlippable) { + this.flipper?.activate(this.flippableElements); + } + this.toggleItemsTabbable(true); this.listeners.on(document, 'keydown', this.handleKeyDown as (event: Event) => void, { capture: true }); @@ -179,6 +197,7 @@ export class PopoverMobile extends PopoverAbstract { this.listeners.off(document, 'keydown', this.handleKeyDown as (event: Event) => void, { capture: true }); this.history.reset(); + this.isLevelFlippable = this.history.currentIsFlippable; this.isHidden = true; @@ -209,7 +228,7 @@ export class PopoverMobile extends PopoverAbstract { */ protected override showNestedItems(item: PopoverItemDefault): void { /** Show nested items */ - this.updateItemsAndHeader(item.children, item.title); + this.updateItemsAndHeader(item.children, item.title, item.isChildrenFlippable); const close = (parent?: boolean): void => { if (parent === true) { @@ -217,7 +236,7 @@ export class PopoverMobile extends PopoverAbstract { } else { this.history.pop(); - this.updateItemsAndHeader(this.history.currentItems, this.history.currentTitle); + this.updateItemsAndHeader(this.history.currentItems, this.history.currentTitle, this.history.currentIsFlippable); } }; @@ -226,6 +245,7 @@ export class PopoverMobile extends PopoverAbstract { this.history.push({ title: item.title, items: item.children, + isFlippable: item.isChildrenFlippable, }); } @@ -303,12 +323,13 @@ export class PopoverMobile extends PopoverAbstract { * Moves focus inside the dialog once it is opened. * * The menu is entered at its first item, so that the arrows navigate from there right away. - * Without a Flipper ('flippable: false'), or with a list that has nothing for it to point at - * (only separators, for example), the trap's own first stop is used instead. And when there - * is no stop at all, the dialog itself takes the focus + * Without a Flipper ('flippable: false'), on a nested level that opted out of keyboard + * navigation, or with a list that has nothing for it to point at (only separators, for + * example), the trap's own first stop is used instead. And when there is no stop at all, + * the dialog itself takes the focus */ private focusFirstElement(): void { - if (this.flipper !== undefined && this.flippableElements.length > 0) { + if (this.flipper !== undefined && this.isLevelFlippable && this.flippableElements.length > 0) { this.flipper.focusFirst(); return; @@ -344,8 +365,9 @@ export class PopoverMobile extends PopoverAbstract { * Removes rendered popover items and header and displays new ones * @param items - new popover items * @param title - new popover header text + * @param isFlippable - false if the new items opted out of keyboard navigation */ - private updateItemsAndHeader(items: PopoverItemParams[], title?: string): void { + private updateItemsAndHeader(items: PopoverItemParams[], title?: string, isFlippable = true): void { /** Re-render header */ if (this.header !== null && this.header !== undefined) { this.header.destroy(); @@ -358,7 +380,7 @@ export class PopoverMobile extends PopoverAbstract { onBackButtonClick: () => { this.history.pop(); - this.updateItemsAndHeader(this.history.currentItems, this.history.currentTitle); + this.updateItemsAndHeader(this.history.currentItems, this.history.currentTitle, this.history.currentIsFlippable); }, }); const headerEl = this.header.getElement(); @@ -379,6 +401,8 @@ export class PopoverMobile extends PopoverAbstract { this.renderItems(this.items); + this.isLevelFlippable = isFlippable; + if (!this.isHidden) { /** * Deactivated before being re-activated, so that the Flipper drops its cursor while it @@ -386,7 +410,15 @@ export class PopoverMobile extends PopoverAbstract { * the cursor is left at */ this.flipper?.deactivate(); - this.flipper?.activate(this.flippableElements); + + /** + * A level that opted out of keyboard navigation leaves the Flipper deactivated, so it + * stops claiming the arrows and Enter: an item holding a text input needs those for + * itself. Its items become individual stops of the panel's Tab trap instead + */ + if (this.isLevelFlippable) { + this.flipper?.activate(this.flippableElements); + } /** Element that was focused has just been removed, so focus is moved into the new list */ this.toggleItemsTabbable(true); diff --git a/packages/ui-kit/src/popover/utils/popover-states-history.ts b/packages/ui-kit/src/popover/utils/popover-states-history.ts index 5822c06..1286754 100644 --- a/packages/ui-kit/src/popover/utils/popover-states-history.ts +++ b/packages/ui-kit/src/popover/utils/popover-states-history.ts @@ -13,6 +13,12 @@ interface PopoverStatesHistoryItem { * Popover items */ items: PopoverItemParams[]; + + /** + * False if the items of this state opted out of keyboard navigation. + * Undefined is treated as navigable, which is the default for every level + */ + isFlippable?: boolean; } /** @@ -61,6 +67,17 @@ export class PopoverStatesHistory { return this.history[this.history.length - 1].items; } + /** + * Whether the items of the current state take part in keyboard navigation + */ + public get currentIsFlippable(): boolean { + if (this.history.length === 0) { + return true; + } + + return this.history[this.history.length - 1].isFlippable !== false; + } + /** * Returns history to initial popover state */ From f52486918ab05193616df26720b6646311be0ad0 Mon Sep 17 00:00:00 2001 From: George Berezhnoy Date: Wed, 23 Sep 2026 18:59:34 +0100 Subject: [PATCH 2/4] Keep the mobile level history in step with the level on screen Two ways the history could disagree with the panel it describes: - The root state was pushed without its flippability, and the flag it feeds was initialised to true regardless of the 'flippable' param, so a popover that opted out of keyboard navigation still claimed to navigate. Nothing acted on it, since such a popover has no Flipper at all, but every read of the flag had to be guarded for it. - A nested level was rendered before its state was pushed, so a close arriving synchronously from onChildrenOpen popped the root state instead - the only one on the stack at that point. Co-Authored-By: Claude Opus 5 --- .../mobile-self-closing-children.html | 61 +++++++++++++++++++ .../ui-kit/e2e/tests/mobile-dialog.spec.ts | 16 +++++ packages/ui-kit/e2e/tests/utils.ts | 1 + packages/ui-kit/src/popover/popover-mobile.ts | 31 +++++++--- 4 files changed, 99 insertions(+), 10 deletions(-) create mode 100644 packages/ui-kit/e2e/fixtures/mobile-self-closing-children.html diff --git a/packages/ui-kit/e2e/fixtures/mobile-self-closing-children.html b/packages/ui-kit/e2e/fixtures/mobile-self-closing-children.html new file mode 100644 index 0000000..935384d --- /dev/null +++ b/packages/ui-kit/e2e/fixtures/mobile-self-closing-children.html @@ -0,0 +1,61 @@ + + + + + UI Kit e2e — mobile with a self-closing nested level + + +
+
+ +
+
+ + + + diff --git a/packages/ui-kit/e2e/tests/mobile-dialog.spec.ts b/packages/ui-kit/e2e/tests/mobile-dialog.spec.ts index c2c727b..a643deb 100644 --- a/packages/ui-kit/e2e/tests/mobile-dialog.spec.ts +++ b/packages/ui-kit/e2e/tests/mobile-dialog.spec.ts @@ -334,6 +334,22 @@ test.describe('mobile dialog focus edge cases', () => { }); }); +test.describe('nested levels', () => { + test('returns to the root level when a nested one closes itself as it opens', async ({ page }) => { + await showPopover(page, 'mobileSelfClosingChildren'); + + await page.getByText('Closes itself').click(); + + /** + * The level pushes its state before rendering, so the close that arrives while it is still + * opening pops that state rather than the root one underneath it + */ + await expect(page.getByRole('menuitem', { name: 'Simple item' })).toBeVisible(); + await expect(page.getByRole('menuitem', { name: 'Closes itself' })).toBeVisible(); + await expect(page.getByRole('menuitem', { name: 'Never seen' })).toHaveCount(0); + }); +}); + test.describe('mobile dialog name', () => { test('is named even without a label of its own', async ({ page }) => { await showPopover(page, 'mobile'); diff --git a/packages/ui-kit/e2e/tests/utils.ts b/packages/ui-kit/e2e/tests/utils.ts index 32a1369..3e37168 100644 --- a/packages/ui-kit/e2e/tests/utils.ts +++ b/packages/ui-kit/e2e/tests/utils.ts @@ -17,6 +17,7 @@ export const fixtures = { mobilePlain: '/e2e/fixtures/mobile-plain.html', mobileEmpty: '/e2e/fixtures/mobile-empty.html', mobileNestedInput: '/e2e/fixtures/mobile-nested-input.html', + mobileSelfClosingChildren: '/e2e/fixtures/mobile-self-closing-children.html', } as const; /** diff --git a/packages/ui-kit/src/popover/popover-mobile.ts b/packages/ui-kit/src/popover/popover-mobile.ts index d5c7ebb..f297fdf 100644 --- a/packages/ui-kit/src/popover/popover-mobile.ts +++ b/packages/ui-kit/src/popover/popover-mobile.ts @@ -60,9 +60,10 @@ export class PopoverMobile extends PopoverAbstract { /** * Whether the items currently on screen take part in keyboard navigation. * Nested levels opt out of it via children.isFlippable, and since they are rendered into the - * same panel rather than into a popover of their own, the flag has to travel with the level + * same panel rather than into a popover of their own, the flag has to travel with the level. + * The root level's own value comes from the 'flippable' construction param */ - private isLevelFlippable = true; + private isLevelFlippable: boolean; /** * Construct the instance @@ -113,7 +114,9 @@ export class PopoverMobile extends PopoverAbstract { * A popover built with 'flippable: false' opts out of keyboard navigation altogether, and * then every item becomes an individual stop of the trap instead - see toggleItemsTabbable() */ - if (params.flippable !== false) { + this.isLevelFlippable = params.flippable !== false; + + if (this.isLevelFlippable) { this.flipper = new Flipper({ items: this.flippableElements, focusedItemClass: popoverItemCls.focused, @@ -127,7 +130,10 @@ export class PopoverMobile extends PopoverAbstract { } /* Save state to history for proper navigation between nested and parent popovers */ - this.history.push({ items: params.items }); + this.history.push({ + items: params.items, + isFlippable: this.isLevelFlippable, + }); } /** @@ -227,6 +233,17 @@ export class PopoverMobile extends PopoverAbstract { * @param item – item object to show nested popover for */ protected override showNestedItems(item: PopoverItemDefault): void { + /** + * Pushed before the level is rendered, and before onChildrenOpen() below is handed a way to + * close it: a close arriving synchronously from there would otherwise pop the root state, + * the only one on the stack at that point, and leave the popover with no state at all + */ + this.history.push({ + title: item.title, + items: item.children, + isFlippable: item.isChildrenFlippable, + }); + /** Show nested items */ this.updateItemsAndHeader(item.children, item.title, item.isChildrenFlippable); @@ -241,12 +258,6 @@ export class PopoverMobile extends PopoverAbstract { }; item.onChildrenOpen(close); - - this.history.push({ - title: item.title, - items: item.children, - isFlippable: item.isChildrenFlippable, - }); } /** From be9f1246519f0463cceb63053d8b50e3a66b8f54 Mon Sep 17 00:00:00 2001 From: George Berezhnoy Date: Wed, 23 Sep 2026 19:01:13 +0100 Subject: [PATCH 3/4] Reopen the mobile popover on the level its history points at Closing resets the level history to the root, but left on screen whatever level the user was on. Reopening then brought that level back up with the root's own flippability, so a nested level that opted out of keyboard navigation had the arrows turned back on over its items - the ones that need those keys for themselves. The level is rendered again on the way in rather than on the way out, so nothing changes under the closing animation. The cursor sync is refused on such a level as well. It could not have landed anywhere as it is, since the Flipper is left holding the previous level's items, but that is a coincidence of the rendering rather than a rule, and the rule is what this states. Co-Authored-By: Claude Opus 5 --- .../ui-kit/e2e/tests/mobile-dialog.spec.ts | 72 +++++++++++++++++++ packages/ui-kit/src/popover/popover-mobile.ts | 33 +++++++++ 2 files changed, 105 insertions(+) diff --git a/packages/ui-kit/e2e/tests/mobile-dialog.spec.ts b/packages/ui-kit/e2e/tests/mobile-dialog.spec.ts index a643deb..3959101 100644 --- a/packages/ui-kit/e2e/tests/mobile-dialog.spec.ts +++ b/packages/ui-kit/e2e/tests/mobile-dialog.spec.ts @@ -350,6 +350,78 @@ test.describe('nested levels', () => { }); }); +test.describe('non-flippable nested level', () => { + test.beforeEach(async ({ page }) => { + await showPopover(page, 'mobileNestedInput'); + + await page.getByText('Has children').click(); + }); + + test('leaves the arrows to the items, however the focus got there', async ({ page }) => { + const input = page.getByLabel('Nested input'); + + await input.focus(); + + /** + * Focus arriving on an item of a level that opted out must not move the navigation cursor + * there: that would re-activate the Flipper, and the arrows would stop reaching the input + */ + await page.keyboard.press('ArrowDown'); + await expect(input).toBeFocused(); + + await page.keyboard.press('ArrowUp'); + await expect(input).toBeFocused(); + }); + + test('walks its items with Tab instead', async ({ page }) => { + await page.keyboard.press('Tab'); + await expect(page.getByLabel('Nested input')).toBeFocused(); + + await page.keyboard.press('Tab'); + await expect(page.getByRole('menuitem', { name: 'Child A' })).toBeFocused(); + }); +}); + +test.describe('reopening', () => { + test('comes back to the root level after being closed on a nested one', async ({ page }) => { + await showPopover(page, 'mobileNestedInput'); + + await page.getByText('Has children').click(); + + await expect(page.getByRole('menuitem', { name: 'Child A' })).toBeVisible(); + + await page.keyboard.press('Escape'); + await callShow(page); + + /** + * Closing resets the level history, so the panel has to come back up showing the root items + * rather than the level the user happened to be on when it was dismissed + */ + await expect(page.getByRole('menuitem', { name: 'Simple item' })).toBeVisible(); + await expect(page.getByRole('menuitem', { name: 'Child A' })).toHaveCount(0); + }); + + test('does not restore keyboard navigation to a level that opted out of it', async ({ page }) => { + await showPopover(page, 'mobileNestedInput'); + + await page.getByText('Has children').click(); + await page.keyboard.press('Escape'); + await callShow(page); + + /** + * The nested level is not navigable, and reopening used to leave its items on screen while + * restoring the root's own flippability - the arrows would then navigate a level whose + * items need those keys for themselves + */ + await expect(page.getByRole('menuitem', { name: 'Simple item' })).toBeFocused(); + + await page.keyboard.press('ArrowDown'); + + await expect(page.getByRole('menuitem', { name: 'Has children' })).toBeFocused(); + await expect(page.getByLabel('Nested input')).toHaveCount(0); + }); +}); + test.describe('mobile dialog name', () => { test('is named even without a label of its own', async ({ page }) => { await showPopover(page, 'mobile'); diff --git a/packages/ui-kit/src/popover/popover-mobile.ts b/packages/ui-kit/src/popover/popover-mobile.ts index f297fdf..1aac427 100644 --- a/packages/ui-kit/src/popover/popover-mobile.ts +++ b/packages/ui-kit/src/popover/popover-mobile.ts @@ -65,6 +65,12 @@ export class PopoverMobile extends PopoverAbstract { */ private isLevelFlippable: boolean; + /** + * Whether the items on screen belong to a nested level rather than to the root one. + * The panel renders every level into the same container, so this is what tells the two apart + */ + private isNestedLevelRendered = false; + /** * Construct the instance * @param params - popover params object @@ -144,6 +150,17 @@ export class PopoverMobile extends PopoverAbstract { return this.isLevelFlippable && super.hasRovingTabindex; } + /** + * A level that opted out of keyboard navigation has no cursor to keep in sync: moving one + * there would re-activate the Flipper and take the arrows away from the items that need them + * for themselves. The Flipper is left holding the previous level's items, which is what + * currently keeps the cursor from landing anywhere - this states the rule rather than + * relying on that + */ + protected override get isFlipperCursorSyncEnabled(): boolean { + return this.isLevelFlippable && super.isFlipperCursorSyncEnabled; + } + /** * Open popover */ @@ -160,6 +177,19 @@ export class PopoverMobile extends PopoverAbstract { this.previouslyFocusedElement = document.activeElement instanceof HTMLElement ? document.activeElement : null; } + /** + * Closing resets the history to the root level, but leaves on screen whatever level the + * user was on. Rendered here rather than on the way out, so that nothing changes under the + * closing animation, and while the dialog still counts as hidden, so this only rebuilds the + * items: the keyboard navigation is set up by the rest of show() below. + * + * Without it a nested level would come back up with the root's own flippability, and one + * that opted out of keyboard navigation would have it turned back on by the activation below + */ + if (this.isNestedLevelRendered) { + this.updateItemsAndHeader(this.history.currentItems, this.history.currentTitle, this.history.currentIsFlippable); + } + super.show(); this.scrollLocker.lock(); @@ -414,6 +444,9 @@ export class PopoverMobile extends PopoverAbstract { this.isLevelFlippable = isFlippable; + /** Only a nested level carries a title, the root one is titleless by construction */ + this.isNestedLevelRendered = title !== undefined; + if (!this.isHidden) { /** * Deactivated before being re-activated, so that the Flipper drops its cursor while it From 82e6107a766e5466c0b746e34a2e5a45d5a3cc7f Mon Sep 17 00:00:00 2001 From: George Berezhnoy Date: Wed, 23 Sep 2026 19:01:13 +0100 Subject: [PATCH 4/4] Keep the navigation cursor where it was when the query is reapplied Adding or removing an item reapplies the search query, which restarts the Flipper. It was restarted without a cursor, on the grounds that giving it one would steal the focus from the search field - but the query is also reapplied while an item, not the field, holds the focus. The highlight then disappeared and the roving tabindex moved to the first item, while the real focus stayed where the user had left it, so the next arrow press silently resumed from the top of the list. The cursor is now restored, but only when the focus was on an item, so typing in the search field is left alone. Co-Authored-By: Claude Opus 5 --- .../e2e/tests/header-and-search.spec.ts | 40 ++++++++++++++++++- .../ui-kit/src/popover/popover-desktop.ts | 17 ++++++++ 2 files changed, 56 insertions(+), 1 deletion(-) diff --git a/packages/ui-kit/e2e/tests/header-and-search.spec.ts b/packages/ui-kit/e2e/tests/header-and-search.spec.ts index c8f355a..8b9f138 100644 --- a/packages/ui-kit/e2e/tests/header-and-search.spec.ts +++ b/packages/ui-kit/e2e/tests/header-and-search.spec.ts @@ -1,5 +1,5 @@ import { expect, test } from '@playwright/test'; -import { addItem, hidePopover, showPopover } from './utils'; +import { addItem, hidePopover, removeItemByName, showPopover } from './utils'; test.describe('search input', () => { test.beforeEach(async ({ page }) => { @@ -144,6 +144,44 @@ test.describe('search input', () => { await page.keyboard.press('ArrowDown'); await expect(page.getByRole('menuitemradio', { name: 'Align Center' })).toBeFocused(); }); + + test('keeps the navigation cursor on the focused result when an item is added', async ({ page }) => { + await page.getByRole('searchbox', { name: 'Search' }).fill('Align'); + + await page.keyboard.press('ArrowDown'); + await expect(page.getByRole('menuitemradio', { name: 'Align Left' })).toBeFocused(); + + /** + * Adding an item reapplies the query, which restarts the Flipper. Without a cursor to + * resume from, the next arrow press would silently start over from the top of the list + * while the focus stayed where the user left it + */ + await addItem(page, { + title: 'Added item', + name: 'added', + }); + + await expect(page.getByRole('menuitemradio', { name: 'Align Left' })).toBeFocused(); + + await page.keyboard.press('ArrowDown'); + await expect(page.getByRole('menuitemradio', { name: 'Align Center' })).toBeFocused(); + }); + + test('keeps the navigation cursor on the focused result when an item is removed', async ({ page }) => { + await page.getByRole('searchbox', { name: 'Search' }).fill('Align'); + + await page.keyboard.press('ArrowDown'); + await page.keyboard.press('ArrowDown'); + await expect(page.getByRole('menuitemradio', { name: 'Align Center' })).toBeFocused(); + + /** Filtered out by the query, so the results the user is navigating do not change */ + await removeItemByName(page, 'bold'); + + await expect(page.getByRole('menuitemradio', { name: 'Align Center' })).toBeFocused(); + + await page.keyboard.press('ArrowUp'); + await expect(page.getByRole('menuitemradio', { name: 'Align Left' })).toBeFocused(); + }); }); test.describe('mobile popover header', () => { diff --git a/packages/ui-kit/src/popover/popover-desktop.ts b/packages/ui-kit/src/popover/popover-desktop.ts index 5e1af3d..93c60dd 100644 --- a/packages/ui-kit/src/popover/popover-desktop.ts +++ b/packages/ui-kit/src/popover/popover-desktop.ts @@ -878,6 +878,15 @@ export class PopoverDesktop extends PopoverAbstract { const flippableElements = data.query === '' ? this.flippableElements : data.items.map(item => (item as PopoverItem).getElement()); if (this.flipper?.isActivated === true) { + /** + * Noted before the Flipper is restarted: re-filtering can happen while an item rather than + * the search field holds the focus (adding or removing an item reapplies the query), and + * restarting the Flipper without a cursor leaves the highlight and the roving tabindex + * behind on the first item while the real focus stays where the user left it + */ + const active = document.activeElement; + const focusedItem = active instanceof HTMLElement && this.nodes.items.contains(active) ? active : null; + /** Update flipper items with only visible */ this.flipper.deactivate(); this.flipper.activate(flippableElements as HTMLElement[]); @@ -888,6 +897,14 @@ export class PopoverDesktop extends PopoverAbstract { * scoped to the currently visible items, so Tab still reaches the filtered results. */ this.toggleItemsTabbable(true, flippableElements as HTMLElement[]); + + /** + * Only when an item was focused: typing in the search field must not move the cursor onto + * an item, since that is what would take the focus away from the query being typed + */ + if (focusedItem !== null) { + this.moveCursorTo(focusedItem); + } } };