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.

Sign up for GitHub

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

fix: fix flickering when expanding/collapsing sections #595

Merged
merged 2 commits into from
Aug 20, 2024
Merged

fix: fix flickering when expanding/collapsing sections #595

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bd6f9e6
fix: fix flickering when expanding/collapsing sections
shadowusr Aug 20, 2024
f0b22fd
fix: fix unit tests
shadowusr Aug 20, 2024
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
122 changes: 57 additions & 65 deletions lib/static/components/details.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,89 +1,81 @@
'use strict';

import React, {Component} from 'react';
import React, {useContext, useLayoutEffect, useState} from 'react';
import PropTypes from 'prop-types';
import classNames from 'classnames';
import {isEmpty, isFunction} from 'lodash';
import {Card, Disclosure} from '@gravity-ui/uikit';
import {Disclosure} from '@gravity-ui/uikit';
import {MeasurementContext} from './measurement-context';

export default class Details extends Component {
static propTypes = {
type: PropTypes.oneOf(['text', 'image']),
title: PropTypes.oneOfType([PropTypes.element, PropTypes.string]).isRequired,
content: PropTypes.oneOfType([PropTypes.func, PropTypes.string, PropTypes.element, PropTypes.array]).isRequired,
extendClassNames: PropTypes.oneOfType([PropTypes.array, PropTypes.string]),
onClick: PropTypes.func,
asHtml: PropTypes.bool
};

state = {isOpened: false};

handleClick = () => {
this.setState((state, props) => {
const newState = {isOpened: !state.isOpened};
export default function Details(props) {
const [isOpened, setIsOpened] = useState(false);

if (props.onClick) {
props.onClick(newState);
}

return newState;
});
};
const handleClick = () => {
const newIsOpened = !isOpened;

stopPropagation = (e) => {
e.stopPropagation();
setIsOpened(newIsOpened);
props.onClick?.({isOpened: newIsOpened});
};

_getContent() {
const content = this.props.content;
const getContent = () => {
const {content} = props;

return isFunction(content) ? content() : content;
}
};

_renderContent() {
if (!this.state.isOpened) {
const renderContent = () => {
if (!isOpened) {
return null;
}

const children = this.props.asHtml ? null : this._getContent();
const extraProps = this.props.asHtml ? {dangerouslySetInnerHTML: {__html: this._getContent()}} : {};
const children = props.asHtml ? null : getContent();
const extraProps = props.asHtml ? {dangerouslySetInnerHTML: {__html: getContent()}} : {};

return <div className='details__content' {...extraProps}>
{children}
</div>;
}
};

render() {
const {type, title, content, extendClassNames} = this.props;
const className = classNames(
'details',
extendClassNames
);
const {title, content, extendClassNames} = props;
const className = classNames(
'details',
extendClassNames
);

return (
isEmpty(content) && !isFunction(content) ? (
<div className={className}>
{title}
</div>
) : (
<Disclosure className={className} onUpdate={this.handleClick}
size='l'>
<Disclosure.Summary>
{(props, defaultButton) => (
<div className={classNames(className, 'details__summary')} aria-controls={props.ariaControls} onClick={props.onClick} id={props.id}>
<div className="details__expand-button" onClick={this.stopPropagation}>
{defaultButton}
</div>
{title}
const {measure} = useContext(MeasurementContext);
useLayoutEffect(() => {
measure?.();
}, [isOpened]);

return (
isEmpty(content) && !isFunction(content) ? (
<div className={className}>
{title}
</div>
) : (
<Disclosure className={className} onUpdate={handleClick} size='l' expanded={isOpened}>
<Disclosure.Summary>
{(props, defaultButton) => (
<div className={classNames(className, 'details__summary')} aria-controls={props.ariaControls} onClick={props.onClick} id={props.id}>
<div className="details__expand-button" onClick={e => e.stopPropagation()}>
{defaultButton}
</div>
)}
</Disclosure.Summary>
{type === 'image' ? this._renderContent() :
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why don't you need this condition now?

Copy link
Member Author

@shadowusr shadowusr Aug 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We wrap components in Card directly where we need it and pass already wrapped components as children.

The Details component shouldn't be aware of anything regarding its children IMO. It shouldn't care what it will render, it just receives ReactNode and renders it as-is. That makes logic a lot easier to understand and reduces complexity. That's from the software design point of view.

Another reason why I got rid of it, because it causes another visual glitch — upon collapsing Meta/History, you'd first see a frame where Disclosure contains empty card and only afterwards it collapses completely.

<Card className='details__card' view='filled'>
{this._renderContent()}
</Card>}
</Disclosure>
)
);
}
{title}
</div>
)}
</Disclosure.Summary>
{renderContent()}
</Disclosure>
)
);
}

Details.propTypes = {
id: PropTypes.string,
ariaControls: PropTypes.arrayOf(PropTypes.string),
title: PropTypes.oneOfType([PropTypes.element, PropTypes.string]).isRequired,
content: PropTypes.oneOfType([PropTypes.func, PropTypes.string, PropTypes.element, PropTypes.array]).isRequired,
extendClassNames: PropTypes.oneOfType([PropTypes.array, PropTypes.string]),
onClick: PropTypes.func,
asHtml: PropTypes.bool
};
3 changes: 3 additions & 0 deletions lib/static/components/measurement-context.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
import {createContext} from 'react';

export const MeasurementContext = createContext({measure: () => {}});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can't figure out how it works and fixes the problem

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intro: react-virtualized needs to know exact sizes of each element for correct positioning. To let react-virtualized know the exact dimensions of element, you should use measure() function that is provided by <CellMeasurer/>.

Problem (how it was before): when we expand section, it immediately renders expanded section. At this point react-virtualized knows nothing about expanded section, because measure() function wasn't called. On the next frame ResizeObserver detects a change and calls measure() that adjusts list items positioning. User sees intermediate frame and perceives it as glitch.

Solution (how it is now): we use useLayoutEffect hook inside elements that change size instead of relying solely on ResizeObserver. Now, when we expand section, the component re-renders, but before painting on the screen useLayoutEffect is called, where we call measure() to let react-virtualized adjust positioning before user sees anything.

Since there are many components that need to be able to call measure() before rendering (section, browser section, screenshot state, meta info, etc.) I introduced MeasurementContext just to avoid prop drilling. Architecture-wise it may be far from perfect (ideally components shouldn't be aware of this), but I think it's good enough before we switch to new UI.

Now, everything looks 100% correct and user never sees intermediate state. However there's definitely some room for optimisation in terms of performance. If you click on expand/collapse all under the devtools CPU x6 slower throttling, it takes around 5s to render. That's because measuring happens tens of times instead of just once before final render. I wasn't able to fix this quickly, and without throttling everything works fine, so decided to keep it as is.

1 change: 0 additions & 1 deletion lib/static/components/section/body/description.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ export default class Description extends Component {

render() {
return <Details
type='text'
title='Description'
content={this._renderDescription}
/>;
Expand Down
11 changes: 6 additions & 5 deletions lib/static/components/section/body/history/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import {isEmpty} from 'lodash';
import Details from '../../../details';

import './index.styl';
import {List} from '@gravity-ui/uikit';
import {Card, List} from '@gravity-ui/uikit';

const History = ({history}) => {
const renderHistoryItem = (item) => {
Expand All @@ -21,12 +21,13 @@ const History = ({history}) => {
isEmpty(history)
? null
: <Details
type='text'
title='History'
content={
<div style={{display: `flex`}}>
<List items={history} renderItem={renderHistoryItem} filterable={false} virtualized={false}/>
</div>
<Card className='details__card' view='filled'>
<div style={{display: `flex`}}>
<List items={history} renderItem={renderHistoryItem} filterable={false} virtualized={false}/>
</div>
</Card>
}
extendClassNames='history'
/>
Expand Down
95 changes: 51 additions & 44 deletions lib/static/components/section/body/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React, {Component, Fragment} from 'react';
import React, {Fragment, useContext, useRef} from 'react';
import {connect} from 'react-redux';
import {bindActionCreators} from 'redux';
import {isNumber} from 'lodash';
Expand All @@ -10,38 +10,28 @@ import * as actions from '../../../modules/actions';
import ExtensionPoint from '../../extension-point';
import {RESULT} from '../../../../constants/extension-points';
import {ArrowRotateLeft} from '@gravity-ui/icons';
import useResizeObserver from '@react-hook/resize-observer';
import {MeasurementContext} from '../../measurement-context';

class Body extends Component {
static propTypes = {
browserId: PropTypes.string.isRequired,
browserName: PropTypes.string.isRequired,
testName: PropTypes.string.isRequired,
resultIds: PropTypes.array.isRequired,
// from store
gui: PropTypes.bool.isRequired,
running: PropTypes.bool.isRequired,
retryIndex: PropTypes.number,
actions: PropTypes.object.isRequired
};

onRetrySwitcherChange = (index) => {
const {browserId, retryIndex} = this.props;
function Body(props) {
const onRetrySwitcherChange = (index) => {
const {browserId, retryIndex} = props;

if (index === retryIndex) {
return;
}

this.props.actions.changeTestRetry({browserId, retryIndex: index});
props.actions.changeTestRetry({browserId, retryIndex: index});
};

onTestRetry = () => {
const {testName, browserName} = this.props;
const onTestRetry = () => {
const {testName, browserName} = props;

this.props.actions.retryTest({testName, browserName});
props.actions.retryTest({testName, browserName});
};

_addRetrySwitcher = () => {
const {resultIds, retryIndex} = this.props;
const addRetrySwitcher = () => {
const {resultIds, retryIndex} = props;

if (resultIds.length <= 1) {
return;
Expand All @@ -52,14 +42,14 @@ class Body extends Component {
<RetrySwitcher
resultIds={resultIds}
retryIndex={retryIndex}
onChange={this.onRetrySwitcherChange}
onChange={onRetrySwitcherChange}
/>
</div>
);
};

_addRetryButton = () => {
const {gui, running} = this.props;
const addRetryButton = () => {
const {gui, running} = props;

return gui
? (
Expand All @@ -71,38 +61,55 @@ class Body extends Component {
</Fragment>}
isSuiteControl={true}
isDisabled={running}
handler={this.onTestRetry}
handler={onTestRetry}
dataTestId="test-retry"
/>
</div>
)
: null;
};

_getActiveResultId = () => {
return this.props.resultIds[this.props.retryIndex];
const getActiveResultId = () => {
return props.resultIds[props.retryIndex];
};

render() {
const {testName} = this.props;
const activeResultId = this._getActiveResultId();

return (
<div className="section__body">
<div className="image-box">
<div className="controls">
{this._addRetrySwitcher()}
{this._addRetryButton()}
</div>
<ExtensionPoint name={RESULT} resultId={activeResultId} testName={testName}>
<Result resultId={activeResultId} testName={testName} />
</ExtensionPoint>
const {testName} = props;
const activeResultId = getActiveResultId();

const {measure} = useContext(MeasurementContext);
const resizeObserverRef = useRef(null);
useResizeObserver(resizeObserverRef, () => {
measure();
});

return (
<div className="section__body" ref={resizeObserverRef}>
<div className="image-box">
<div className="controls">
{addRetrySwitcher()}
{addRetryButton()}
</div>
<ExtensionPoint name={RESULT} resultId={activeResultId} testName={testName}>
<Result resultId={activeResultId} testName={testName} />
</ExtensionPoint>
</div>
);
}
</div>
);
}

Body.propTypes = {
browserId: PropTypes.string.isRequired,
browserName: PropTypes.string.isRequired,
testName: PropTypes.string.isRequired,
resultIds: PropTypes.array.isRequired,
onResize: PropTypes.func,
// from store
gui: PropTypes.bool.isRequired,
running: PropTypes.bool.isRequired,
retryIndex: PropTypes.number,
actions: PropTypes.object.isRequired
};

export default connect(
({gui, running, view: {retryIndex: viewRetryIndex}, tree}, {browserId}) => {
const {retryIndex: browserRetryIndex, lastMatchedRetryIndex} = tree.browsers.stateById[browserId] || {};
Expand Down
Loading
Loading