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: Chat - Link in end of line displays tooltip over text and not on link #27817

Closed
wants to merge 5 commits into from
Closed

Fix: Chat - Link in end of line displays tooltip over text and not on link #27817

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c8257c9
Fix: Chat - Link in end of line displays tooltip over text and not on…
ewanmellor Sep 19, 2023
36ef9c8
Fix: Chat - Link in end of line displays tooltip over text and not on…
ewanmellor Sep 21, 2023
f21c41c
Fix: Chat - Link in end of line displays tooltip over text and not on…
ewanmellor Sep 21, 2023
b2000cd
Fix: Chat - Link in end of line displays tooltip over text and not on…
ewanmellor Sep 21, 2023
3dc42e9
Fix: Chat - Link in end of line displays tooltip over text and not on…
ewanmellor Sep 21, 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
5 changes: 4 additions & 1 deletion src/components/AnchorForCommentsOnly/BaseAnchorForCommentsOnly.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,10 @@ function BaseAnchorForCommentsOnly({onPressIn, onPressOut, href = '', rel = '',
accessibilityRole={CONST.ACCESSIBILITY_ROLE.LINK}
accessibilityLabel={href}
>
<Tooltip text={href}>
<Tooltip
text={href}
shouldUseMultilinePositioning
>
<Text
ref={(el) => (linkRef = el)}
style={StyleSheet.flatten([style, defaultTextStyle])}
Expand Down
20 changes: 12 additions & 8 deletions src/components/Hoverable/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,9 @@ class Hoverable extends Component {
* Sets the hover state of this component to true and execute the onHoverIn callback.
*
* @param {Boolean} isHovered - Whether or not this component is hovered.
* @param {Event} ev - The event that triggered this state change.
*/
setIsHovered(isHovered) {
setIsHovered(isHovered, ev) {
if (this.props.disabled) {
return;
}
Expand All @@ -94,7 +95,7 @@ class Hoverable extends Component {
if (this.isScrollingRef && this.props.shouldHandleScroll && !this.state.isHovered) return;

if (isHovered !== this.state.isHovered) {
this.setState({isHovered}, isHovered ? this.props.onHoverIn : this.props.onHoverOut);
this.setState({isHovered}, () => (isHovered ? this.props.onHoverIn : this.props.onHoverOut)(ev));
Copy link
Contributor

Choose a reason for hiding this comment

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

Given that onHoverIn may be called without events it's better to use onMouseEnter. Add onMouseEnter and onMouseLeave props to Hoverable and use onMouseEnter to set the target and mouse position.

In Tooltip:

    const target = useRef(null);
    const initialMousePosition = useRef({x: 0, y: 0});

    const updateTargetAndMousePosition = useCallback((e) => {
        target.current = e.target;
        initialMousePosition.current = {x: e.clientX, y: e.clientY};
    }, []);

    <Hoverable onMouseEnter={updateTargetAndMousePosition) ... />

}
}

Expand All @@ -113,15 +114,18 @@ class Hoverable extends Component {
return;
}

this.setIsHovered(false);
this.setIsHovered(false, e);
}

handleVisibilityChange() {
/**
* @param {Event} ev - The visibility-change event object.
*/
handleVisibilityChange(ev) {
if (document.visibilityState !== 'hidden') {
return;
}

this.setIsHovered(false);
this.setIsHovered(false, ev);
}

render() {
Expand Down Expand Up @@ -154,14 +158,14 @@ class Hoverable extends Component {
}
},
onMouseEnter: (el) => {
this.setIsHovered(true);
this.setIsHovered(true, el);

if (_.isFunction(child.props.onMouseEnter)) {
child.props.onMouseEnter(el);
}
},
onMouseLeave: (el) => {
this.setIsHovered(false);
this.setIsHovered(false, el);

if (_.isFunction(child.props.onMouseLeave)) {
child.props.onMouseLeave(el);
Expand All @@ -171,7 +175,7 @@ class Hoverable extends Component {
// Check if the blur event occurred due to clicking outside the element
// and the wrapperView contains the element that caused the blur and reset isHovered
if (!this.wrapperView.contains(el.target) && !this.wrapperView.contains(el.relatedTarget)) {
this.setIsHovered(false);
this.setIsHovered(false, el);
}

if (_.isFunction(child.props.onBlur)) {
Expand Down
156 changes: 105 additions & 51 deletions src/components/Tooltip/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,49 @@ import useWindowDimensions from '../../hooks/useWindowDimensions';

const hasHoverSupport = DeviceCapabilities.hasHoverSupport();

/**
* Choose the correct bounding box for the tooltip to be positioned against.
* This handles the case where the target is wrapped across two lines, and
* so we need to find the correct part (the one that the user is hovering
* over) and show the tooltip there.
*
* This is only used when shouldUseMultilinePositioning == true.
*
* @param {Element} target The DOM element being hovered over.
* @param {number} clientX The X position from the MouseEvent.
* @param {number} clientY The Y position from the MouseEvent.
* @param {number} slop An allowed slop factor when searching for the bounding
* box. If the user is moving the mouse quickly we can end up getting a
* hover event with the position outside any of our bounding boxes. We retry
* with a small slop factor in that case, so if we have a bounding box close
* enough then we go with that.
Comment on lines +23 to +27
Copy link
Contributor

Choose a reason for hiding this comment

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

I have been checking and I think the need for the tolerance is not related to moving the mouse quickly, I can reproduce the bug constantly and this seems due to the returned bbs values corresponding to the text fragments only and not the block. Let's have the tolerance always used and integrate this function inside chooseBoundingBox

* @return {DOMRect} The chosen bounding box.
*/
function chooseBoundingBox(target, clientX, clientY, slop = 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we'd need a safer algo without the recursive call.

const bbs = target.getClientRects();
Copy link
Contributor

Choose a reason for hiding this comment

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

minor optimisation, if bbs contains only one element then just use it (that element would be same as getBoundingClientRect)

for (let i = 0; i < bbs.length; i++) {
const bb = bbs[i];
if (bb.x - slop <= clientX && bb.x + bb.width + slop >= clientX && bb.y - slop <= clientY && bb.y + bb.height + slop >= clientY) {
return bb;
}
}
if (slop === 0) {
// Retry with a slop factor, in case the user is moving the mouse quickly.
return chooseBoundingBox(target, clientX, clientY, 5);
}
// Fall back to the full bounding box if we failed to find a matching one
// (shouldn't happen).
return target.getBoundingClientRect();
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't write code that can't be reached.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return target.getBoundingClientRect();
return bbs[0];

}

/**
* A component used to wrap an element intended for displaying a tooltip. The term "tooltip's target" refers to the
* wrapped element, which, upon hover, triggers the tooltip to be shown.
* @param {propTypes} props
* @returns {ReactNodeLike}
*/
function Tooltip(props) {
const {children, numberOfLines, maxWidth, text, renderTooltipContent, renderTooltipContentKey} = props;
const {children, numberOfLines, maxWidth, text, renderTooltipContent, renderTooltipContentKey, shouldUseMultilinePositioning} = props;

const {preferredLocale} = useLocalize();
const {windowWidth} = useWindowDimensions();
Expand All @@ -44,33 +79,59 @@ function Tooltip(props) {
const prevText = usePrevious(text);

/**
* Display the tooltip in an animation.
* Update the tooltip bounding rectangle.
*
* @param {Object} bounds - updated bounds
*/
const showTooltip = useCallback(() => {
if (!isRendered) {
setIsRendered(true);
const updateBounds = useCallback((bounds) => {
if (bounds.width === 0) {
setIsRendered(false);
}
setWrapperWidth(bounds.width);
setWrapperHeight(bounds.height);
setXOffset(bounds.x);
setYOffset(bounds.y);
}, []);

setIsVisible(true);

animation.current.stopAnimation();

// When TooltipSense is active, immediately show the tooltip
if (TooltipSense.isActive()) {
animation.current.setValue(1);
} else {
isTooltipSenseInitiator.current = true;
Animated.timing(animation.current, {
toValue: 1,
duration: 140,
delay: 500,
useNativeDriver: false,
}).start(({finished}) => {
isAnimationCanceled.current = !finished;
});
}
TooltipSense.activate();
}, [isRendered]);
/**
* Display the tooltip in an animation.
*/
const showTooltip = useCallback(
(ev) => {
if (shouldUseMultilinePositioning) {
if (ev) {
const {clientX, clientY, target} = ev;
const bb = chooseBoundingBox(target, clientX, clientY);
updateBounds(bb);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Undo this. chooseBoundingBox should be called from updateBounds

    const updateBounds = (bounds) => {
        if (bounds.width === 0) {
            setIsRendered(false);
        }
        const betterBounds = chooseBoundingBox(target.current, initialMousePosition.current.x, initialMousePosition.current.y);
        setWrapperWidth(betterBounds.width);
        setWrapperHeight(betterBounds.height);
        setXOffset(betterBounds.x);
        setYOffset(betterBounds.y);
    };

Copy link
Author

Choose a reason for hiding this comment

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

@s77rt This doesn't make sense. If the user hovers over one part of the link and then moves to the other part, you need to move the tooltip. updateBounds is only called when the element moves, but in that case the element itself has not moved, so we will miss the necessary update. That's why it needs to be in the hover event handler.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if this is a valid case. if the user moves to the other part then we'd have two possible outcomes:

  1. User is still hovering the target, in this case the tooltip does not need to be updated as this case is only possible if the mouse stays at the same row (x position)
  2. User lost hover, in this case onMouseEnter will be called once the user hover the next part and the tooltip will be placed correctly

Copy link
Author

Choose a reason for hiding this comment

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

@s77rt Right, but your proposed updateTargetAndMousePosition doesn't set the tooltip position, it just saves the mouse position at that time. So when the onMouseEnter is called, you remember the location but don't do anything with that. And you won't get a call to updateBounds because the element hasn't moved.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that's the intended use. updateBounds will be called once the tooltip is visible or on bounds change

Copy link
Author

Choose a reason for hiding this comment

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

@s77rt "updateBounds will be called by BoundsObserver". This only happens if the element moves. If the user is moving the mouse between two parts of the same link, the element does not move and updateBounds is not called.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's correct. But the user can't move the mouse between two parts without firing the onMouseLeave and then onMouseEnter again on the second part

Copy link
Author

Choose a reason for hiding this comment

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

@s77rt Yes, but again, if the user does onMouseLeave and onMouseEnter, then you have nothing calling updateBounds. Your proposed updateTargetAndMousePosition does not call updateBounds. So the tooltip does not move. What am I missing from what you are saying?

Copy link
Contributor

Choose a reason for hiding this comment

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

  • onMouseLeave -> onHoverOut -> hideTooltip -> setIsVisible(false)
  • onMouseEnter (1st callback) -> updateTargetAndMousePosition
  • onMouseEnter (2nd callback) -> onHoverIn -> showTooltip -> setIsVisible(true)

Besides setting the target and mouse position, we have a change in the isVisible state which is passed to the enabled prop of BoundsObserver and per the docs https://socket.dev/npm/package/@react-ng/bounds-observer

Additionally, this callback will be invoked on enabling, i.e. when the enabled property changes to true

Copy link
Author

Choose a reason for hiding this comment

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

@s77rt OK, I finally see what you are saying. I have updated this PR with all the changes mentioned.


if (!isRendered) {
setIsRendered(true);
}

setIsVisible(true);

animation.current.stopAnimation();

// When TooltipSense is active, immediately show the tooltip
if (TooltipSense.isActive()) {
animation.current.setValue(1);
} else {
isTooltipSenseInitiator.current = true;
Animated.timing(animation.current, {
toValue: 1,
duration: 140,
delay: 500,
useNativeDriver: false,
}).start(({finished}) => {
isAnimationCanceled.current = !finished;
});
}
TooltipSense.activate();
},
[isRendered, shouldUseMultilinePositioning, updateBounds],
);

// eslint-disable-next-line rulesdir/prefer-early-return
useEffect(() => {
Expand All @@ -82,21 +143,6 @@ function Tooltip(props) {
}
}, [isVisible, text, prevText, showTooltip]);

/**
* Update the tooltip bounding rectangle
*
* @param {Object} bounds - updated bounds
*/
const updateBounds = (bounds) => {
if (bounds.width === 0) {
setIsRendered(false);
}
setWrapperWidth(bounds.width);
setWrapperHeight(bounds.height);
setXOffset(bounds.x);
setYOffset(bounds.y);
};

/**
* Hide the tooltip in an animation.
*/
Expand Down Expand Up @@ -126,6 +172,16 @@ function Tooltip(props) {
return children;
}

const hoverableChildren = (
<Hoverable
onHoverIn={showTooltip}
onHoverOut={hideTooltip}
shouldHandleScroll={props.shouldHandleScroll}
>
{children}
</Hoverable>
);

return (
<>
{isRendered && (
Expand All @@ -147,18 +203,16 @@ function Tooltip(props) {
key={[text, ...renderTooltipContentKey, preferredLocale]}
/>
)}
<BoundsObserver
enabled={isVisible}
onBoundsChange={updateBounds}
>
<Hoverable
onHoverIn={showTooltip}
onHoverOut={hideTooltip}
shouldHandleScroll={props.shouldHandleScroll}
{shouldUseMultilinePositioning ? (
hoverableChildren
) : (
<BoundsObserver
enabled={isVisible}
onBoundsChange={updateBounds}
>
{children}
</Hoverable>
</BoundsObserver>
{hoverableChildren}
</BoundsObserver>
)}
</>
);
}
Expand Down
18 changes: 18 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 @@ -31,6 +31,23 @@ const propTypes = {

/** passes this down to Hoverable component to decide whether to handle the scroll behaviour to show hover once the scroll ends */
shouldHandleScroll: PropTypes.bool,

/**
* Whether to use a different algorithm for positioning the tooltip.
*
* If true, when the user hovers over an element, we check whether that
* has multiple bounding boxes (i.e. it is text that has wrapped onto two
* lines). If it does, we select the correct bounding box to use for the
* tooltip.
*
* If false, the tooltip is positioned relative to the center of the
* target element. This is more performant, because it does not need to
* do any extra work inside the onmouseenter handler.
*
* Defaults to false. Set this to true if your tooltip target could wrap
* onto multiple lines.
*/
shouldUseMultilinePositioning: PropTypes.bool,
};

const defaultProps = {
Expand All @@ -42,6 +59,7 @@ const defaultProps = {
renderTooltipContent: undefined,
renderTooltipContentKey: [],
shouldHandleScroll: false,
shouldUseMultilinePositioning: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's remove this prop. I think we want to have this feature enabled for all tooltips

};

export {propTypes, defaultProps};
Loading