Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
5 changes: 5 additions & 0 deletions .changeset/itchy-readers-yell.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@primer/react": patch
---

fix(SelectPanel): do not bubble up keyboard events
18 changes: 9 additions & 9 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

24 changes: 23 additions & 1 deletion packages/react/src/SelectPanel/SelectPanel.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import {SearchIcon, TriangleDownIcon, XIcon, type IconProps} from '@primer/octicons-react'
import React, {useCallback, useEffect, useMemo, useRef, useState} from 'react'
import React, {useCallback, useEffect, useMemo, useRef, useState, type KeyboardEventHandler} from 'react'
import type {AnchoredOverlayProps} from '../AnchoredOverlay'
import {AnchoredOverlay} from '../AnchoredOverlay'
import type {AnchoredOverlayWrapperAnchorProps} from '../AnchoredOverlay/AnchoredOverlay'
Expand Down Expand Up @@ -27,6 +27,7 @@ import {debounce} from '@github/mini-throttle'
import {useResponsiveValue} from '../hooks/useResponsiveValue'
import type {ButtonProps, LinkButtonProps} from '../Button/types'
import {Banner} from '../Banner'
import {isAlphabetKey} from '../hooks/useMnemonics'

// we add a delay so that it does not interrupt default screen reader announcement and queues after it
const SHORT_DELAY_MS = 500
Expand Down Expand Up @@ -741,6 +742,26 @@ function Panel({
'anchored',
)

const preventBubbling =
(customOnKeyDown: KeyboardEventHandler<HTMLDivElement> | undefined) =>
(event: React.KeyboardEvent<HTMLDivElement>) => {
// skip if a TextInput has focus
customOnKeyDown?.(event)

const activeElement = document.activeElement as HTMLElement
if (activeElement.tagName === 'INPUT' || activeElement.tagName === 'TEXTAREA') return

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.

❤️


// skip if used with modifier to preserve shortcuts like ⌘ + F
const hasModifier = event.ctrlKey || event.altKey || event.metaKey
if (hasModifier) return

// skip if it's not a alphabet key
Comment thread
francinelucca marked this conversation as resolved.
if (!isAlphabetKey(event.nativeEvent as KeyboardEvent)) return

// if this is a typeahead event, don't propagate outside of menu
event.stopPropagation()
}

return (
<>
<AnchoredOverlay
Expand Down Expand Up @@ -773,6 +794,7 @@ function Panel({
}
: {}),
} as React.CSSProperties,
onKeyDown: preventBubbling(overlayProps?.onKeyDown),

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.

Should we feature flag this to only exist if the primer_react_select_panel_remove_active_descendant flag is activated?

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.

We definitely can, I thought you said this was reproducible without it though so I thought it my be beneficial to keep it in regardless? 👀

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.

To be honest, it would let us test it a bit more when staffshipped alongside the FF. I don't anticipate there being any issues with preventing events from bubbling, but I'm curious if anything would come up 🤔

}}
focusTrapSettings={focusTrapSettings}
focusZoneSettings={focusZoneSettings}
Expand Down
8 changes: 4 additions & 4 deletions packages/react/src/hooks/useMnemonics.ts
Original file line number Diff line number Diff line change
Expand Up @@ -82,9 +82,9 @@ export const useMnemonics = (open: boolean, providedRef?: React.RefObject<HTMLEl
[open, containerRef],
)

const isAlphabetKey = (event: KeyboardEvent) => {
return event.key.length === 1 && /[a-z\d]/i.test(event.key)
}

return {containerRef}
}

export const isAlphabetKey = (event: KeyboardEvent) => {
return event.key.length === 1 && /[a-z\d]/i.test(event.key)
}
Loading