-
Notifications
You must be signed in to change notification settings - Fork 342
Keep focus in overlay editor #915
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?
Conversation
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 improves overlay editor focus behavior by switching from disabled to readOnly on native inputs, removing auto-focused textareas in specific editors, and making the overlay container itself focusable and auto-focused on mount.
- Use readOnly instead of disabled for editable elements to preserve text selection.
- Remove per-editor autoFocus
<textarea>
and focus the overlay wrapper. - Add default export for MarkdownOverlayEditor and update tests to cover open/close cycles.
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
setup-react-18-test.sh | Added @testing-library/dom to test dependencies |
packages/core/test/data-grid-overlay.test.tsx | Updated import of MarkdownOverlayEditor to default import |
packages/core/test/data-editor.test.tsx | Renamed test and added a new test covering multiple overlays |
packages/core/src/internal/data-grid-overlay-editor/**/*.tsx | Removed per-editor <textarea> , added readOnly and focus logic on overlay container |
packages/core/src/cells/*.tsx | Replaced disabled props with readOnly on cell renderers |
Comments suppressed due to low confidence (4)
packages/core/src/internal/data-grid-overlay-editor/private/uri-overlay-editor.tsx:46
- Removing this
<textarea>
leaves the URI overlay without any focusable or copyable element, preventing users from selecting or copying the URI. Consider keeping a readOnly input or textarea to preserve content selection.
<textarea className="gdg-input" autoFocus={true} />
packages/core/src/internal/data-grid-overlay-editor/private/markdown-overlay-editor.tsx:61
- The markdown overlay no longer renders a textarea for editing, so users cannot modify or copy markdown content. Reintroduce a readOnly or editable textarea inside the overlay container.
<textarea className="gdg-md-edit-textarea gdg-input" autoFocus={true} />
packages/core/src/internal/data-grid-overlay-editor/private/bubbles-overlay-editor.tsx:16
- Removing the textarea here removes the user’s ability to select or copy the bubble text. Consider rendering a readOnly textarea or another focusable element for copy operations.
<textarea className="gdg-input" autoFocus={true} />
packages/core/test/data-editor.test.tsx:1390
- The new test covers only the first five overlay types (bubble through number) and omits the URI overlay. To fully exercise all overlay editors, include the URI column in the test.
const columns = basicProps.columns.slice(0, 5);
ref={elem => { | ||
ref(elem); | ||
if (elem) elem.focus(); | ||
}} |
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.
[nitpick] Using an inline ref callback to both forward the ref and call elem.focus()
may trigger focus on every render. It’s better to forward the ref normally and call focus once in a useEffect
on mount.
ref={elem => { | |
ref(elem); | |
if (elem) elem.focus(); | |
}} | |
ref={ref} |
Copilot uses AI. Check for mistakes.
@@ -47,6 +47,10 @@ export const DataGridOverlayEditorStyle = styled.div<Props>` | |||
0px 6px 12px rgba(62, 65, 86, 0.15); | |||
|
|||
animation: glide_fade_in 60ms 1; | |||
|
|||
&:focus { | |||
outline: none; |
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.
[nitpick] Removing the default focus outline without providing an alternative focus indicator can harm keyboard accessibility. Consider adding a visible focus style to meet accessibility guidelines.
outline: none; | |
outline: none; | |
box-shadow: 0 0 0 2px var(--gdg-focus-color, #4D90FE); /* Default to a blue focus ring */ |
Copilot uses AI. Check for mistakes.
I think this looks fine. But also a bit risky since it touches a very important part. I will try to do another iteration and check it out locally + read a bit about potential downsides. |
Not a blocker for me; it is important that we find a way to always maintain focus if user is interacting with the grid, and not have it be lost unexpectedly to body. There can be other alternatives to this PR as well. |
Tries to fix #910
readOnly
instead of disabled; this allows the element to be focus-able but not editable. Benefit of this is that selection still works if a person wants to copy the cell content.We should add some tests to check that click within cell and then pressing escape works for each internal cell editor.