From c8257c9c8856f75bf0b0645dc5a2572481d8317b Mon Sep 17 00:00:00 2001 From: Ewan Mellor Date: Tue, 19 Sep 2023 15:45:19 -0700 Subject: [PATCH 1/5] Fix: Chat - Link in end of line displays tooltip over text and not on link Add a new prop to Tooltip called shouldUseMultilinePositioning. This enables a new algorithm for finding the bounding box target for the tooltip. In the case where the target has wrapped onto multiple lines (e.g. it is a link in a chat window) then the link has multiple bounding boxes, and we want to show the tooltip against the one that the user is hovering over. As part of this, extend Hoverable to pass the Event to its onHoverIn / onHoverOut callbacks. Enable this new algorithm from BaseAnchorForCommentsOnly, which is the base class for links that show up in chat. --- .../BaseAnchorForCommentsOnly.js | 5 +- src/components/Hoverable/index.js | 20 ++- src/components/Tooltip/index.js | 156 ++++++++++++------ src/components/Tooltip/tooltipPropTypes.js | 18 ++ 4 files changed, 139 insertions(+), 60 deletions(-) diff --git a/src/components/AnchorForCommentsOnly/BaseAnchorForCommentsOnly.js b/src/components/AnchorForCommentsOnly/BaseAnchorForCommentsOnly.js index 9cfe9d893d8e..7ab8d9577b09 100644 --- a/src/components/AnchorForCommentsOnly/BaseAnchorForCommentsOnly.js +++ b/src/components/AnchorForCommentsOnly/BaseAnchorForCommentsOnly.js @@ -77,7 +77,10 @@ function BaseAnchorForCommentsOnly({onPressIn, onPressOut, href = '', rel = '', accessibilityRole={CONST.ACCESSIBILITY_ROLE.LINK} accessibilityLabel={href} > - + (linkRef = el)} style={StyleSheet.flatten([style, defaultTextStyle])} diff --git a/src/components/Hoverable/index.js b/src/components/Hoverable/index.js index 5da41f1388fb..29f45dfd38b4 100644 --- a/src/components/Hoverable/index.js +++ b/src/components/Hoverable/index.js @@ -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; } @@ -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)); } } @@ -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() { @@ -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); @@ -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)) { diff --git a/src/components/Tooltip/index.js b/src/components/Tooltip/index.js index f60982f52dd4..a6d2c922bdba 100644 --- a/src/components/Tooltip/index.js +++ b/src/components/Tooltip/index.js @@ -13,6 +13,41 @@ 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. + * @return {DOMRect} The chosen bounding box. + */ +function chooseBoundingBox(target, clientX, clientY, slop = 0) { + const bbs = target.getClientRects(); + 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(); +} + /** * 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. @@ -20,7 +55,7 @@ const hasHoverSupport = DeviceCapabilities.hasHoverSupport(); * @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(); @@ -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); + } + } + + 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(() => { @@ -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. */ @@ -126,6 +172,16 @@ function Tooltip(props) { return children; } + const hoverableChildren = ( + + {children} + + ); + return ( <> {isRendered && ( @@ -147,18 +203,16 @@ function Tooltip(props) { key={[text, ...renderTooltipContentKey, preferredLocale]} /> )} - - - {children} - - + {hoverableChildren} + + )} ); } diff --git a/src/components/Tooltip/tooltipPropTypes.js b/src/components/Tooltip/tooltipPropTypes.js index 2ddf8120d58c..1b9c84742ada 100644 --- a/src/components/Tooltip/tooltipPropTypes.js +++ b/src/components/Tooltip/tooltipPropTypes.js @@ -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 = { @@ -42,6 +59,7 @@ const defaultProps = { renderTooltipContent: undefined, renderTooltipContentKey: [], shouldHandleScroll: false, + shouldUseMultilinePositioning: false, }; export {propTypes, defaultProps}; From 36ef9c8bfd5ca7c4707c980e8df7de183362a788 Mon Sep 17 00:00:00 2001 From: Ewan Mellor Date: Wed, 20 Sep 2023 17:34:32 -0700 Subject: [PATCH 2/5] Fix: Chat - Link in end of line displays tooltip over text and not on link Changes from PR review: o Remove the shouldUseMultilinePositioning prop, and instead make the new algorithm the one to use in all situations. o Refactor chooseBoundingBox to avoid a recursive call. o Shortcut chooseBoundingBox in the case where the element only has one bounding box. o Revert the changes to Hoverable that pass the MouseEvent through onHoverIn / onHoverOut, and instead add direct pass-throughs for the onMouseEnter / onMouseLeave events. o Change the algorithm so that the onHoverIn event handler only records the mouse position, and rely on BoundsObserver being enabled as a side-effect of the isVisible change to move the tooltip. --- .../BaseAnchorForCommentsOnly.js | 5 +- .../Hoverable/hoverablePropTypes.js | 6 + src/components/Hoverable/index.js | 26 +-- src/components/Tooltip/index.js | 185 ++++++++++-------- src/components/Tooltip/tooltipPropTypes.js | 18 -- 5 files changed, 122 insertions(+), 118 deletions(-) diff --git a/src/components/AnchorForCommentsOnly/BaseAnchorForCommentsOnly.js b/src/components/AnchorForCommentsOnly/BaseAnchorForCommentsOnly.js index 7ab8d9577b09..9cfe9d893d8e 100644 --- a/src/components/AnchorForCommentsOnly/BaseAnchorForCommentsOnly.js +++ b/src/components/AnchorForCommentsOnly/BaseAnchorForCommentsOnly.js @@ -77,10 +77,7 @@ function BaseAnchorForCommentsOnly({onPressIn, onPressOut, href = '', rel = '', accessibilityRole={CONST.ACCESSIBILITY_ROLE.LINK} accessibilityLabel={href} > - + (linkRef = el)} style={StyleSheet.flatten([style, defaultTextStyle])} diff --git a/src/components/Hoverable/hoverablePropTypes.js b/src/components/Hoverable/hoverablePropTypes.js index d483a06d6aaf..a3aeaa597d7a 100644 --- a/src/components/Hoverable/hoverablePropTypes.js +++ b/src/components/Hoverable/hoverablePropTypes.js @@ -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, }; diff --git a/src/components/Hoverable/index.js b/src/components/Hoverable/index.js index 29f45dfd38b4..621f0efa5a1d 100644 --- a/src/components/Hoverable/index.js +++ b/src/components/Hoverable/index.js @@ -76,9 +76,8 @@ 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, ev) { + setIsHovered(isHovered) { if (this.props.disabled) { return; } @@ -95,7 +94,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)(ev)); + this.setState({isHovered}, isHovered ? this.props.onHoverIn : this.props.onHoverOut); } } @@ -114,18 +113,15 @@ class Hoverable extends Component { return; } - this.setIsHovered(false, e); + this.setIsHovered(false); } - /** - * @param {Event} ev - The visibility-change event object. - */ - handleVisibilityChange(ev) { + handleVisibilityChange() { if (document.visibilityState !== 'hidden') { return; } - this.setIsHovered(false, ev); + this.setIsHovered(false); } render() { @@ -158,24 +154,30 @@ class Hoverable extends Component { } }, onMouseEnter: (el) => { - this.setIsHovered(true, el); + this.setIsHovered(true); if (_.isFunction(child.props.onMouseEnter)) { child.props.onMouseEnter(el); } + if (_.isFunction(this.props.onMouseEnter)) { + this.props.onMouseEnter(el); + } }, onMouseLeave: (el) => { - this.setIsHovered(false, el); + this.setIsHovered(false); if (_.isFunction(child.props.onMouseLeave)) { child.props.onMouseLeave(el); } + if (_.isFunction(this.props.onMouseLeave)) { + this.props.onMouseLeave(el); + } }, onBlur: (el) => { // 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, el); + this.setIsHovered(false); } if (_.isFunction(child.props.onBlur)) { diff --git a/src/components/Tooltip/index.js b/src/components/Tooltip/index.js index a6d2c922bdba..1d9df55f063c 100644 --- a/src/components/Tooltip/index.js +++ b/src/components/Tooltip/index.js @@ -14,14 +14,10 @@ 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. + * Choose the correct bounding box from the given list. + * This is a helper function for chooseBoundingBox below. * - * This is only used when shouldUseMultilinePositioning == true. - * - * @param {Element} target The DOM element being hovered over. + * @param {Element} bbs The bounding boxes of 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 @@ -29,22 +25,48 @@ const hasHoverSupport = DeviceCapabilities.hasHoverSupport(); * 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. - * @return {DOMRect} The chosen bounding box. + * @return {DOMRect | null} The chosen bounding box. null if we failed to find + * a matching one, which can happen if the user is moving the mouse quickly + * and the onHoverOver event actually fires outside the element bounding box. */ -function chooseBoundingBox(target, clientX, clientY, slop = 0) { - const bbs = target.getClientRects(); +function chooseBoundingBoxWithSlop(bbs, clientX, clientY, slop) { 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); + return null; +} + +/** + * 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 bbs = target.getClientRects(); + if (bbs.length === 1) { + return bbs[0]; + } + let bb = chooseBoundingBoxWithSlop(bbs, clientX, clientY, 0); + if (bb) { + return bb; } - // Fall back to the full bounding box if we failed to find a matching one - // (shouldn't happen). + // Retry with a slop factor, in case the user is moving the mouse quickly. + bb = chooseBoundingBoxWithSlop(bbs, clientX, clientY, 5); + if (bb) { + return bb; + } + // Fall back to the full bounding box if we failed to find a matching one. + // This could only happen if the user is moving the mouse very quickly + // and they got it outside our slop above. return target.getBoundingClientRect(); } @@ -55,7 +77,7 @@ function chooseBoundingBox(target, clientX, clientY, slop = 0) { * @returns {ReactNodeLike} */ function Tooltip(props) { - const {children, numberOfLines, maxWidth, text, renderTooltipContent, renderTooltipContentKey, shouldUseMultilinePositioning} = props; + const {children, numberOfLines, maxWidth, text, renderTooltipContent, renderTooltipContentKey} = props; const {preferredLocale} = useLocalize(); const {windowWidth} = useWindowDimensions(); @@ -78,60 +100,42 @@ function Tooltip(props) { const isAnimationCanceled = useRef(false); const prevText = usePrevious(text); - /** - * Update the tooltip bounding rectangle. - * - * @param {Object} bounds - updated bounds - */ - const updateBounds = useCallback((bounds) => { - if (bounds.width === 0) { - setIsRendered(false); - } - setWrapperWidth(bounds.width); - setWrapperHeight(bounds.height); - setXOffset(bounds.x); - setYOffset(bounds.y); + 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}; }, []); /** * 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); - } - } - - 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], - ); + const showTooltip = useCallback(() => { + 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]); // eslint-disable-next-line rulesdir/prefer-early-return useEffect(() => { @@ -143,6 +147,26 @@ 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); + } + const t = target.current; + if (!t) { + return; + } + const betterBounds = chooseBoundingBox(t, initialMousePosition.current.x, initialMousePosition.current.y); + setWrapperWidth(betterBounds.width); + setWrapperHeight(betterBounds.height); + setXOffset(betterBounds.x); + setYOffset(betterBounds.y); + }; + /** * Hide the tooltip in an animation. */ @@ -172,16 +196,6 @@ function Tooltip(props) { return children; } - const hoverableChildren = ( - - {children} - - ); - return ( <> {isRendered && ( @@ -203,16 +217,19 @@ function Tooltip(props) { key={[text, ...renderTooltipContentKey, preferredLocale]} /> )} - {shouldUseMultilinePositioning ? ( - hoverableChildren - ) : ( - + - {hoverableChildren} - - )} + {children} + + ); } diff --git a/src/components/Tooltip/tooltipPropTypes.js b/src/components/Tooltip/tooltipPropTypes.js index 1b9c84742ada..2ddf8120d58c 100644 --- a/src/components/Tooltip/tooltipPropTypes.js +++ b/src/components/Tooltip/tooltipPropTypes.js @@ -31,23 +31,6 @@ 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 = { @@ -59,7 +42,6 @@ const defaultProps = { renderTooltipContent: undefined, renderTooltipContentKey: [], shouldHandleScroll: false, - shouldUseMultilinePositioning: false, }; export {propTypes, defaultProps}; From f21c41cc13e9ea2effb6c0bfca11e7911b829ae4 Mon Sep 17 00:00:00 2001 From: Ewan Mellor Date: Wed, 20 Sep 2023 18:46:42 -0700 Subject: [PATCH 3/5] Fix: Chat - Link in end of line displays tooltip over text and not on link Changes from PR review: o Reorder the callbacks from Hoverable. o Remove a null check. o Add a comment. --- src/components/Hoverable/index.js | 12 ++++++------ src/components/Tooltip/index.js | 7 ++++--- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/src/components/Hoverable/index.js b/src/components/Hoverable/index.js index 621f0efa5a1d..610734b7dc2e 100644 --- a/src/components/Hoverable/index.js +++ b/src/components/Hoverable/index.js @@ -156,22 +156,22 @@ class Hoverable extends Component { onMouseEnter: (el) => { this.setIsHovered(true); - if (_.isFunction(child.props.onMouseEnter)) { - child.props.onMouseEnter(el); - } if (_.isFunction(this.props.onMouseEnter)) { this.props.onMouseEnter(el); } + if (_.isFunction(child.props.onMouseEnter)) { + child.props.onMouseEnter(el); + } }, onMouseLeave: (el) => { this.setIsHovered(false); - if (_.isFunction(child.props.onMouseLeave)) { - child.props.onMouseLeave(el); - } if (_.isFunction(this.props.onMouseLeave)) { this.props.onMouseLeave(el); } + if (_.isFunction(child.props.onMouseLeave)) { + child.props.onMouseLeave(el); + } }, onBlur: (el) => { // Check if the blur event occurred due to clicking outside the element diff --git a/src/components/Tooltip/index.js b/src/components/Tooltip/index.js index 1d9df55f063c..ea8a5b1f87e3 100644 --- a/src/components/Tooltip/index.js +++ b/src/components/Tooltip/index.js @@ -157,9 +157,10 @@ function Tooltip(props) { setIsRendered(false); } const t = target.current; - if (!t) { - return; - } + // 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(t, initialMousePosition.current.x, initialMousePosition.current.y); setWrapperWidth(betterBounds.width); setWrapperHeight(betterBounds.height); From b2000cd3f9f8d005db660fd0a15e743eab26b57d Mon Sep 17 00:00:00 2001 From: Ewan Mellor Date: Wed, 20 Sep 2023 18:50:11 -0700 Subject: [PATCH 4/5] Fix: Chat - Link in end of line displays tooltip over text and not on link Changes from PR review: o Small tidyup. --- src/components/Tooltip/index.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/components/Tooltip/index.js b/src/components/Tooltip/index.js index ea8a5b1f87e3..7137b4c6a065 100644 --- a/src/components/Tooltip/index.js +++ b/src/components/Tooltip/index.js @@ -156,12 +156,11 @@ function Tooltip(props) { if (bounds.width === 0) { setIsRendered(false); } - const t = target.current; // 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(t, initialMousePosition.current.x, initialMousePosition.current.y); + const betterBounds = chooseBoundingBox(target.current, initialMousePosition.current.x, initialMousePosition.current.y); setWrapperWidth(betterBounds.width); setWrapperHeight(betterBounds.height); setXOffset(betterBounds.x); From 3dc42e9a034620d283ae61107ce6bd258d06909b Mon Sep 17 00:00:00 2001 From: Ewan Mellor Date: Thu, 21 Sep 2023 09:26:20 -0700 Subject: [PATCH 5/5] Fix: Chat - Link in end of line displays tooltip over text and not on link Change from PR review: move the onMouseEnter / onMouseLeave callbacks before the setIsHovered calls. --- src/components/Hoverable/index.js | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/components/Hoverable/index.js b/src/components/Hoverable/index.js index 610734b7dc2e..8ef09f34e409 100644 --- a/src/components/Hoverable/index.js +++ b/src/components/Hoverable/index.js @@ -154,21 +154,23 @@ class Hoverable extends Component { } }, onMouseEnter: (el) => { - this.setIsHovered(true); - if (_.isFunction(this.props.onMouseEnter)) { this.props.onMouseEnter(el); } + + this.setIsHovered(true); + if (_.isFunction(child.props.onMouseEnter)) { child.props.onMouseEnter(el); } }, onMouseLeave: (el) => { - this.setIsHovered(false); - if (_.isFunction(this.props.onMouseLeave)) { this.props.onMouseLeave(el); } + + this.setIsHovered(false); + if (_.isFunction(child.props.onMouseLeave)) { child.props.onMouseLeave(el); }