Skip to content
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

Display warnings in tooltips for native events that render sync updates #21975

Merged
merged 2 commits into from
Jul 27, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -509,6 +509,7 @@ function AutoSizedCanvas({data, height, width}: AutoSizedCanvasProps) {
</ContextMenu>
{!isContextMenuShown && !surfaceRef.current.hasActiveView() && (
<EventTooltip
canvasRef={canvasRef}
data={data}
hoveredEvent={hoveredEvent}
origin={mouseLocation}
Expand Down
24 changes: 22 additions & 2 deletions packages/react-devtools-scheduling-profiler/src/EventTooltip.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import useSmartTooltip from './utils/useSmartTooltip';
import styles from './EventTooltip.css';

type Props = {|
canvasRef: {|current: HTMLCanvasElement | null|},
data: ReactProfilerData,
hoveredEvent: ReactHoverContextInfo | null,
origin: Point,
Expand Down Expand Up @@ -102,8 +103,14 @@ function getReactMeasureLabel(type): string | null {
}
}

export default function EventTooltip({data, hoveredEvent, origin}: Props) {
export default function EventTooltip({
canvasRef,
data,
hoveredEvent,
origin,
}: Props) {
const tooltipRef = useSmartTooltip({
canvasRef,
mouseX: origin.x,
mouseY: origin.y,
});
Expand Down Expand Up @@ -209,7 +216,19 @@ const TooltipNativeEvent = ({
nativeEvent: NativeEvent,
tooltipRef: Return<typeof useRef>,
}) => {
const {duration, timestamp, type} = nativeEvent;
const {duration, timestamp, type, warnings} = nativeEvent;

const warningElements = [];
if (warnings !== null) {
warnings.forEach((warning, index) => {
warningElements.push(
<Fragment key={index}>
<div className={styles.DetailsGridLabel}>Warning:</div>
<div>{warning}</div>
</Fragment>,
);
});
}

return (
<div className={styles.Tooltip} ref={tooltipRef}>
Expand All @@ -221,6 +240,7 @@ const TooltipNativeEvent = ({
<div>{formatTimestamp(timestamp)}</div>
<div className={styles.DetailsGridLabel}>Duration:</div>
<div>{formatDuration(duration)}</div>
{warningElements}
</div>
</div>
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ export class NativeEventsView extends View {
showHoverHighlight: boolean,
) {
const {frame} = this;
const {depth, duration, highlight, timestamp, type} = event;
const {depth, duration, timestamp, type, warnings} = event;

baseY += depth * ROW_WITH_BORDER_HEIGHT;

Expand All @@ -152,7 +152,7 @@ export class NativeEventsView extends View {

const drawableRect = intersectionOfRects(eventRect, rect);
context.beginPath();
if (highlight) {
if (warnings !== null) {
context.fillStyle = showHoverHighlight
? COLORS.NATIVE_EVENT_WARNING_HOVER
: COLORS.NATIVE_EVENT_WARNING;
Expand Down Expand Up @@ -182,9 +182,10 @@ export class NativeEventsView extends View {
);

if (trimmedName !== null) {
context.fillStyle = highlight
? COLORS.NATIVE_EVENT_WARNING_TEXT
: COLORS.TEXT_COLOR;
context.fillStyle =
warnings !== null
? COLORS.NATIVE_EVENT_WARNING_TEXT
: COLORS.TEXT_COLOR;

context.fillText(
trimmedName,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -202,9 +202,9 @@ function processTimelineEvent(
const nativeEvent = {
depth,
duration,
highlight: false,
timestamp,
type,
warnings: null,
};

currentProfilerData.nativeEvents.push(nativeEvent);
Expand Down Expand Up @@ -339,8 +339,13 @@ function processTimelineEvent(
const nativeEvent = nativeEventStack[i];
const stopTime = nativeEvent.timestamp + nativeEvent.duration;
if (stopTime > startTime) {
// Warn about sync updates that happen an event handler.
nativeEvent.highlight = true;
const warning =
'An event handler scheduled a synchronous update with React.';
if (nativeEvent.warnings === null) {
nativeEvent.warnings = new Set([warning]);
} else {
nativeEvent.warnings.add(warning);
}
}
}
} else if (
Expand Down
2 changes: 1 addition & 1 deletion packages/react-devtools-scheduling-profiler/src/types.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,9 @@ export type ReactLane = number;
export type NativeEvent = {|
+depth: number,
+duration: Milliseconds,
highlight: boolean,
+timestamp: Milliseconds,
+type: string,
warnings: Set<string> | null,
|};

type BaseReactEvent = {|
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,22 +12,32 @@ import {useLayoutEffect, useRef} from 'react';
const TOOLTIP_OFFSET = 4;

export default function useSmartTooltip({
canvasRef,
mouseX,
mouseY,
}: {
canvasRef: {|current: HTMLCanvasElement | null|},
mouseX: number,
mouseY: number,
}) {
const ref = useRef<HTMLElement | null>(null);

// HACK: Browser extension reports window.innerHeight of 0,
// so we fallback to using the tooltip target element.
let height = window.innerHeight;
let width = window.innerWidth;
const target = canvasRef.current;
if (target !== null) {
const rect = target.getBoundingClientRect();
height = rect.top + rect.height;
width = rect.left + rect.width;
}

useLayoutEffect(() => {
const element = ref.current;
if (element !== null) {
// Let's check the vertical position.
if (
mouseY + TOOLTIP_OFFSET + element.offsetHeight >=
window.innerHeight
) {
if (mouseY + TOOLTIP_OFFSET + element.offsetHeight >= height) {
// The tooltip doesn't fit below the mouse cursor (which is our
// default strategy). Therefore we try to position it either above the
// mouse cursor or finally aligned with the window's top edge.
Expand All @@ -45,7 +55,7 @@ export default function useSmartTooltip({
}

// Now let's check the horizontal position.
if (mouseX + TOOLTIP_OFFSET + element.offsetWidth >= window.innerWidth) {
if (mouseX + TOOLTIP_OFFSET + element.offsetWidth >= width) {
// The tooltip doesn't fit at the right of the mouse cursor (which is
// our default strategy). Therefore we try to position it either at the
// left of the mouse cursor or finally aligned with the window's left
Expand Down