-
Notifications
You must be signed in to change notification settings - Fork 12
Respect contenteditable
elements when checking for editability
#594
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
Changes from all commits
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/behaviors": patch | ||
--- | ||
|
||
Respect `contenteditable` elements when checking for editability in `focus-zone` behavior |
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
@@ -0,0 +1,35 @@ | ||||
const nonEditableInputTypes = new Set([ | ||||
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. Copilot detected a code snippet with 2 occurrences. See search results for more details. Matched Code Snippet
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback |
||||
'button', | ||||
'checkbox', | ||||
'color', | ||||
'file', | ||||
'hidden', | ||||
'image', | ||||
'radio', | ||||
'range', | ||||
'reset', | ||||
'submit', | ||||
]) | ||||
|
||||
/** | ||||
* Returns true if `element` is editable - that is, if it can be focused and typed in like an input or textarea. | ||||
*/ | ||||
export function isEditableElement(target: EventTarget | null): boolean { | ||||
if (!(target instanceof HTMLElement)) return false | ||||
|
||||
const name = target.nodeName.toLowerCase() | ||||
const type = target.getAttribute('type')?.toLowerCase() ?? 'text' | ||||
|
||||
const isReadonly = | ||||
target.ariaReadOnly === 'true' || | ||||
target.getAttribute('aria-readonly') === 'true' || | ||||
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. The readonly check is duplicated between
Suggested change
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback |
||||
target.getAttribute('readonly') !== null | ||||
|
||||
return ( | ||||
(name === 'select' || | ||||
name === 'textarea' || | ||||
(name === 'input' && !nonEditableInputTypes.has(type)) || | ||||
target.isContentEditable) && | ||||
!isReadonly | ||||
) | ||||
} |
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.
The removal of the single-character key handling for select elements is problematic. Select elements should ignore typeable characters (keyLength === 1) as they change the selection, but this logic was removed when switching to
isEditable
. This will cause focus zone navigation to trigger inappropriately when typing in select elements.Copilot uses AI. Check for mistakes.