From fd07862fa914aa768f3c52e35be71db0bd432287 Mon Sep 17 00:00:00 2001 From: Julien Wajsberg Date: Fri, 5 May 2023 19:22:26 +0200 Subject: [PATCH 1/5] Make our ResizeObserver wrapper more solid --- src/utils/resize-observer-wrapper.js | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/src/utils/resize-observer-wrapper.js b/src/utils/resize-observer-wrapper.js index 67986f0e81..f2e4976b00 100644 --- a/src/utils/resize-observer-wrapper.js +++ b/src/utils/resize-observer-wrapper.js @@ -17,21 +17,34 @@ export type ResizeObserverWrapper = {| function createResizeObserverWrapper() { // This keeps the list of callbacks for each observed element. const callbacks: Map> = new Map(); - const resizeObserver = new ResizeObserver((entries) => { + let _resizeObserver = null; + + function resizeObserverCallback(entries) { for (const entry of entries) { const callbacksForElement = callbacks.get(entry.target); if (callbacksForElement) { callbacksForElement.forEach((callback) => callback(entry.contentRect)); } } - }); + } + + function getResizeObserver() { + if (!_resizeObserver) { + _resizeObserver = new ResizeObserver(resizeObserverCallback); + } + return _resizeObserver; + } + + function stopResizeObserver() { + _resizeObserver = null; + } return { subscribe(element: HTMLElement, callback: ResizeObserverCallback) { const callbacksForElement = callbacks.get(element) ?? new Set(); callbacks.set(element, callbacksForElement); callbacksForElement.add(callback); - resizeObserver.observe(element); + getResizeObserver().observe(element); }, unsubscribe(element: HTMLElement, callback: ResizeObserverCallback) { const callbacksForElement = callbacks.get(element); @@ -44,12 +57,12 @@ function createResizeObserverWrapper() { } if (callbacksForElement.size === 0) { callbacks.delete(element); - resizeObserver.unobserve(element); + getResizeObserver().unobserve(element); } if (callbacks.size === 0) { // It's important to clean this up properly so that tests are behaving // as expected. - _resizeObserverWrapper = null; + stopResizeObserver(); } } else { console.warn( From f0a04d67c6fcf7221a8273805c40212a335100f0 Mon Sep 17 00:00:00 2001 From: Julien Wajsberg Date: Fri, 5 May 2023 19:31:25 +0200 Subject: [PATCH 2/5] Move the visibilitychange handling to the resize observer wrapper --- src/components/shared/WithSize.js | 31 ++-------------------------- src/utils/resize-observer-wrapper.js | 28 ++++++++++++++++++++++--- 2 files changed, 27 insertions(+), 32 deletions(-) diff --git a/src/components/shared/WithSize.js b/src/components/shared/WithSize.js index bef02144c1..a6a33f500d 100644 --- a/src/components/shared/WithSize.js +++ b/src/components/shared/WithSize.js @@ -44,7 +44,6 @@ export function withSize< // See: https://github.com/firefox-devtools/profiler/issues/3062 // eslint-disable-next-line flowtype/no-existential-type return class WithSizeWrapper extends React.PureComponent<*, State> { - _dirtySize: DOMRectReadOnly | null = null; state = { width: 0, height: 0 }; _container: HTMLElement | null; @@ -55,37 +54,15 @@ export function withSize< } this._container = container; getResizeObserverWrapper().subscribe(container, this._resizeListener); - window.addEventListener( - 'visibilitychange', - this._visibilityChangeListener - ); } - // The size is only updated when the document is visible. - // In other cases resizing is registered in _dirtySize. + // The listener is only called when the document is visible. _resizeListener = (contentRect: DOMRectReadOnly) => { const container = this._container; if (!container) { return; } - if (document.hidden) { - this._dirtySize = contentRect; - } else { - this._updateSize(container, contentRect); - } - }; - - // If resizing was registered when the document wasn't visible, - // the size will be updated when the document becomes visible - _visibilityChangeListener = () => { - const container = this._container; - if (!container) { - return; - } - if (!document.hidden && this._dirtySize) { - this._updateSize(container, this._dirtySize); - this._dirtySize = null; - } + this._updateSize(container, contentRect); }; componentWillUnmount() { @@ -94,10 +71,6 @@ export function withSize< getResizeObserverWrapper().unsubscribe(container, this._resizeListener); } - window.removeEventListener( - 'visibilitychange', - this._visibilityChangeListener - ); this._container = null; } diff --git a/src/utils/resize-observer-wrapper.js b/src/utils/resize-observer-wrapper.js index f2e4976b00..78b0a86cde 100644 --- a/src/utils/resize-observer-wrapper.js +++ b/src/utils/resize-observer-wrapper.js @@ -17,26 +17,48 @@ export type ResizeObserverWrapper = {| function createResizeObserverWrapper() { // This keeps the list of callbacks for each observed element. const callbacks: Map> = new Map(); + // This keeps the list of changes while the tab is hidden. + const dirtyChanges: Map = new Map(); + let _resizeObserver = null; + function notifyListenersForElement(element: Element, rect: DOMRectReadOnly) { + const callbacksForElement = callbacks.get(element); + if (callbacksForElement) { + callbacksForElement.forEach((callback) => callback(rect)); + } + } + function resizeObserverCallback(entries) { for (const entry of entries) { - const callbacksForElement = callbacks.get(entry.target); - if (callbacksForElement) { - callbacksForElement.forEach((callback) => callback(entry.contentRect)); + if (document.hidden) { + dirtyChanges.set(entry.target, entry.contentRect); + } else { + notifyListenersForElement(entry.target, entry.contentRect); } } } + function visibilityChangeListener() { + if (!document.hidden) { + dirtyChanges.forEach((rect, element) => + notifyListenersForElement(element, rect) + ); + dirtyChanges.clear(); + } + } + function getResizeObserver() { if (!_resizeObserver) { _resizeObserver = new ResizeObserver(resizeObserverCallback); + window.addEventListener('visibilitychange', visibilityChangeListener); } return _resizeObserver; } function stopResizeObserver() { _resizeObserver = null; + window.removeEventListener('visibilityChange', visibilityChangeListener); } return { From a99818e38e2c492fbf4f3ad4b0fa89507ac9ebc4 Mon Sep 17 00:00:00 2001 From: Julien Wajsberg Date: Thu, 4 May 2023 16:12:09 +0200 Subject: [PATCH 3/5] Use scrollTop instead of getBoundingClientRect to control VirtualList rendering This works around bug-1831148 (https://bugzil.la/1831148). --- src/components/shared/VirtualList.js | 68 +++++++--------------------- 1 file changed, 16 insertions(+), 52 deletions(-) diff --git a/src/components/shared/VirtualList.js b/src/components/shared/VirtualList.js index b1d1dfb5e6..95a8586607 100644 --- a/src/components/shared/VirtualList.js +++ b/src/components/shared/VirtualList.js @@ -154,21 +154,6 @@ type VirtualListInnerProps = {| class VirtualListInner extends React.PureComponent< VirtualListInnerProps > { - _container: ?HTMLElement; - - _takeContainerRef = (element: ?HTMLDivElement) => { - this._container = element; - }; - - /* This method is used by users of this component. */ - /* eslint-disable-next-line react/no-unused-class-component-methods */ - getBoundingClientRect() { - if (this._container) { - return this._container.getBoundingClientRect(); - } - return new DOMRect(0, 0, 0, 0); - } - render() { const { itemHeight, @@ -194,7 +179,6 @@ class VirtualListInner extends React.PureComponent< return (
= {| +ariaActiveDescendant?: null | string, |}; -type Geometry = { - // getBoundingClientRect in the Flow definitions is wrong, and labels the return values - // as a ClientRect, and not a DOMRect. https://github.com/facebook/flow/issues/5475 - // - // Account for that here: - outerRect: DOMRect | ClientRect, - innerRectY: CssPixels, -}; +type VirtualListState = {| + scrollTop: number, + clientHeight: number, +|}; export class VirtualList extends React.PureComponent< - VirtualListProps + VirtualListProps, + VirtualListState > { _container: {| current: HTMLDivElement | null |} = React.createRef(); - _inner: {| current: VirtualListInner | null |} = React.createRef(); - _geometry: ?Geometry; + state = { scrollTop: 0, clientHeight: 0 }; componentDidMount() { document.addEventListener('copy', this._onCopy, false); @@ -285,8 +265,6 @@ export class VirtualList extends React.PureComponent< 'The container was assumed to exist while mounting The VirtualList.' ); } - container.addEventListener('scroll', this._onScroll); - this._onScroll(); // for initial size } componentWillUnmount() { @@ -297,12 +275,13 @@ export class VirtualList extends React.PureComponent< 'The container was assumed to exist while unmounting The VirtualList.' ); } - container.removeEventListener('scroll', this._onScroll); } - _onScroll = () => { - this._geometry = this._queryGeometry(); - this.forceUpdate(); + _onScroll = (event: SyntheticEvent) => { + this.setState({ + scrollTop: event.currentTarget.scrollTop, + clientHeight: event.currentTarget.clientHeight, + }); }; _onCopy = (event: ClipboardEvent) => { @@ -312,29 +291,14 @@ export class VirtualList extends React.PureComponent< } }; - _queryGeometry(): Geometry | void { - const container = this._container.current; - const inner = this._inner.current; - if (!container || !inner) { - return undefined; - } - const outerRect = container.getBoundingClientRect(); - const innerRectY = inner.getBoundingClientRect().top; - return { outerRect, innerRectY }; - } - computeVisibleRange() { const { itemHeight, disableOverscan } = this.props; - if (!this._geometry) { - return { visibleRangeStart: 0, visibleRangeEnd: 100 }; - } - const { outerRect, innerRectY } = this._geometry; + const { scrollTop, clientHeight } = this.state; const overscan = disableOverscan ? 0 : 25; const chunkSize = 16; - let visibleRangeStart = - Math.floor((outerRect.top - innerRectY) / itemHeight) - overscan; + let visibleRangeStart = Math.floor(scrollTop / itemHeight) - overscan; let visibleRangeEnd = - Math.ceil((outerRect.bottom - innerRectY) / itemHeight) + overscan; + Math.ceil((scrollTop + clientHeight) / itemHeight) + overscan; if (!disableOverscan) { visibleRangeStart = Math.floor(visibleRangeStart / chunkSize) * chunkSize; visibleRangeEnd = Math.ceil(visibleRangeEnd / chunkSize) * chunkSize; @@ -502,6 +466,7 @@ export class VirtualList extends React.PureComponent< role={ariaRole} aria-label={ariaLabel} aria-activedescendant={ariaActiveDescendant} + onScroll={this._onScroll} >
extends React.PureComponent< containerWidth={containerWidth} forceRender={forceRender} key={columnIndex} - ref={columnIndex === 0 ? this._inner : undefined} /> ))}
From 54d164253a0bc3fd103526df5b7242e03fc7a83b Mon Sep 17 00:00:00 2001 From: Julien Wajsberg Date: Fri, 5 May 2023 18:48:28 +0200 Subject: [PATCH 4/5] Use a ResizeObserver to get the container height --- src/components/shared/VirtualList.js | 22 ++++++++++++++----- .../components/ProfileCallTreeView.test.js | 6 ++++- 2 files changed, 21 insertions(+), 7 deletions(-) diff --git a/src/components/shared/VirtualList.js b/src/components/shared/VirtualList.js index 95a8586607..4253d06b0a 100644 --- a/src/components/shared/VirtualList.js +++ b/src/components/shared/VirtualList.js @@ -36,6 +36,7 @@ import * as React from 'react'; import classNames from 'classnames'; import range from 'array-range'; +import { getResizeObserverWrapper } from 'firefox-profiler/utils/resize-observer-wrapper'; import type { CssPixels } from 'firefox-profiler/types'; @@ -246,8 +247,10 @@ type VirtualListProps = {| |}; type VirtualListState = {| - scrollTop: number, - clientHeight: number, + // This value is updated from the scroll event. + scrollTop: CssPixels, + // This is updated from a resize observer. + containerHeight: CssPixels, |}; export class VirtualList extends React.PureComponent< @@ -255,7 +258,7 @@ export class VirtualList extends React.PureComponent< VirtualListState > { _container: {| current: HTMLDivElement | null |} = React.createRef(); - state = { scrollTop: 0, clientHeight: 0 }; + state = { scrollTop: 0, containerHeight: 0 }; componentDidMount() { document.addEventListener('copy', this._onCopy, false); @@ -265,6 +268,8 @@ export class VirtualList extends React.PureComponent< 'The container was assumed to exist while mounting The VirtualList.' ); } + + getResizeObserverWrapper().subscribe(container, this._resizeListener); } componentWillUnmount() { @@ -275,12 +280,17 @@ export class VirtualList extends React.PureComponent< 'The container was assumed to exist while unmounting The VirtualList.' ); } + getResizeObserverWrapper().unsubscribe(container, this._resizeListener); } + // The listener is only called when the document is visible. + _resizeListener = (contentRect: DOMRectReadOnly) => { + this.setState({ containerHeight: contentRect.height }); + }; + _onScroll = (event: SyntheticEvent) => { this.setState({ scrollTop: event.currentTarget.scrollTop, - clientHeight: event.currentTarget.clientHeight, }); }; @@ -293,12 +303,12 @@ export class VirtualList extends React.PureComponent< computeVisibleRange() { const { itemHeight, disableOverscan } = this.props; - const { scrollTop, clientHeight } = this.state; + const { scrollTop, containerHeight } = this.state; const overscan = disableOverscan ? 0 : 25; const chunkSize = 16; let visibleRangeStart = Math.floor(scrollTop / itemHeight) - overscan; let visibleRangeEnd = - Math.ceil((scrollTop + clientHeight) / itemHeight) + overscan; + Math.ceil((scrollTop + containerHeight) / itemHeight) + overscan; if (!disableOverscan) { visibleRangeStart = Math.floor(visibleRangeStart / chunkSize) * chunkSize; visibleRangeEnd = Math.ceil(visibleRangeEnd / chunkSize) * chunkSize; diff --git a/src/test/components/ProfileCallTreeView.test.js b/src/test/components/ProfileCallTreeView.test.js index c7df77e4f0..07d24b3bac 100644 --- a/src/test/components/ProfileCallTreeView.test.js +++ b/src/test/components/ProfileCallTreeView.test.js @@ -44,13 +44,14 @@ import { } from '../fixtures/profiles/processed-profile'; import { createGeckoProfile } from '../fixtures/profiles/gecko-profile'; import { autoMockElementSize } from '../fixtures/mocks/element-size'; +import { triggerResizeObservers } from '../fixtures/mocks/resize-observer'; import type { Profile } from 'firefox-profiler/types'; autoMockCanvasContext(); // This makes the bounding box large enough so that we don't trigger -// VirtualList's virtualization. We assert this above. +// VirtualList's virtualization. We assert this below. autoMockElementSize({ width: 1000, height: 2000 }); describe('calltree/ProfileCallTreeView', function () { @@ -381,6 +382,9 @@ describe('calltree/ProfileCallTreeView navigation keys', () => { ); + // This automatically uses the bounding box set in autoMockElementSize. + triggerResizeObservers(); + // Assert that we used a large enough bounding box to include all children. const renderedRows = container.querySelectorAll( '.treeViewRow.treeViewRowScrolledColumns' From c6800c46012c35a3669b44388e3c30cd1f3db0ad Mon Sep 17 00:00:00 2001 From: Julien Wajsberg Date: Fri, 19 May 2023 09:51:06 -0400 Subject: [PATCH 5/5] Review comment: use only small letters when removing the visibilitychange event listener --- src/utils/resize-observer-wrapper.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/utils/resize-observer-wrapper.js b/src/utils/resize-observer-wrapper.js index 78b0a86cde..3adfb228c9 100644 --- a/src/utils/resize-observer-wrapper.js +++ b/src/utils/resize-observer-wrapper.js @@ -58,7 +58,7 @@ function createResizeObserverWrapper() { function stopResizeObserver() { _resizeObserver = null; - window.removeEventListener('visibilityChange', visibilityChangeListener); + window.removeEventListener('visibilitychange', visibilityChangeListener); } return {