Fix spurious autoscroll
Dan committed
Apr 7, 2019 at 21:09 UTC
0a6d637619fe357ee44f43827487d2dbe6051486
2 files changed
+33
-7
src/devtools/views/Components/Element.js
+21
-5
@@ -4,7 +4,7 @@ import React, {
4
Fragment,
5
useCallback,
6
useContext,
7
- useEffect,
7
+ useLayoutEffect,
8
useMemo,
9
useRef,
10
} from 'react';
@@ -19,9 +19,11 @@ import styles from './Element.css';
19
type Props = {
20
index: number,
21
style: Object,
22
+ // TODO: I can't get the correct type to work here:
23
+ data: Object,
24
};
25
24
-export default function ElementView({ index, style }: Props) {
26
+export default function ElementView({ index, style, data }: Props) {
27
const {
28
baseDepth,
29
getElementAtIndex,
@@ -29,11 +31,11 @@ export default function ElementView({ index, style }: Props) {
31
selectedElementID,
32
selectElementByID,
33
} = useContext(TreeContext);
32
-
34
const element = getElementAtIndex(index);
35
36
const id = element === null ? null : element.id;
37
const isSelected = selectedElementID === id;
38
+ const lastScrolledIDRef = data.lastScrolledIDRef;
39
40
const handleDoubleClick = useCallback(() => {
41
if (id !== null) {
@@ -43,8 +45,22 @@ export default function ElementView({ index, style }: Props) {
45
46
const ref = useRef<HTMLSpanElement | null>(null);
47
46
- useEffect(() => {
48
+ // The tree above has its own autoscrolling, but it only works for rows.
49
+ // However, even when the row gets into the viewport, the component name
50
+ // might be too far left or right on the screen. Adjust it in this case.
51
+ useLayoutEffect(() => {
52
if (isSelected) {
53
+ // Don't select the same item twice.
54
+ // A row may appear and disappear just by scrolling:
55
+ // https://github.com/bvaughn/react-devtools-experimental/issues/67
56
+ // It doesn't necessarily indicate a user action.
57
+ // TODO: we might want to revamp the autoscroll logic
58
+ // to only happen explicitly for user-initiated events.
59
+ if (lastScrolledIDRef.current === id) {
60
+ return;
61
+ }
62
+ lastScrolledIDRef.current = id;
63
+
64
if (ref.current !== null) {
65
ref.current.scrollIntoView({
66
behavior: 'auto',
@@ -53,7 +69,7 @@ export default function ElementView({ index, style }: Props) {
69
});
70
}
71
}
56
- }, [isSelected]);
72
+ }, [id, isSelected, lastScrolledIDRef]);
73
74
// TODO Add click and key handlers for toggling element open/close state.
75
src/devtools/views/Components/Tree.js
+12
-2
@@ -38,13 +38,22 @@ export default function Tree(props: Props) {
38
const { lineHeight } = useContext(SettingsContext);
39
40
// Make sure a newly selected element is visible in the list.
41
- // This is helpful for things like the owners list.
41
+ // This is helpful for things like the owners list and search.
42
useLayoutEffect(() => {
43
if (selectedElementIndex !== null && listRef.current != null) {
44
listRef.current.scrollToItem(selectedElementIndex);
45
+ // Note this autoscroll only works for rows.
46
+ // There's another autoscroll inside the elements
47
+ // that ensures the component name is visible horizontally.
48
+ // It's too early to do it now because the row might not exist yet.
49
}
50
}, [listRef, selectedElementIndex]);
51
52
+ // This ref is passed down the context to elements.
53
+ // It lets them avoid autoscrolling to the same item many times
54
+ // when a selected virtual row goes in and out of the viewport.
55
+ const lastScrolledIDRef = useRef(null);
56
+
57
// Navigate the tree with up/down arrow keys.
58
useEffect(() => {
59
if (treeRef.current === null) {
@@ -98,8 +107,9 @@ export default function Tree(props: Props) {
107
baseDepth,
108
numElements,
109
getElementAtIndex,
110
+ lastScrolledIDRef,
111
}),
102
- [baseDepth, numElements, getElementAtIndex]
112
+ [baseDepth, numElements, getElementAtIndex, lastScrolledIDRef]
113
);
114
115
return (