Marker charts flickers when search filter is changed

[mstange]

Steps to reproduce:

1. Go to any profile's marker chart, e.g. https://share.firefox.dev/3Y0Xc90
2. Type into the search filter field.

Whenever a new filter is applied, the chart contents disappear for a brief moment and then re-appear.

┆Issue is synchronized with this Jira Task

[arai-a]

This is caused by the following two lines:

profiler/src/components/shared/chart/Viewport.tsx

Lines 304 to 305 in 0d72df8

	this.setState(this.getDefaultState(newProps));
	this._setSizeNextFrame();

which is performed when the viewportNeedsUpdate property returns true

profiler/src/components/shared/chart/Viewport.tsx

Lines 295 to 305 in 0d72df8

	override UNSAFE_componentWillReceiveProps(
	newProps: ChartViewportImplProps<OwnProps>
	) {
	if (
	this.props.viewportProps.viewportNeedsUpdate(
	this.props.chartProps,
	newProps.chartProps
	)
	) {
	this.setState(this.getDefaultState(newProps));
	this._setSizeNextFrame();

profiler/src/components/marker-chart/index.tsx

Lines 196 to 202 in 0d72df8

	// This function is given the MarkerChartCanvas's chartProps.
	function viewportNeedsUpdate(
	prevProps: { readonly markerTimingAndBuckets: MarkerTimingAndBuckets },
	newProps: { readonly markerTimingAndBuckets: MarkerTimingAndBuckets }
	) {
	return prevProps.markerTimingAndBuckets !== newProps.markerTimingAndBuckets;
	}

This resets the viewport size to 0x0, and canvas size is also set to 0x0 in the following, due to containerWidth and containerHeight being zero.
This results in the flash, until the canvas size is set to the correct size in the next rendering.

profiler/src/components/shared/chart/Viewport.tsx

Lines 254 to 264 in 0d72df8

	getDefaultState(props: ChartViewportImplProps<OwnProps>) {
	const { previewSelection, timeRange } = props.viewportProps;
	const horizontalViewport = this.getHorizontalViewport(
	previewSelection,
	timeRange
	);
	const { startsAtBottom, maxViewportHeight } = props.viewportProps;
	return {
	containerWidth: 0,
	containerHeight: 0,

profiler/src/components/shared/chart/Canvas.tsx

Lines 142 to 159 in 0d72df8

	const devicePixelWidth: DevicePixels = Math.floor(
	containerWidth * devicePixelRatio
	);
	const devicePixelHeight: DevicePixels = Math.floor(
	containerHeight * devicePixelRatio
	);
	if (
	canvas.width !== devicePixelWidth ||
	canvas.height !== devicePixelHeight
	) {
	// The canvas needs to be sized to the container.
	canvas.width = devicePixelWidth;
	canvas.height = devicePixelHeight;
	// Set the CSS width depending on the actual values of canvas.width and height.
	// They can be float values depending on the value of devicePixelRatio.
	canvas.style.width = canvas.width / devicePixelRatio + 'px';
	canvas.style.height = canvas.height / devicePixelRatio + 'px';

Then, even if I comment out the above this.setState and _setSizeNextFrame calls in UNSAFE_componentWillReceiveProps, no test fails (maybe it's just that there's no testcase tho).

Also, the flame graph's viewportNeedsUpdate explicitly mentions the similar problem.

profiler/src/components/flame-graph/FlameGraph.tsx

Lines 424 to 430 in 0d72df8

	function viewportNeedsUpdate() {
	// By always returning false we prevent the viewport from being
	// reset and scrolled all the way to the bottom when doing
	// operations like changing the time selection or applying a
	// transform.
	return false;
	}

Thus, possible options would be:

• (a) if the state reset is really unnecessary, just omit the logic, including viewportNeedsUpdate
• (b) if the state reset is necessary but it's not necessary to resize the canvas, reduce the effect of the state reset
• (c) if the state and canvas size reset are necessary to calculate the correct viewport size:
  • (c-1) somehow avoid the re-rendering while the canvas size is set to 0x0
  • (c-2) copy the canvas content to different element that doesn't contribute to the viewport size (maybe set it to the background image or something)