-
Notifications
You must be signed in to change notification settings - Fork 636
fix(SelectPanel): do not bubble up keyboard events #6900
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
e0efa41
e97259d
a3a3da1
01212de
b34a87e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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' | ||
|
@@ -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 | ||
|
@@ -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 | ||
|
||
// 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 | ||
francinelucca marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if (!isAlphabetKey(event.nativeEvent as KeyboardEvent)) return | ||
|
||
// if this is a typeahead event, don't propagate outside of menu | ||
event.stopPropagation() | ||
} | ||
|
||
return ( | ||
<> | ||
<AnchoredOverlay | ||
|
@@ -773,6 +794,7 @@ function Panel({ | |
} | ||
: {}), | ||
} as React.CSSProperties, | ||
onKeyDown: preventBubbling(overlayProps?.onKeyDown), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we feature flag this to only exist if the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? 👀 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤️