Skip to content

Commit d14ce51

Browse files
hoxyqhoxyq
authored
refactor[react-devtools]: rewrite context menus (#29049)
## Summary - While rolling out RDT 5.2.0 on Fusebox, we've discovered that context menus don't work well with this environment. The reason for it is the context menu state implementation - in a global context we define a map of registered context menus, basically what is shown at the moment (see deleted Contexts.js file). These maps are not invalidated on each re-initialization of DevTools frontend, since the bundle (react-devtools-fusebox module) is not reloaded, and this results into RDT throwing an error that some context menu was already registered. - We should not keep such data in a global state, since there is no guarantee that this will be invalidated with each re-initialization of DevTools (like with browser extension, for example). - The new implementation is based on a `ContextMenuContainer` component, which will add all required `contextmenu` event listeners to the anchor-element. This component will also receive a list of `items` that will be displayed in the shown context menu. - The `ContextMenuContainer` component is also using `useImperativeHandle` hook to extend the instance of the component, so context menus can be managed imperatively via `ref`: `contextMenu.current?.hide()`, for example. - **Changed**: The option for copying value to clipboard is now hidden for functions. The reasons for it are: - It is broken in the current implementation, because we call `JSON.stringify` on the value, see `packages/react-devtools-shared/src/backend/utils.js`. - I don't see any reasonable value in doing this for the user, since `Go to definition` option is available and you can inspect the real code and then copy it. - We already filter out fields from objects, if their value is a function, because the whole object is passed to `JSON.stringify`. ## How did you test this change? ### Works with element props and hooks: - All context menu items work reliably for props items - All context menu items work reliably or hooks items https://github.com/facebook/react/assets/28902667/5e2d58b0-92fa-4624-ad1e-2bbd7f12678f ### Works with timeline profiler: - All context menu items work reliably: copying, zooming, ... - Context menu automatically closes on the scroll event https://github.com/facebook/react/assets/28902667/de744cd0-372a-402a-9fa0-743857048d24 ### Works with Fusebox: - Produces no errors - Copy to clipboard context menu item works reliably https://github.com/facebook/react/assets/28902667/0288f5bf-0d44-435c-8842-6b57bc8a7a24
1 parent c325aec commit d14ce51

File tree

19 files changed

+809
-642
lines changed

19 files changed

+809
-642
lines changed

‎packages/react-devtools-inline/__tests__/__e2e__/components.test.js‎

Lines changed: 35 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ const devToolsUtils = require('./devtools-utils');
88
const {test, expect} = require('@playwright/test');
99
const config = require('../../playwright.config');
1010
const semver = require('semver');
11+
1112
test.use(config);
1213
test.describe('Components', () => {
1314
let page;
@@ -59,41 +60,56 @@ test.describe('Components', () => {
5960
const isEditableValue = semver.gte(config.use.react_version, '16.8.0');
6061

6162
// Then read the inspected values.
62-
const [propName, propValue] = await page.evaluate(
63+
const {
64+
name: propName,
65+
value: propValue,
66+
existingNameElementsSize,
67+
existingValueElementsSize,
68+
} = await page.evaluate(
6369
isEditable => {
6470
const {createTestNameSelector, findAllNodes} =
6571
window.REACT_DOM_DEVTOOLS;
6672
const container = document.getElementById('devtools');
6773

6874
// Get name of first prop
69-
const selectorName = isEditable.name
75+
const nameSelector = isEditable.name
7076
? 'EditableName'
7177
: 'NonEditableName';
72-
const nameElement = findAllNodes(container, [
73-
createTestNameSelector('InspectedElementPropsTree'),
74-
createTestNameSelector(selectorName),
75-
])[0];
76-
const name = isEditable.name
77-
? nameElement.value
78-
: nameElement.innerText;
79-
8078
// Get value of first prop
81-
const selectorValue = isEditable.value
79+
const valueSelector = isEditable.value
8280
? 'EditableValue'
8381
: 'NonEditableValue';
84-
const valueElement = findAllNodes(container, [
82+
83+
const existingNameElements = findAllNodes(container, [
8584
createTestNameSelector('InspectedElementPropsTree'),
86-
createTestNameSelector(selectorValue),
87-
])[0];
88-
const value = isEditable.value
89-
? valueElement.value
90-
: valueElement.innerText;
85+
createTestNameSelector('KeyValue'),
86+
createTestNameSelector(nameSelector),
87+
]);
88+
const existingValueElements = findAllNodes(container, [
89+
createTestNameSelector('InspectedElementPropsTree'),
90+
createTestNameSelector('KeyValue'),
91+
createTestNameSelector(valueSelector),
92+
]);
9193

92-
return [name, value];
94+
const name = isEditable.name
95+
? existingNameElements[0].value
96+
: existingNameElements[0].innerText;
97+
const value = isEditable.value
98+
? existingValueElements[0].value
99+
: existingValueElements[0].innerText;
100+
101+
return {
102+
name,
103+
value,
104+
existingNameElementsSize: existingNameElements.length,
105+
existingValueElementsSize: existingValueElements.length,
106+
};
93107
},
94108
{name: isEditableName, value: isEditableValue}
95109
);
96110

111+
expect(existingNameElementsSize).toBe(1);
112+
expect(existingValueElementsSize).toBe(1);
97113
expect(propName).toBe('label');
98114
expect(propValue).toBe('"one"');
99115
});
@@ -135,6 +151,7 @@ test.describe('Components', () => {
135151

136152
focusWithin(container, [
137153
createTestNameSelector('InspectedElementPropsTree'),
154+
createTestNameSelector('KeyValue'),
138155
createTestNameSelector('EditableValue'),
139156
]);
140157
});

‎packages/react-devtools-shared/src/backend/utils.js‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -140,11 +140,15 @@ export function serializeToString(data: any): string {
140140
return 'undefined';
141141
}
142142

143+
if (typeof data === 'function') {
144+
return data.toString();
145+
}
146+
143147
const cache = new Set<mixed>();
144148
// Use a custom replacer function to protect against circular references.
145149
return JSON.stringify(
146150
data,
147-
(key, value) => {
151+
(key: string, value: any) => {
148152
if (typeof value === 'object' && value !== null) {
149153
if (cache.has(value)) {
150154
return;

‎packages/react-devtools-shared/src/devtools/ContextMenu/ContextMenu.css‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,4 +6,4 @@
66
overflow: hidden;
77
z-index: 10000002;
88
user-select: none;
9-
}
9+
}

‎packages/react-devtools-shared/src/devtools/ContextMenu/ContextMenu.js‎

Lines changed: 80 additions & 111 deletions
Original file line numberDiff line numberDiff line change
@@ -8,141 +8,110 @@
88
*/
99

1010
import * as React from 'react';
11-
import {useContext, useEffect, useLayoutEffect, useRef, useState} from 'react';
11+
import {useLayoutEffect, createRef} from 'react';
1212
import {createPortal} from 'react-dom';
13-
import {RegistryContext} from './Contexts';
1413

15-
import styles from './ContextMenu.css';
14+
import ContextMenuItem from './ContextMenuItem';
15+
16+
import type {
17+
ContextMenuItem as ContextMenuItemType,
18+
ContextMenuPosition,
19+
ContextMenuRef,
20+
} from './types';
1621

17-
import type {RegistryContextType} from './Contexts';
22+
import styles from './ContextMenu.css';
1823

19-
function repositionToFit(element: HTMLElement, pageX: number, pageY: number) {
24+
function repositionToFit(element: HTMLElement, x: number, y: number) {
2025
const ownerWindow = element.ownerDocument.defaultView;
21-
if (element !== null) {
22-
if (pageY + element.offsetHeight >= ownerWindow.innerHeight) {
23-
if (pageY - element.offsetHeight > 0) {
24-
element.style.top = `${pageY - element.offsetHeight}px`;
25-
} else {
26-
element.style.top = '0px';
27-
}
26+
if (y + element.offsetHeight >= ownerWindow.innerHeight) {
27+
if (y - element.offsetHeight > 0) {
28+
element.style.top = `${y - element.offsetHeight}px`;
2829
} else {
29-
element.style.top = `${pageY}px`;
30+
element.style.top = '0px';
3031
}
32+
} else {
33+
element.style.top = `${y}px`;
34+
}
3135

32-
if (pageX + element.offsetWidth >= ownerWindow.innerWidth) {
33-
if (pageX - element.offsetWidth > 0) {
34-
element.style.left = `${pageX - element.offsetWidth}px`;
35-
} else {
36-
element.style.left = '0px';
37-
}
36+
if (x + element.offsetWidth >= ownerWindow.innerWidth) {
37+
if (x - element.offsetWidth > 0) {
38+
element.style.left = `${x - element.offsetWidth}px`;
3839
} else {
39-
element.style.left = `${pageX}px`;
40+
element.style.left = '0px';
4041
}
42+
} else {
43+
element.style.left = `${x}px`;
4144
}
4245
}
4346

44-
const HIDDEN_STATE = {
45-
data: null,
46-
isVisible: false,
47-
pageX: 0,
48-
pageY: 0,
49-
};
50-
5147
type Props = {
52-
children: (data: Object) => React$Node,
53-
id: string,
48+
anchorElementRef: {current: React.ElementRef<any> | null},
49+
items: ContextMenuItemType[],
50+
position: ContextMenuPosition,
51+
hide: () => void,
52+
ref?: ContextMenuRef,
5453
};
5554

56-
export default function ContextMenu({children, id}: Props): React.Node {
57-
const {hideMenu, registerMenu} =
58-
useContext<RegistryContextType>(RegistryContext);
59-
60-
const [state, setState] = useState(HIDDEN_STATE);
55+
export default function ContextMenu({
56+
anchorElementRef,
57+
position,
58+
items,
59+
hide,
60+
ref = createRef(),
61+
}: Props): React.Node {
62+
// This works on the assumption that ContextMenu component is only rendered when it should be shown
63+
const anchor = anchorElementRef.current;
64+
65+
if (anchor == null) {
66+
throw new Error(
67+
'Attempted to open a context menu for an element, which is not mounted',
68+
);
69+
}
6170

62-
const bodyAccessorRef = useRef(null);
63-
const containerRef = useRef(null);
64-
const menuRef = useRef(null);
71+
const ownerDocument = anchor.ownerDocument;
72+
const portalContainer = ownerDocument.querySelector(
73+
'[data-react-devtools-portal-root]',
74+
);
6575

66-
useEffect(() => {
67-
const element = bodyAccessorRef.current;
68-
if (element !== null) {
69-
const ownerDocument = element.ownerDocument;
70-
containerRef.current = ownerDocument.querySelector(
71-
'[data-react-devtools-portal-root]',
72-
);
76+
useLayoutEffect(() => {
77+
const menu = ((ref.current: any): HTMLElement);
7378

74-
if (containerRef.current == null) {
75-
console.warn(
76-
'DevTools tooltip root node not found; context menus will be disabled.',
77-
);
79+
function hideUnlessContains(event: Event) {
80+
if (!menu.contains(((event.target: any): Node))) {
81+
hide();
7882
}
7983
}
80-
}, []);
8184

82-
useEffect(() => {
83-
const showMenuFn = ({
84-
data,
85-
pageX,
86-
pageY,
87-
}: {
88-
data: any,
89-
pageX: number,
90-
pageY: number,
91-
}) => {
92-
setState({data, isVisible: true, pageX, pageY});
93-
};
94-
const hideMenuFn = () => setState(HIDDEN_STATE);
95-
return registerMenu(id, showMenuFn, hideMenuFn);
96-
}, [id]);
85+
ownerDocument.addEventListener('mousedown', hideUnlessContains);
86+
ownerDocument.addEventListener('touchstart', hideUnlessContains);
87+
ownerDocument.addEventListener('keydown', hideUnlessContains);
9788

98-
useLayoutEffect(() => {
99-
if (!state.isVisible) {
100-
return;
101-
}
89+
const ownerWindow = ownerDocument.defaultView;
90+
ownerWindow.addEventListener('resize', hide);
10291

103-
const menu = ((menuRef.current: any): HTMLElement);
104-
const container = containerRef.current;
105-
if (container !== null) {
106-
// $FlowFixMe[missing-local-annot]
107-
const hideUnlessContains = event => {
108-
if (!menu.contains(event.target)) {
109-
hideMenu();
110-
}
111-
};
112-
113-
const ownerDocument = container.ownerDocument;
114-
ownerDocument.addEventListener('mousedown', hideUnlessContains);
115-
ownerDocument.addEventListener('touchstart', hideUnlessContains);
116-
ownerDocument.addEventListener('keydown', hideUnlessContains);
117-
118-
const ownerWindow = ownerDocument.defaultView;
119-
ownerWindow.addEventListener('resize', hideMenu);
120-
121-
repositionToFit(menu, state.pageX, state.pageY);
122-
123-
return () => {
124-
ownerDocument.removeEventListener('mousedown', hideUnlessContains);
125-
ownerDocument.removeEventListener('touchstart', hideUnlessContains);
126-
ownerDocument.removeEventListener('keydown', hideUnlessContains);
127-
128-
ownerWindow.removeEventListener('resize', hideMenu);
129-
};
130-
}
131-
}, [state]);
92+
repositionToFit(menu, position.x, position.y);
13293

133-
if (!state.isVisible) {
134-
return <div ref={bodyAccessorRef} />;
135-
} else {
136-
const container = containerRef.current;
137-
if (container !== null) {
138-
return createPortal(
139-
<div ref={menuRef} className={styles.ContextMenu}>
140-
{children(state.data)}
141-
</div>,
142-
container,
143-
);
144-
} else {
145-
return null;
146-
}
94+
return () => {
95+
ownerDocument.removeEventListener('mousedown', hideUnlessContains);
96+
ownerDocument.removeEventListener('touchstart', hideUnlessContains);
97+
ownerDocument.removeEventListener('keydown', hideUnlessContains);
98+
99+
ownerWindow.removeEventListener('resize', hide);
100+
};
101+
}, []);
102+
103+
if (portalContainer == null || items.length === 0) {
104+
return null;
147105
}
106+
107+
return createPortal(
108+
<div className={styles.ContextMenu} ref={ref}>
109+
{items.map(({onClick, content}, index) => (
110+
<ContextMenuItem key={index} onClick={onClick} hide={hide}>
111+
{content}
112+
</ContextMenuItem>
113+
))}
114+
</div>,
115+
portalContainer,
116+
);
148117
}

‎packages/react-devtools-shared/src/devtools/ContextMenu/ContextMenuContainer.js‎

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
/**
2+
* Copyright (c) Meta Platforms, Inc. and affiliates.
3+
*
4+
* This source code is licensed under the MIT license found in the
5+
* LICENSE file in the root directory of this source tree.
6+
*
7+
* @flow
8+
*/
9+
10+
import * as React from 'react';
11+
import {useImperativeHandle} from 'react';
12+
13+
import ContextMenu from './ContextMenu';
14+
import useContextMenu from './useContextMenu';
15+
16+
import type {ContextMenuItem, ContextMenuRef} from './types';
17+
18+
type Props = {
19+
anchorElementRef: {
20+
current: React.ElementRef<any> | null,
21+
},
22+
items: ContextMenuItem[],
23+
closedMenuStub?: React.Node | null,
24+
ref?: ContextMenuRef,
25+
};
26+
27+
export default function ContextMenuContainer({
28+
anchorElementRef,
29+
items,
30+
closedMenuStub = null,
31+
ref,
32+
}: Props): React.Node {
33+
const {shouldShow, position, hide} = useContextMenu(anchorElementRef);
34+
35+
useImperativeHandle(
36+
ref,
37+
() => ({
38+
isShown() {
39+
return shouldShow;
40+
},
41+
hide,
42+
}),
43+
[shouldShow, hide],
44+
);
45+
46+
if (!shouldShow) {
47+
return closedMenuStub;
48+
}
49+
50+
return (
51+
<ContextMenu
52+
anchorElementRef={anchorElementRef}
53+
position={position}
54+
hide={hide}
55+
items={items}
56+
ref={ref}
57+
/>
58+
);
59+
}

0 commit comments

Comments
 (0)