Commit 9913ac73 by Zoltán Bedi Committed by GitHub

Bug: trace viewer doesn't show more than 300 spans (#29377)

* Fix: trace viewer scroll issue

* Fix lazy loading

* Listview test fixes

* Move scrollElement to props
parent e0dee062
...@@ -2,11 +2,8 @@ ...@@ -2,11 +2,8 @@
exports[`<ListView> shallow tests matches a snapshot 1`] = ` exports[`<ListView> shallow tests matches a snapshot 1`] = `
<div <div
onScroll={[Function]}
style={ style={
Object { Object {
"height": "100%",
"overflowY": "auto",
"position": "relative", "position": "relative",
} }
} }
......
...@@ -69,7 +69,7 @@ describe('<ListView>', () => { ...@@ -69,7 +69,7 @@ describe('<ListView>', () => {
itemsWrapperClassName: 'SomeClassName', itemsWrapperClassName: 'SomeClassName',
viewBuffer: 10, viewBuffer: 10,
viewBufferMin: 5, viewBufferMin: 5,
windowScroller: false, windowScroller: true,
}; };
describe('shallow tests', () => { describe('shallow tests', () => {
...@@ -152,10 +152,6 @@ describe('<ListView>', () => { ...@@ -152,10 +152,6 @@ describe('<ListView>', () => {
instance = wrapper.instance(); instance = wrapper.instance();
}); });
it('getViewHeight() returns the viewHeight', () => {
expect(instance.getViewHeight()).toBe(clientHeight);
});
it('getBottomVisibleIndex() returns a number', () => { it('getBottomVisibleIndex() returns a number', () => {
const n = instance.getBottomVisibleIndex(); const n = instance.getBottomVisibleIndex();
expect(Number.isNaN(n)).toBe(false); expect(Number.isNaN(n)).toBe(false);
...@@ -233,9 +229,9 @@ describe('<ListView>', () => { ...@@ -233,9 +229,9 @@ describe('<ListView>', () => {
}, },
}); });
const hasChanged = instance._isViewChanged(); const hasChanged = instance._isViewChanged();
expect(hasChanged).toBe(true);
expect(spyFns.clientHeight).toHaveBeenCalled(); expect(spyFns.clientHeight).toHaveBeenCalled();
expect(spyFns.scrollTop).toHaveBeenCalled(); expect(spyFns.scrollTop).toHaveBeenCalled();
expect(hasChanged).toBe(true);
}); });
}); });
}); });
......
...@@ -67,7 +67,7 @@ type TListViewProps = { ...@@ -67,7 +67,7 @@ type TListViewProps = {
itemsWrapperClassName?: string; itemsWrapperClassName?: string;
/** /**
* When adding new items to the DOM, this is the number of items to add above * When adding new items to the DOM, this is the number of items to add above
* and below the current view. E.g. if list is 100 items and is srcolled * and below the current view. E.g. if list is 100 items and is scrolled
* halfway down (so items [46, 55] are in view), then when a new range of * halfway down (so items [46, 55] are in view), then when a new range of
* items is rendered, it will render items `46 - viewBuffer` to * items is rendered, it will render items `46 - viewBuffer` to
* `55 + viewBuffer`. * `55 + viewBuffer`.
...@@ -89,15 +89,20 @@ type TListViewProps = { ...@@ -89,15 +89,20 @@ type TListViewProps = {
* - Ref:https://github.com/bvaughn/react-virtualized/blob/497e2a1942529560681d65a9ef9f5e9c9c9a49ba/docs/WindowScroller.md * - Ref:https://github.com/bvaughn/react-virtualized/blob/497e2a1942529560681d65a9ef9f5e9c9c9a49ba/docs/WindowScroller.md
*/ */
windowScroller?: boolean; windowScroller?: boolean;
/**
* You need to pass in scrollElement when windowScroller is set to false.
* This element is responsible for tracking scrolling for lazy loading.
*/
scrollElement?: Element;
}; };
const DEFAULT_INITIAL_DRAW = 300; const DEFAULT_INITIAL_DRAW = 100;
/** /**
* Virtualized list view component, for the most part, only renders the window * Virtualized list view component, for the most part, only renders the window
* of items that are in-view with some buffer before and after. Listens for * of items that are in-view with some buffer before and after. Listens for
* scroll events and updates which items are rendered. See react-virtualized * scroll events and updates which items are rendered. See react-virtualized
* for a suite of components with similar, but generalized, functinality. * for a suite of components with similar, but generalized, functionality.
* https://github.com/bvaughn/react-virtualized * https://github.com/bvaughn/react-virtualized
* *
* Note: Presently, ListView cannot be a PureComponent. This is because ListView * Note: Presently, ListView cannot be a PureComponent. This is because ListView
...@@ -157,9 +162,9 @@ export default class ListView extends React.Component<TListViewProps> { ...@@ -157,9 +162,9 @@ export default class ListView extends React.Component<TListViewProps> {
_windowScrollListenerAdded: boolean; _windowScrollListenerAdded: boolean;
_htmlElm: HTMLElement; _htmlElm: HTMLElement;
/** /**
* HTMLElement holding the scroller. * Element holding the scroller.
*/ */
_wrapperElm: HTMLElement | TNil; _wrapperElm: Element | TNil;
/** /**
* HTMLElement holding the rendered items. * HTMLElement holding the rendered items.
*/ */
...@@ -202,6 +207,10 @@ export default class ListView extends React.Component<TListViewProps> { ...@@ -202,6 +207,10 @@ export default class ListView extends React.Component<TListViewProps> {
} }
window.addEventListener('scroll', this._onScroll); window.addEventListener('scroll', this._onScroll);
this._windowScrollListenerAdded = true; this._windowScrollListenerAdded = true;
} else {
// The wrapper element should be the one that handles the scrolling. Once we are not using scroll-canvas we can remove this.
this._wrapperElm = this.props.scrollElement;
this._wrapperElm?.addEventListener('scroll', this._onScroll);
} }
} }
...@@ -214,6 +223,8 @@ export default class ListView extends React.Component<TListViewProps> { ...@@ -214,6 +223,8 @@ export default class ListView extends React.Component<TListViewProps> {
componentWillUnmount() { componentWillUnmount() {
if (this._windowScrollListenerAdded) { if (this._windowScrollListenerAdded) {
window.removeEventListener('scroll', this._onScroll); window.removeEventListener('scroll', this._onScroll);
} else {
this._wrapperElm?.removeEventListener('scroll', this._onScroll);
} }
} }
...@@ -308,8 +319,11 @@ export default class ListView extends React.Component<TListViewProps> { ...@@ -308,8 +319,11 @@ export default class ListView extends React.Component<TListViewProps> {
}; };
_initWrapper = (elm: HTMLElement | TNil) => { _initWrapper = (elm: HTMLElement | TNil) => {
if (!this.props.windowScroller) {
return;
}
this._wrapperElm = elm; this._wrapperElm = elm;
if (!this.props.windowScroller && elm) { if (elm) {
this._viewHeight = elm.clientHeight; this._viewHeight = elm.clientHeight;
} }
}; };
...@@ -374,8 +388,8 @@ export default class ListView extends React.Component<TListViewProps> { ...@@ -374,8 +388,8 @@ export default class ListView extends React.Component<TListViewProps> {
}; };
/** /**
* Get the height of the element at index `i`; first check the known heigths, * Get the height of the element at index `i`; first check the known heights,
* fallbck to `.props.itemHeightGetter(...)`. * fallback to `.props.itemHeightGetter(...)`.
*/ */
_getHeight = (i: number) => { _getHeight = (i: number) => {
const key = this.props.getKeyFromIndex(i); const key = this.props.getKeyFromIndex(i);
......
...@@ -82,6 +82,7 @@ type TVirtualizedTraceViewOwnProps = { ...@@ -82,6 +82,7 @@ type TVirtualizedTraceViewOwnProps = {
createSpanLink?: ( createSpanLink?: (
span: TraceSpan span: TraceSpan
) => { href: string; onClick?: (e: React.MouseEvent) => void; content: React.ReactNode }; ) => { href: string; onClick?: (e: React.MouseEvent) => void; content: React.ReactNode };
scrollElement?: Element;
}; };
type VirtualizedTraceViewProps = TVirtualizedTraceViewOwnProps & TExtractUiFindFromStateReturn & TTraceTimeline; type VirtualizedTraceViewProps = TVirtualizedTraceViewOwnProps & TExtractUiFindFromStateReturn & TTraceTimeline;
...@@ -445,6 +446,7 @@ export class UnthemedVirtualizedTraceView extends React.Component<VirtualizedTra ...@@ -445,6 +446,7 @@ export class UnthemedVirtualizedTraceView extends React.Component<VirtualizedTra
render() { render() {
const styles = getStyles(); const styles = getStyles();
const { scrollElement } = this.props;
return ( return (
<div> <div>
<ListView <ListView
...@@ -452,12 +454,13 @@ export class UnthemedVirtualizedTraceView extends React.Component<VirtualizedTra ...@@ -452,12 +454,13 @@ export class UnthemedVirtualizedTraceView extends React.Component<VirtualizedTra
dataLength={this.rowStates.length} dataLength={this.rowStates.length}
itemHeightGetter={this.getRowHeight} itemHeightGetter={this.getRowHeight}
itemRenderer={this.renderRow} itemRenderer={this.renderRow}
viewBuffer={300} viewBuffer={50}
viewBufferMin={100} viewBufferMin={50}
itemsWrapperClassName={styles.rowsWrapper} itemsWrapperClassName={styles.rowsWrapper}
getKeyFromIndex={this.getKeyFromIndex} getKeyFromIndex={this.getKeyFromIndex}
getIndexFromKey={this.getIndexFromKey} getIndexFromKey={this.getIndexFromKey}
windowScroller windowScroller={false}
scrollElement={scrollElement}
/> />
</div> </div>
); );
......
...@@ -102,6 +102,7 @@ type TProps = TExtractUiFindFromStateReturn & { ...@@ -102,6 +102,7 @@ type TProps = TExtractUiFindFromStateReturn & {
createSpanLink?: ( createSpanLink?: (
span: TraceSpan span: TraceSpan
) => { href: string; onClick?: (e: React.MouseEvent) => void; content: React.ReactNode }; ) => { href: string; onClick?: (e: React.MouseEvent) => void; content: React.ReactNode };
scrollElement?: Element;
}; };
type State = { type State = {
......
...@@ -83,6 +83,7 @@ export function TraceView(props: Props) { ...@@ -83,6 +83,7 @@ export function TraceView(props: Props) {
); );
const createSpanLink = useMemo(() => createSpanLinkFactory(props.splitOpenFn), [props.splitOpenFn]); const createSpanLink = useMemo(() => createSpanLinkFactory(props.splitOpenFn), [props.splitOpenFn]);
const scrollElement = document.getElementsByClassName('scroll-canvas')[0];
if (!traceProp) { if (!traceProp) {
return null; return null;
...@@ -148,6 +149,7 @@ export function TraceView(props: Props) { ...@@ -148,6 +149,7 @@ export function TraceView(props: Props) {
)} )}
uiFind={search} uiFind={search}
createSpanLink={createSpanLink} createSpanLink={createSpanLink}
scrollElement={scrollElement}
/> />
</UIElementsContext.Provider> </UIElementsContext.Provider>
</ThemeProvider> </ThemeProvider>
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment