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

ewanmellor · 2023-09-19T23:05:09Z

Details

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.

Fixed Issues

$ #27585
PROPOSAL: #27585 (comment)

Tests

Action Performed:

On a platform with a mouse (i.e. not mobile):

Open the app
Open any report
Type in some text and at the end of line, add link with long text eg: Test test test test test test test test test test
Send the text, text should have text and link in first line and link should be continued in second line too. Adjust the window size to force the text to wrap if necessary.
Hover on link and observe that tooltip is displayed over text

Expected Result:
App should display tooltip over link on hover. The tooltip should appear over the same section of the link that the user is hovering over.

Verify that no errors appear in the JS console

Offline tests

N/A. There is no network interaction in this change.

QA Steps

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

… 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.

melvin-bot · 2023-09-19T23:05:14Z

@s77rt Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

github-actions · 2023-09-19T23:05:32Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

ewanmellor · 2023-09-19T23:07:14Z

I have read the CLA Document and I hereby sign the CLA

ewanmellor · 2023-09-19T23:07:26Z

recheck

ewanmellor · 2023-09-20T03:50:55Z

recheck

s77rt · 2023-09-20T17:41:39Z

src/components/Tooltip/tooltipPropTypes.js

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


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

s77rt · 2023-09-20T17:45:19Z

src/components/Hoverable/index.js

@@ -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));


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) ... />

s77rt · 2023-09-20T17:46:19Z

src/components/Tooltip/index.js

+            if (shouldUseMultilinePositioning) {
+                if (ev) {
+                    const {clientX, clientY, target} = ev;
+                    const bb = chooseBoundingBox(target, clientX, clientY);
+                    updateBounds(bb);
+                }
+            }


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); };

@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.

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:

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)

User lost hover, in this case onMouseEnter will be called once the user hover the next part and the tooltip will be placed correctly

@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.

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

@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.

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

@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?

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

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

s77rt · 2023-09-20T17:47:23Z

src/components/Tooltip/index.js

+ * enough then we go with that.
+ * @return {DOMRect} The chosen bounding box.
+ */
+function chooseBoundingBox(target, clientX, clientY, slop = 0) {


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

s77rt · 2023-09-20T17:48:01Z

src/components/Tooltip/index.js

+    // Fall back to the full bounding box if we failed to find a matching one
+    // (shouldn't happen).
+    return target.getBoundingClientRect();


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

s77rt · 2023-09-20T17:48:46Z

src/components/Tooltip/index.js

+ * @return {DOMRect} The chosen bounding box.
+ */
+function chooseBoundingBox(target, clientX, clientY, slop = 0) {
+    const bbs = target.getClientRects();


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

… 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.

ewanmellor · 2023-09-21T00:44:23Z

recheck

s77rt · 2023-09-21T01:33:45Z

src/components/Hoverable/index.js

+                if (_.isFunction(this.props.onMouseEnter)) {
+                    this.props.onMouseEnter(el);
+                }


Move this to be the first callback (same for onMouseLeave)

This is still not the first, it needs to goes before setIsHovered

s77rt · 2023-09-21T01:34:42Z

src/components/Tooltip/index.js

+        if (!t) {
+            return;
+        }


Is there a case where target is not defined?

No, but the type system would not be able to see that, because it cannot tell that updateBounds will never be called before a hover event makes the tooltip visible.

Regardless, this file is not TypeScript, so I've removed it.

s77rt · 2023-09-21T01:35:20Z

src/components/Tooltip/index.js

+        if (!t) {
+            return;
+        }
+        const betterBounds = chooseBoundingBox(t, initialMousePosition.current.x, initialMousePosition.current.y);


Please add a comment here on why we need better bounding box

… link Changes from PR review: o Reorder the callbacks from Hoverable. o Remove a null check. o Add a comment.

… link Changes from PR review: o Small tidyup.

ewanmellor · 2023-09-21T01:52:35Z

recheck

… link Change from PR review: move the onMouseEnter / onMouseLeave callbacks before the setIsHovered calls.

s77rt · 2023-09-22T22:58:37Z

src/components/Tooltip/index.js

+ * @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.


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

s77rt · 2023-09-22T22:58:55Z

src/components/Tooltip/index.js

+    // 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();


Suggested change

return target.getBoundingClientRect();

return bbs[0];

ewanmellor requested a review from a team as a code owner September 19, 2023 23:05

melvin-bot bot requested review from s77rt and removed request for a team September 19, 2023 23:05

s77rt suggested changes Sep 20, 2023

View reviewed changes

s77rt suggested changes Sep 21, 2023

View reviewed changes

ewanmellor added 2 commits September 20, 2023 18:46

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

f21c41c

… link Changes from PR review: o Reorder the callbacks from Hoverable. o Remove a null check. o Add a comment.

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

b2000cd

… link Changes from PR review: o Small tidyup.

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

3dc42e9

… link Change from PR review: move the onMouseEnter / onMouseLeave callbacks before the setIsHovered calls.

s77rt suggested changes Sep 22, 2023

View reviewed changes

ewanmellor closed this Sep 27, 2023

teneeto mentioned this pull request Oct 10, 2023

fix chat-link in end of line displays tooltip over text and not on link #29134

Merged

59 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ewanmellor commented Sep 19, 2023 •

edited

Loading

melvin-bot bot commented Sep 19, 2023

github-actions bot commented Sep 19, 2023 •

edited

Loading

ewanmellor commented Sep 19, 2023

ewanmellor commented Sep 19, 2023

ewanmellor commented Sep 20, 2023

s77rt Sep 20, 2023

s77rt Sep 20, 2023

s77rt Sep 20, 2023

ewanmellor Sep 20, 2023

s77rt Sep 20, 2023

ewanmellor Sep 20, 2023

s77rt Sep 20, 2023

ewanmellor Sep 20, 2023

s77rt Sep 20, 2023

ewanmellor Sep 20, 2023

s77rt Sep 20, 2023

ewanmellor Sep 21, 2023

s77rt Sep 20, 2023

s77rt Sep 20, 2023

s77rt Sep 20, 2023

ewanmellor commented Sep 21, 2023

s77rt Sep 21, 2023

ewanmellor Sep 21, 2023

s77rt Sep 21, 2023

ewanmellor Sep 21, 2023

s77rt Sep 21, 2023

ewanmellor Sep 21, 2023

s77rt Sep 21, 2023

ewanmellor Sep 21, 2023

ewanmellor commented Sep 21, 2023

s77rt Sep 22, 2023

s77rt Sep 22, 2023

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

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

Conversation

ewanmellor commented Sep 19, 2023 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented Sep 19, 2023

github-actions bot commented Sep 19, 2023 • edited Loading

ewanmellor commented Sep 19, 2023

ewanmellor commented Sep 19, 2023

ewanmellor commented Sep 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ewanmellor commented Sep 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ewanmellor commented Sep 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ewanmellor commented Sep 19, 2023 •

edited

Loading

github-actions bot commented Sep 19, 2023 •

edited

Loading