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

Jump to bottom

Migrate TooltipRenderedOnPageBody and fix tooltip flicker/moving when the content changes #18189

Merged
merged 7 commits into from
May 8, 2023
Merged

Migrate TooltipRenderedOnPageBody and fix tooltip flicker/moving when the content changes #18189

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a6f3833
add tooltip text as the key
bernhardoj Apr 29, 2023
7628b19
add renderTooltipContentKey as additional key of tooltip
bernhardoj Apr 29, 2023
a467937
migrate to functional component and fix flicker when content changes
bernhardoj Apr 29, 2023
be791fd
Merge branch 'main' into fix/17555
bernhardoj May 8, 2023
9ba08a4
update comment
bernhardoj May 8, 2023
8cdba0e
update proptypes to be more specific
bernhardoj May 8, 2023
ec87028
add comment
bernhardoj May 8, 2023
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
1 change: 1 addition & 0 deletions src/components/Reactions/ReportActionItemReactions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ const ReportActionItemReactions = (props) => {
accountIDs={reactionUsers}
/>
)}
renderTooltipContentKey={[...reactionUsers, ...emojiCodes]}
key={reaction.emoji}
>
<EmojiReactionBubble
Expand Down
207 changes: 82 additions & 125 deletions src/components/Tooltip/TooltipRenderedOnPageBody.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
import React from 'react';
import React, {
useLayoutEffect, useEffect, useState, useRef, useMemo,
} from 'react';
import PropTypes from 'prop-types';
import {Animated, View} from 'react-native';
import ReactDOM from 'react-dom';
Expand Down Expand Up @@ -58,137 +60,92 @@ const defaultProps = {
// On every tooltip hover, we update the position in state which will result in re-rendering.
// We also update the state on layout changes which will be triggered often.
// There will be n number of tooltip components in the page.
// It's good to memorize this one.
class TooltipRenderedOnPageBody extends React.PureComponent {
constructor(props) {
super(props);
this.state = {
// The width of tooltip's inner content. Has to be undefined in the beginning
// as a width of 0 will cause the content to be rendered of a width of 0,
// which prevents us from measuring it correctly.
tooltipContentWidth: undefined,

// The width and height of the tooltip itself
tooltipWidth: 0,
tooltipHeight: 0,
};

if (props.renderTooltipContent && props.text) {
Log.warn('Developer error: Cannot use both text and renderTooltipContent props at the same time in <TooltipRenderedOnPageBody />!');
}

this.measureTooltip = this.measureTooltip.bind(this);
this.updateTooltipContentWidth = this.updateTooltipContentWidth.bind(this);
}

componentDidMount() {
this.updateTooltipContentWidth();
}

componentDidUpdate(prevProps) {
// We need to re-calculate the tooltipContentWidth if it is greater than maxWidth.
// So that the wrapperWidth still be updated again with correct value
if (this.state.tooltipContentWidth > prevProps.maxWidth) {
this.updateTooltipContentWidth();
}

if (prevProps.text === this.props.text && prevProps.renderTooltipContent === this.props.renderTooltipContent) {
// It's good to memoize this one.
const TooltipRenderedOnPageBody = (props) => {
// The width of tooltip's inner content. Has to be undefined in the beginning
// as a width of 0 will cause the content to be rendered of a width of 0,
// which prevents us from measuring it correctly.
const [tooltipContentWidth, setTooltipContentWidth] = useState(undefined);
const [tooltipWidth, setTooltipWidth] = useState(0);
const [tooltipHeight, setTooltipHeight] = useState(0);
const contentRef = useRef();
const wrapper = useRef();

useEffect(() => {
if (!props.renderTooltipContent || !props.text) {
return;
}

// Reset the tooltip text width to 0 so that we can measure it again.
// eslint-disable-next-line react/no-did-update-set-state
this.setState({tooltipContentWidth: undefined}, this.updateTooltipContentWidth);
}

updateTooltipContentWidth() {
if (!this.contentRef) {
return;
}

this.setState({
tooltipContentWidth: this.contentRef.offsetWidth,
});
}

/**
* Measure the size of the tooltip itself.
*
* @param {Object} nativeEvent
*/
measureTooltip({nativeEvent}) {
this.setState({
tooltipWidth: nativeEvent.layout.width,
tooltipHeight: nativeEvent.layout.height,
});
}

render() {
const {
animationStyle,
tooltipWrapperStyle,
tooltipTextStyle,
pointerWrapperStyle,
pointerStyle,
} = getTooltipStyles(
this.props.animation,
this.props.windowWidth,
this.props.xOffset,
this.props.yOffset,
this.props.wrapperWidth,
this.props.wrapperHeight,
this.props.maxWidth,
this.state.tooltipWidth,
this.state.tooltipHeight,
this.state.tooltipContentWidth,
this.props.shiftHorizontal,
this.props.shiftVertical,
Log.warn('Developer error: Cannot use both text and renderTooltipContent props at the same time in <TooltipRenderedOnPageBody />!');
}, [props.text, props.renderTooltipContent]);

useLayoutEffect(() => {
// Calculate the tooltip width and height before the browser repaints the screen to prevent flicker
// because of the late update of the width and the height from onLayout.
Copy link
Contributor

Choose a reason for hiding this comment

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

Coming from #48691, we had an issue where the tooltip displays as blank. This happens because the tooltip component gets frozen when navigating several screens away and then back and when this effect is called a second time, the size is calculated as 0 due to the animation effect scale(0). More details here.

const rect = wrapper.current.getBoundingClientRect();

setTooltipWidth(rect.width);
setTooltipHeight(rect.height);
setTooltipContentWidth(contentRef.current.offsetWidth);
}, []);

const {
animationStyle,
tooltipWrapperStyle,
tooltipTextStyle,
pointerWrapperStyle,
pointerStyle,
} = useMemo(
() => getTooltipStyles(
props.animation,
props.windowWidth,
props.xOffset,
props.yOffset,
props.wrapperWidth,
props.wrapperHeight,
props.maxWidth,
tooltipWidth,
tooltipHeight,
tooltipContentWidth,
props.shiftHorizontal,
props.shiftVertical,
),
[props.animation, props.windowWidth, props.xOffset, props.yOffset, props.wrapperWidth, props.wrapperHeight,
props.maxWidth, tooltipWidth, tooltipHeight, tooltipContentWidth, props.shiftHorizontal, props.shiftVertical],
);

let content;
if (props.renderTooltipContent) {
content = (
<View ref={contentRef}>
{props.renderTooltipContent()}
</View>
);

const contentRef = (ref) => {
// Once the content for the tooltip first renders, update the width of the tooltip dynamically to fit the width of the content.
// Note that we can't have this code in componentDidMount because the ref for the content won't be set until after the first render
if (this.contentRef) {
return;
}

this.contentRef = ref;
this.updateTooltipContentWidth();
};

let content;
if (this.props.renderTooltipContent) {
content = (
<View ref={contentRef}>
{this.props.renderTooltipContent()}
</View>
);
} else {
content = (
<Text numberOfLines={this.props.numberOfLines} style={tooltipTextStyle}>
<Text style={tooltipTextStyle} ref={contentRef}>
{this.props.text}
</Text>
} else {
content = (
<Text numberOfLines={props.numberOfLines} style={tooltipTextStyle}>
<Text style={tooltipTextStyle} ref={contentRef}>
{props.text}
</Text>
);
}

return ReactDOM.createPortal(
<Animated.View
onLayout={this.measureTooltip}
style={[tooltipWrapperStyle, animationStyle]}
>
{content}
<View style={pointerWrapperStyle}>
<View style={pointerStyle} />
</View>
</Animated.View>,
document.querySelector('body'),
</Text>
);
}
}

return ReactDOM.createPortal(
<Animated.View
ref={wrapper}
style={[tooltipWrapperStyle, animationStyle]}
>
{content}
<View style={pointerWrapperStyle}>
<View style={pointerStyle} />
</View>
</Animated.View>,
document.querySelector('body'),
);
};

TooltipRenderedOnPageBody.propTypes = propTypes;
TooltipRenderedOnPageBody.defaultProps = defaultProps;
TooltipRenderedOnPageBody.displayName = 'TooltipRenderedOnPageBody';

export default TooltipRenderedOnPageBody;
export default React.memo(TooltipRenderedOnPageBody);
4 changes: 4 additions & 0 deletions src/components/Tooltip/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,10 @@ class Tooltip extends PureComponent {
maxWidth={this.props.maxWidth}
numberOfLines={this.props.numberOfLines}
renderTooltipContent={this.props.renderTooltipContent}

// We pass a key, so whenever the content changes this component will completely remount with a fresh state.
// This prevents flickering/moving while remaining performant.
key={[this.props.text, ...this.props.renderTooltipContentKey]}
Copy link
Contributor

Choose a reason for hiding this comment

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

This line caused a regression in #19693 , Using a key will trigger the view to re-render when the text changes. However, if the text changes before the animation finishes, it will cancel the animation, and the tooltip will disappear. More context

/>
)}
<Hoverable
Expand Down
4 changes: 4 additions & 0 deletions src/components/Tooltip/tooltipPropTypes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,9 @@ const propTypes = {

/** Render custom content inside the tooltip. Note: This cannot be used together with the text props. */
renderTooltipContent: PropTypes.func,

/** Unique key of renderTooltipContent to rerender the tooltip when one of the key changes */
renderTooltipContentKey: PropTypes.arrayOf(PropTypes.string),
};

const defaultProps = {
Expand All @@ -49,6 +52,7 @@ const defaultProps = {
maxWidth: variables.sideBarWidth,
numberOfLines: CONST.TOOLTIP_MAX_LINES,
renderTooltipContent: undefined,
renderTooltipContentKey: [],
focusable: true,
};

Expand Down
85 changes: 43 additions & 42 deletions src/styles/getTooltipStyles.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,21 +134,54 @@ export default function getTooltipStyles(
// We get wrapper width based on the tooltip's inner text width so the wrapper is just big enough to fit text and prevent white space.
// If the text width is less than the maximum available width, add horizontal padding.
// Note: tooltipContentWidth ignores the fractions (OffsetWidth) so add 1px to fit the text properly.
const wrapperWidth = tooltipContentWidth && (tooltipContentWidth < maxWidth
? tooltipContentWidth + (spacing.ph2.paddingHorizontal * 2) + 1
: maxWidth);
const wrapperWidth = tooltipContentWidth && (tooltipContentWidth + (spacing.ph2.paddingHorizontal * 2) + 1);

// Hide the tooltip entirely if it's position hasn't finished measuring yet. This prevents UI jank where the tooltip flashes in the top left corner of the screen.
const opacity = (xOffset === 0 && yOffset === 0) ? 0 : 1;

const isTooltipSizeReady = tooltipWidth !== 0 && tooltipHeight !== 0;
const scale = !isTooltipSizeReady ? 1 : currentSize;
let wrapperTop = 0;
let wrapperLeft = 0;

if (isTooltipSizeReady) {
// Because it uses fixed positioning, the top-left corner of the tooltip is aligned
// with the top-left corner of the window by default.
// we will use yOffset to position the tooltip relative to the Wrapped Component
// So we need to shift the tooltip vertically and horizontally to position it correctly.
//
// First, we'll position it vertically.
// To shift the tooltip down, we'll give `top` a positive value.
// To shift the tooltip up, we'll give `top` a negative value.
wrapperTop = shouldShowBelow

// We need to shift the tooltip down below the component. So shift the tooltip down (+) by...
? (yOffset + componentHeight + POINTER_HEIGHT + manualShiftVertical)

// We need to shift the tooltip up above the component. So shift the tooltip up (-) by...
: ((yOffset - (tooltipHeight + POINTER_HEIGHT)) + manualShiftVertical);

// Next, we'll position it horizontally.
// we will use xOffset to position the tooltip relative to the Wrapped Component
// To shift the tooltip right, we'll give `left` a positive value.
// To shift the tooltip left, we'll give `left` a negative value.
//
// So we'll:
// 1) Shift the tooltip right (+) to the center of the component,
// so the left edge lines up with the component center.
// 2) Shift it left (-) to by half the tooltip's width,
// so the tooltip's center lines up with the center of the wrapped component.
// 3) Add the horizontal shift (left or right) computed above to keep it out of the gutters.
// 4) Lastly, add the manual horizontal shift passed in as a parameter.
wrapperLeft = xOffset + ((componentWidth / 2) - (tooltipWidth / 2)) + horizontalShift + manualShiftHorizontal;
}

return {
animationStyle: {
// remember Transform causes a new Local cordinate system
// https://drafts.csswg.org/css-transforms-1/#transform-rendering
// so Position fixed children will be relative to this new Local cordinate system
transform: [{
scale: currentSize,
}],
transform: [{scale}],
},
tooltipWrapperStyle: {
position: 'fixed',
Expand All @@ -158,52 +191,20 @@ export default function getTooltipStyles(
...spacing.ph2,
zIndex: variables.tooltipzIndex,
width: wrapperWidth,
maxWidth,
top: wrapperTop,
left: wrapperLeft,
opacity,

// We are adding this to prevent the tooltip text from being selected and copied on CTRL + A.
...styles.userSelectNone,

// Because it uses fixed positioning, the top-left corner of the tooltip is aligned
// with the top-left corner of the window by default.
// we will use yOffset to position the tooltip relative to the Wrapped Component
// So we need to shift the tooltip vertically and horizontally to position it correctly.
//
// First, we'll position it vertically.
// To shift the tooltip down, we'll give `top` a positive value.
// To shift the tooltip up, we'll give `top` a negative value.
top: shouldShowBelow

// We need to shift the tooltip down below the component. So shift the tooltip down (+) by...
? (yOffset + componentHeight + POINTER_HEIGHT + manualShiftVertical)

// We need to shift the tooltip up above the component. So shift the tooltip up (-) by...
: ((yOffset - (tooltipHeight + POINTER_HEIGHT)) + manualShiftVertical),

// Next, we'll position it horizontally.
// we will use xOffset to position the tooltip relative to the Wrapped Component
// To shift the tooltip right, we'll give `left` a positive value.
// To shift the tooltip left, we'll give `left` a negative value.
//
// So we'll:
// 1) Shift the tooltip right (+) to the center of the component,
// so the left edge lines up with the component center.
// 2) Shift it left (-) to by half the tooltip's width,
// so the tooltip's center lines up with the center of the wrapped component.
// 3) Add the horizontal shift (left or right) computed above to keep it out of the gutters.
// 4) Lastly, add the manual horizontal shift passed in as a parameter.
left: xOffset + ((componentWidth / 2) - (tooltipWidth / 2)) + horizontalShift + manualShiftHorizontal,

opacity,
},
tooltipTextStyle: {
color: themeColors.textReversed,
fontFamily: fontFamily.EXP_NEUE,
fontSize: tooltipFontSize,
overflow: 'hidden',
lineHeight: variables.lineHeightSmall,

// To measure tooltip text width correctly we render it freely i.e. text should not wrap to parent's boundaries.
// More info: https://github.com/Expensify/App/issues/15949#issuecomment-1483011998
...(tooltipContentWidth ? {} : styles.pre),
},
pointerWrapperStyle: {
position: 'fixed',
Expand Down
Loading