-
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
Conversation
🦋 Changeset detectedLatest commit: 4905192 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Updated focus-zone behavior to respect contenteditable elements.
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.
Pull Request Overview
This PR updates the focus-zone component to respect contenteditable
elements when determining if an element is editable, treating them similarly to traditional input elements for keyboard navigation handling.
- Introduces a new
isEditableElement
utility function that detects editable elements includingcontenteditable
elements - Refactors focus handling logic to use the new utility instead of hardcoded input/textarea checks
- Adds specific multiline input handling for
contenteditable
elements similar to textarea behavior
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
File | Description |
---|---|
src/utils/is-editable-element.ts | New utility function to determine if an element is editable, including contenteditable support |
src/focus-zone.ts | Updated focus handling logic to use the new utility and treat contenteditable elements as multiline inputs |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
||
const isReadonly = | ||
target.ariaReadOnly === 'true' || | ||
target.getAttribute('aria-readonly') === 'true' || |
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 readonly check is duplicated between ariaReadOnly
property and aria-readonly
attribute. The ariaReadOnly
property should already reflect the aria-readonly
attribute value, making the second check redundant.
target.getAttribute('aria-readonly') === 'true' || |
Copilot uses AI. Check for mistakes.
return true | ||
} | ||
|
||
// Some situations we want to ignore with <select> elements | ||
// Some situations we specifically want to ignore with <select> elements | ||
if (activeElement instanceof HTMLSelectElement) { |
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.
if (activeElement instanceof HTMLSelectElement) { | |
if (activeElement instanceof HTMLSelectElement) { | |
// Ignore single-character (typeable) keys, as they change the selection | |
if (keyLength === 1) { | |
return true | |
} |
Copilot uses AI. Check for mistakes.
@@ -0,0 +1,35 @@ | |||
const nonEditableInputTypes = new Set([ |
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.
Copilot detected a code snippet with 2 occurrences. See search results for more details.
Matched Code Snippet
,
'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' ||
target.getAttribute('readonly') !== null
return (
(name === 'select' ||
name === 'textarea' ||
(name === 'input' && !nonEditableInputTypes.has(type)) ||
target
Copilot uses AI. Check for mistakes.
In
focus-zone
we check to see if focus is on an input element before applying focus change commands, since character keys should always be typable in inputs and arrow keys should typically control selection.However, this doesn't currently consider
contenteditable
inputs, which are any HTML element that can act like it's an input.This PR adds a new
isEditableElement
util that determines whether an element is editable. This helper is duplicated from our internal codebase; once this ships I can remove the duplicated code from over there.It also adds some specific handling to reuse the
textarea
multiline logic oncontenteditable
elements.This fixes https://github.com/github/core-ux/issues/669 (internal issue).