Attach DevTools Tree keyboard events to the Tree container (not the document) (#24164)
We used to listen to at the document level for this event. That allowed us to listen to up/down arrow key events while another section of DevTools (like the search input) was focused. This was a minor UX positive. (We had to use ownerDocument rather than document for this, because the DevTools extension renders the Components and Profiler tabs into portals.) This approach caused a problem though: it meant that a react-devtools-inline instance could steal (and prevent/block) keyboard events from other JavaScript on the page– which could even include other react-devtools-inline instances. This is a potential major UX negative. Given the above trade offs, we now listen on the root of the Tree itself.
Brian Vaughn committed
Mar 25, 2022 at 09:41 UTC
a6bdb882b73cd0b2702656d767606c74ac0b6670
1 file changed
+17
-10
packages/react-devtools-shared/src/devtools/views/Components/Tree.js
+17
-10
@@ -126,10 +126,6 @@ export default function Tree(props: Props) {
126
return;
127
}
128
129
- // TODO We should ignore arrow keys if the focus is outside of DevTools.
130
- // Otherwise the inline (embedded) DevTools might change selection unexpectedly,
131
- // e.g. when a text input or a select has focus.
132
-
129
let element;
130
switch (event.key) {
131
case 'ArrowDown':
@@ -192,14 +188,25 @@ export default function Tree(props: Props) {
188
setIsNavigatingWithKeyboard(true);
189
};
190
195
- // It's important to listen to the ownerDocument to support the browser extension.
196
- // Here we use portals to render individual tabs (e.g. Profiler),
197
- // and the root document might belong to a different window.
198
- const ownerDocument = treeRef.current.ownerDocument;
199
- ownerDocument.addEventListener('keydown', handleKeyDown);
191
+ // We used to listen to at the document level for this event.
192
+ // That allowed us to listen to up/down arrow key events while another section
193
+ // of DevTools (like the search input) was focused.
194
+ // This was a minor UX positive.
195
+ //
196
+ // (We had to use ownerDocument rather than document for this, because the
197
+ // DevTools extension renders the Components and Profiler tabs into portals.)
198
+ //
199
+ // This approach caused a problem though: it meant that a react-devtools-inline
200
+ // instance could steal (and prevent/block) keyboard events from other JavaScript
201
+ // on the page– which could even include other react-devtools-inline instances.
202
+ // This is a potential major UX negative.
203
+ //
204
+ // Given the above trade offs, we now listen on the root of the Tree itself.
205
+ const container = treeRef.current;
206
+ container.addEventListener('keydown', handleKeyDown);
207
208
return () => {
202
- ownerDocument.removeEventListener('keydown', handleKeyDown);
209
+ container.removeEventListener('keydown', handleKeyDown);
210
};
211
}, [dispatch, selectedElementID, store]);
212