Skip to content

fix: arrow key navigation in search skips keyword buttons #1704

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

Merged
serhalp merged 2 commits into from
Feb 27, 2026
Merged

fix: arrow key navigation in search skips keyword buttons #1704

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4081cc9
fix: arrow key navigation in search skips keyword buttons
serhalp Feb 27, 2026
af31ca9
fix: undo overeager new escape hatch
serhalp Feb 27, 2026
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
1 change: 0 additions & 1 deletion app/components/Package/Card.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,6 @@ const numberFormatter = useNumberFormatter()
size="small"
:aria-pressed="props.filters?.keywords.includes(keyword)"
:title="`Filter by ${keyword}`"
:data-result-index="index"
@click.stop="emit('clickKeyword', keyword)"
>
{{ keyword }}
Expand Down
26 changes: 24 additions & 2 deletions app/pages/search.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -453,7 +453,24 @@ watch(displayResults, results => {
}
})

/**
* Focus the header search input
*/
function focusSearchInput() {
const searchInput = document.querySelector<HTMLInputElement>(
'input[type="search"], input[name="q"]',
)
searchInput?.focus()
}

function handleResultsKeydown(e: KeyboardEvent) {
// Escape returns focus to the search input from anywhere on the page
if (e.key === 'Escape') {
e.preventDefault()
focusSearchInput()
return
}

@coderabbitai coderabbitai Bot Feb 27, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Escape is handled too globally.

At Line 469, preventDefault() runs for every Escape press on the page, even outside result navigation. This can interfere with expected Escape behaviour in other UI contexts (e.g. overlays/popovers). Scope this branch to when a result/suggestion item is focused (or when search navigation state is active).

@danielroe danielroe Feb 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

is this a fair point @serhalp?

@coderabbitai coderabbitai Bot Feb 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

@knowler knowler Feb 27, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, we should have some checks here, since Escape is used for dismissing popover menus and dropdowns and, eventually, tooltips.

@knowler knowler Feb 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Also, FWIW, the / key also returns the focus to the search box. Do we need an additional key?

@serhalp serhalp Feb 27, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks, friends! I was a bit overeager here. Removed the new Esc binding.


// If the active element is an input, navigate to exact match or wait for results
if (e.key === 'Enter' && document.activeElement?.tagName === 'INPUT') {
// Get value directly from input (not from route query, which may be debounced)
Expand Down Expand Up @@ -489,7 +506,12 @@ function handleResultsKeydown(e: KeyboardEvent) {

if (e.key === 'ArrowUp') {
e.preventDefault()
const nextIndex = currentIndex < 0 ? 0 : Math.max(currentIndex - 1, 0)
// At first result or no result focused: return focus to search input
if (currentIndex <= 0) {
focusSearchInput()
return
}
const nextIndex = currentIndex - 1
const el = elements[nextIndex]
if (el) focusElement(el)
return
Expand All @@ -508,7 +530,7 @@ function handleResultsKeydown(e: KeyboardEvent) {
}
}

onKeyDown(['ArrowDown', 'ArrowUp', 'Enter'], handleResultsKeydown)
onKeyDown(['ArrowDown', 'ArrowUp', 'Enter', 'Escape'], handleResultsKeydown)

useSeoMeta({
title: () =>
Expand Down
60 changes: 60 additions & 0 deletions test/e2e/interactions.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,66 @@ test.describe('Search Pages', () => {
await expect(page).toHaveURL(/\/(package|org|user)\/vue/)
})

test('/search?q=vue → ArrowDown navigates only between results, not keyword buttons', async ({
page,
goto,
}) => {
await goto('/search?q=vue', { waitUntil: 'hydration' })

await expect(page.locator('text=/found \\d+|showing \\d+/i').first()).toBeVisible({
timeout: 15000,
})

const firstResult = page.locator('[data-result-index="0"]').first()
const secondResult = page.locator('[data-result-index="1"]').first()
await expect(firstResult).toBeVisible()
await expect(secondResult).toBeVisible()

// ArrowDown from input focuses the first result
await page.keyboard.press('ArrowDown')
await expect(firstResult).toBeFocused()

// Second ArrowDown focuses the second result (not a keyword button within the first)
await page.keyboard.press('ArrowDown')
await expect(secondResult).toBeFocused()
})

test('/search?q=vue → ArrowUp from first result returns focus to search input', async ({
page,
goto,
}) => {
await goto('/search?q=vue', { waitUntil: 'hydration' })

await expect(page.locator('text=/found \\d+|showing \\d+/i').first()).toBeVisible({
timeout: 15000,
})

// Navigate to first result
await page.keyboard.press('ArrowDown')
await expect(page.locator('[data-result-index="0"]').first()).toBeFocused()

// ArrowUp returns to the search input
await page.keyboard.press('ArrowUp')
await expect(page.locator('input[type="search"]')).toBeFocused()
})
Comment on lines +76 to +117

@coderabbitai coderabbitai Bot Feb 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Make the “from search input” precondition explicit in both new keyboard tests.

At Line 91 and Line 110, the tests assert behaviour “from input”, but they never focus/assert the header search input before the first ArrowDown. That allows false positives if global key handling changes.

Suggested hardening diff 
   test('/search?q=vue → ArrowDown navigates only between results, not keyword buttons', async ({
     page,
     goto,
   }) => {
     await goto('/search?q=vue', { waitUntil: 'hydration' })
@@
+    const headerSearchInput = page.locator('#header-search')
+    await headerSearchInput.focus()
+    await expect(headerSearchInput).toBeFocused()
+
     const firstResult = page.locator('[data-result-index="0"]').first()
     const secondResult = page.locator('[data-result-index="1"]').first()
@@
   test('/search?q=vue → ArrowUp from first result returns focus to search input', async ({
     page,
     goto,
   }) => {
     await goto('/search?q=vue', { waitUntil: 'hydration' })
@@
+    const headerSearchInput = page.locator('#header-search')
+    await headerSearchInput.focus()
+    await expect(headerSearchInput).toBeFocused()
+
     // Navigate to first result
     await page.keyboard.press('ArrowDown')
     await expect(page.locator('[data-result-index="0"]').first()).toBeFocused()
@@
     // ArrowUp returns to the search input
     await page.keyboard.press('ArrowUp')
-    await expect(page.locator('input[type="search"]')).toBeFocused()
+    await expect(headerSearchInput).toBeFocused()
   })


test('/search?q=vue → Escape returns focus to search input', async ({ page, goto }) => {
await goto('/search?q=vue', { waitUntil: 'hydration' })

await expect(page.locator('text=/found \\d+|showing \\d+/i').first()).toBeVisible({
timeout: 15000,
})

// Navigate into results
await page.keyboard.press('ArrowDown')
await page.keyboard.press('ArrowDown')
await expect(page.locator('[data-result-index="1"]').first()).toBeFocused()

// Escape returns to the search input
await page.keyboard.press('Escape')
await expect(page.locator('input[type="search"]')).toBeFocused()
})

test('/search?q=vue → "/" focuses the search input from results', async ({ page, goto }) => {
await goto('/search?q=vue', { waitUntil: 'hydration' })

Expand Down
Loading