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.

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 #29134

Merged
merged 8 commits into from
Oct 12, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 6 additions & 0 deletions src/components/Hoverable/hoverablePropTypes.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,12 @@ const propTypes = {
/** Function that executes when the mouse leaves the children. */
onHoverOut: PropTypes.func,

/** Direct pass-through of React's onMouseEnter event. */
onMouseEnter: PropTypes.func,

/** Direct pass-through of React's onMouseLeave event. */
onMouseLeave: PropTypes.func,

/** Decides whether to handle the scroll behaviour to show hover once the scroll ends */
shouldHandleScroll: PropTypes.bool,
};
Expand Down
8 changes: 8 additions & 0 deletions src/components/Hoverable/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -155,13 +155,21 @@ class Hoverable extends Component {
}
},
onMouseEnter: (el) => {
if (_.isFunction(this.props.onMouseEnter)) {
this.props.onMouseEnter(el);
}

this.setIsHovered(true);

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

this.setIsHovered(false);

if (_.isFunction(child.props.onMouseLeave)) {
Expand Down
67 changes: 58 additions & 9 deletions src/components/Tooltip/BaseTooltip.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import _ from 'underscore';
import React, {memo, useCallback, useEffect, useRef, useState} from 'react';
import {Animated} from 'react-native';
import {BoundsObserver} from '@react-ng/bounds-observer';
import Str from 'expensify-common/lib/str';
import TooltipRenderedOnPageBody from './TooltipRenderedOnPageBody';
import Hoverable from '../Hoverable';
import * as tooltipPropTypes from './tooltipPropTypes';
Expand All @@ -19,9 +20,41 @@ const hasHoverSupport = DeviceCapabilities.hasHoverSupport();
* @param {propTypes} props
* @returns {ReactNodeLike}
*/
function Tooltip(props) {
const {children, numberOfLines, maxWidth, text, renderTooltipContent, renderTooltipContentKey} = props;

/**
* 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.
*
* @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.
* @return {DOMRect} The chosen bounding box.
*/

function chooseBoundingBox(target, clientX, clientY) {
const slop = 5;
const bbs = target.getClientRects();
const clientXMin = clientX - slop;
const clientXMax = clientX + slop;
const clientYMin = clientY - slop;
const clientYMax = clientY + slop;

for (let i = 0; i < bbs.length; i++) {
const bb = bbs[i];
if (clientXMin <= bb.right && clientXMax >= bb.left && clientYMin <= bb.bottom && clientYMax >= bb.top) {
return bb;
}
}

// If no matching bounding box is found, fall back to the first one.
// This could only happen if the user is moving the mouse very quickly
// and they got it outside our slop above.
return bbs[0];
}

function Tooltip({children, numberOfLines, maxWidth, text, renderTooltipContent, renderTooltipContentKey, shouldHandleScroll, shiftHorizontal, shiftVertical}) {
const {preferredLocale} = useLocalize();
const {windowWidth} = useWindowDimensions();

Expand All @@ -43,6 +76,14 @@ function Tooltip(props) {
const isAnimationCanceled = useRef(false);
const prevText = usePrevious(text);

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

const updateTargetAndMousePosition = useCallback((e) => {
target.current = e.target;
Copy link
Contributor

Choose a reason for hiding this comment

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

We should have used the currentTarget (the element that is listening to the mouse enter event) instead of target (the element that caused the event to fire and bubble up). Coming from #29678

initialMousePosition.current = {x: e.clientX, y: e.clientY};
}, []);

/**
* Display the tooltip in an animation.
*/
Expand Down Expand Up @@ -91,10 +132,15 @@ function Tooltip(props) {
if (bounds.width === 0) {
setIsRendered(false);
}
setWrapperWidth(bounds.width);
setWrapperHeight(bounds.height);
setXOffset(bounds.x);
setYOffset(bounds.y);
// Choose a bounding box for the tooltip to target.
// In the case when the target is a link that has wrapped onto
// multiple lines, we want to show the tooltip over the part
// of the link that the user is hovering over.
const betterBounds = chooseBoundingBox(target.current, initialMousePosition.current.x, initialMousePosition.current.y);
setWrapperWidth(betterBounds.width);
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this is leading to a crash with Uncaught TypeError: Cannot read properties of undefined (reading 'width'). This is what I did:

  1. Sign in with a new account
  2. Go to Settings > Workspaces > Create New Workspace (don't create anything)
  3. Click the back button on the modal
    image

image

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes it's, more info #29446 (comment) and PR fixing it #29468

setWrapperHeight(betterBounds.height);
setXOffset(betterBounds.x);
setYOffset(betterBounds.y);
};

/**
Expand Down Expand Up @@ -136,8 +182,8 @@ function Tooltip(props) {
yOffset={yOffset}
targetWidth={wrapperWidth}
targetHeight={wrapperHeight}
shiftHorizontal={_.result(props, 'shiftHorizontal')}
shiftVertical={_.result(props, 'shiftVertical')}
shiftHorizontal={Str.result(shiftHorizontal)}
shiftVertical={Str.result(shiftVertical)}
text={text}
maxWidth={maxWidth}
numberOfLines={numberOfLines}
Expand All @@ -152,9 +198,10 @@ function Tooltip(props) {
onBoundsChange={updateBounds}
>
<Hoverable
onMouseEnter={updateTargetAndMousePosition}
onHoverIn={showTooltip}
onHoverOut={hideTooltip}
shouldHandleScroll={props.shouldHandleScroll}
shouldHandleScroll={shouldHandleScroll}
>
{children}
</Hoverable>
Expand All @@ -165,4 +212,6 @@ function Tooltip(props) {

Tooltip.propTypes = tooltipPropTypes.propTypes;
Tooltip.defaultProps = tooltipPropTypes.defaultProps;
Tooltip.displayName = 'Tooltip';

export default memo(Tooltip);
Loading