App: [$8000] Mac / Safari - Copy to clipboard Tooltip doesn't show copied on clicking the clipboard icon.

Thanks @s77rt. With the simplified proposal and both of @mollfpr’s concerns addressed, I think we’re in a good spot with your latest proposal, so let’s go with that!

@getusha that’s an interesting found.

IMO that’s a different issue, in the accepted proposal we don’t change any tooltip style related but yeah that appears because we enable the tooltip on focus.

cc @francoisl

Edit: We can even type on the composer.

@s77rt I raised the same earlier here but they are not considering keyboard navigation issues as of now.

Besides, my proposal, can you confirm to @getusha that his proposal is the same as @Pujan92 because you skipped @Pujan92 proposal. also you didn’t mention the regression, see here https://github.com/Expensify/App/issues/13146#issuecomment-1366844738

Yes, there’s a possibility where the regression arises as you mention. It’s okay, to correct each other but let’s keep the conversation civil.

---

Yes, i explained that in my comment you might seen that, which was if the tooltip updates and the user doesn’t have the chance to see it due to navigation / opened modal does the successText have any value? does this make sense to you?

I also have the same thought at first. To me having successText on the Hoverable component doesn’t make sense and I think your proposal has the same meaning as Pujan’s proposal where it’s easier to understand.

---

Put cursor into yellow circle and wait for the rectangle to come over the circle. :hover is not triggered until you move mouse on safari but working fine on chrome.

Thanks, @jatinsonijs for the summary and the example! I’ll read through your observation.

@s77rt paid $12000 @mananjadhav paid $250 anything missing!??!!?

We need to fill this out too. @mollfpr can you tackle the first three? @kevinksullivan, can you do the last one?

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• The PR that introduced the bug has been identified. Link to the PR:
• The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• A regression test has been added or updated so that the same bug will not reach production again. Link to the GH issue for creating the test here:

@jatinsonijs @s77rt Okay I confirm that onModalWillShow={DomUtils.blurActiveElement} is causing a problem regression here.

As per Contribute guideline regression are bugs which exist in staging but not in production, m i right @mollfpr ?

Yes, I believe @francoisl can create the issue and add the DeployBlockerCash to the issue and the payment will be done there.

@s77rt I am seeing it’s working nicely with the mouse moments 🎉 . For 2 times focus on the elements I agree with @getusha which doesn’t give a good experience. I know accessibility isn’t a concern right now but I believe we should not degrade in that direction with the current behavior.

Applied in Upwork. PR is ready.

Proposal 1 (Updated)

diff --git a/src/components/Hoverable/hoverablePropTypes.js b/src/components/Hoverable/hoverablePropTypes.js
index 07b8a8741e..c17fa804b6 100644
--- a/src/components/Hoverable/hoverablePropTypes.js
+++ b/src/components/Hoverable/hoverablePropTypes.js
@@ -19,9 +19,6 @@ const propTypes = {
 
     /** Function that executes when the mouse leaves the children. */
     onHoverOut: PropTypes.func,
-
-    // If the mouse clicks outside, should we dismiss hover?
-    resetsOnClickOutside: PropTypes.bool,
 };
 
 const defaultProps = {
@@ -29,7 +26,6 @@ const defaultProps = {
     containerStyles: [],
     onHoverIn: () => {},
     onHoverOut: () => {},
-    resetsOnClickOutside: false,
 };
 
 export {
diff --git a/src/components/Hoverable/index.js b/src/components/Hoverable/index.js
index f06ed56027..ccf46cf2ff 100644
--- a/src/components/Hoverable/index.js
+++ b/src/components/Hoverable/index.js
@@ -50,7 +50,12 @@ class Hoverable extends Component {
      */
     setIsHovered(isHovered) {
         if (isHovered !== this.state.isHovered && !(isHovered && this.hoverDisabled)) {
-            this.setState({isHovered}, isHovered ? this.props.onHoverIn : this.props.onHoverOut);
+            this.setState({isHovered});
+            if (isHovered) {
+                this.props.onHoverIn();
+            } else {
+                this.props.onHoverOut();
+            }
         }
 
         // we reset the Hover block in case touchmove was not first after touctstart
@@ -72,10 +77,6 @@ class Hoverable extends Component {
         if (!this.state.isHovered) {
             return;
         }
-        if (this.props.resetsOnClickOutside) {
-            this.setIsHovered(false);
-            return;
-        }
         if (this.wrapperView && !this.wrapperView.contains(event.target)) {
             this.setIsHovered(false);
         }
diff --git a/src/components/Tooltip/index.js b/src/components/Tooltip/index.js
index 8d3a345492..af46f8154d 100644
--- a/src/components/Tooltip/index.js
+++ b/src/components/Tooltip/index.js
@@ -189,7 +189,6 @@ class Tooltip extends PureComponent {
                     containerStyles={this.props.containerStyles}
                     onHoverIn={this.showTooltip}
                     onHoverOut={this.hideTooltip}
-                    resetsOnClickOutside
                 >
                     {child}
                 </Hoverable>
diff --git a/src/libs/Navigation/AppNavigator/modalCardStyleInterpolator.js b/src/libs/Navigation/AppNavigator/modalCardStyleInterpolator.js
index 1ec48a3563..af49ab82de 100644
--- a/src/libs/Navigation/AppNavigator/modalCardStyleInterpolator.js
+++ b/src/libs/Navigation/AppNavigator/modalCardStyleInterpolator.js
@@ -14,9 +14,9 @@ export default (
         },
     },
 ) => {
-    const translateX = Animated.multiply(progress.interpolate({
+    const right = Animated.multiply(progress.interpolate({
         inputRange: [0, 1],
-        outputRange: [isSmallScreenWidth ? screen.width : variables.sideBarWidth, 0],
+        outputRange: [isSmallScreenWidth ? -screen.width : -variables.sideBarWidth, 0],
         extrapolate: 'clamp',
     }), inverted);
 
@@ -26,7 +26,7 @@ export default (
     if (isFullScreenModal && !isSmallScreenWidth) {
         cardStyle.opacity = opacity;
     } else {
-        cardStyle.transform = [{translateX}];
+        cardStyle.right = right;
     }
 
     return ({

Details

This is my initial proposal which is also global and is based on the approach that we should not hide the tooltip in the first place, but this has one major drawback which is in Safari tooltips won’t hide unless you move the mouse. Just submitting in case we decided to ignore that behaviour on Safari.

Reviewing now…

Let’s hold on further comments until we’re able to get feedback on the existing proposals from @mananjadhav. Can that happen today?

I see your point. As for the regression test I don’t think we can do it for such a case, can we?

elementFromPoint always works as expected. Taking in consideration hidden elements and non accessible elements. I just didn’t reply to @getusha statement because I feel he just wanted to find an excuse to not go with my proposal without providing proofs/sources/scenarios.

The main objective here is to fix the issue the right way, does not much matter by whom.

I just don’t like the idea of partial fixes.

I think my proposal is the best so far just because the fix is global. The wrost case scenario is that the tooltip may stay visible until modal is fully closed which is I think a small price to pay. Even the probability of occurrences of that scenario is not that high.

@s77rt i am not comfortable you making this kind assumptions, it is not great to disregard and disrespect your peers opinion, As i worked on the previous issue (related to this), i just didn’t like the approach and was sharing my concern. I was also trying to see the best solution without having a proposal. Please let’s respect each others opinion and idea let’s not argue. Also please if you have proved regressions related to my proposal i would be happy to improve also take the teams opinion and try to improve.

Also for the statement let’s fix the issue globally, i don’t know what that means but if it is related to my proposal, it is not “impossible” or difficult as you tried to express that we can add successText on the tooltip everywhere and it’s good practice IMO. i will add it for every place and update my proposal. Please let’s not argue again, Thanks.

Just to leave a note here 😄, this solution is temporary since it breaks accessibility (can confirm TAB, all clickable have two focus) Currently we are ignoring those issues but when we start to fix accessibility this solution is must to remove.

This PR was reverted here as it was a temporary solution for this issue. cc @mollfpr @francoisl

Addressed https://github.com/Expensify/App/issues/14121#issuecomment-1396372989 Thanks for the ping @jatinsonijs

Compensating @mananjadhav 25% of the job price for their C+ work before they went OOO. This issue has close to 400 comments 😮 and there were multiple proposals that Manan reviewed.
I’ve added details to our Pricing Precedent - Contributor pay - edge cases - partial pay doc.

@mananjadhav can you address in the #contributor-plus Slack channel? Please provide reasoning and details, if we issue compensation, the goal would be to try to processize or document this edge case so we can reference again in the future. Also… saw 344 hidden items above 😨

@mollfpr can you help @kevinksullivan and I figured out how much is owed to whom? I believe it’s

• @s77rt is due $12000, $8000 for the job and $4000 for timeliness bonus (see below, timeliness bonuses won’t be issued in the future if there are regressions.)
• @mananjadhav $250 for reporting. Can you please accept the job and reply here once you have? https://www.upwork.com/jobs/~014886ff5b8f1db983
• @jatinsonijs your compensation should be associated with a separate GH, right?

I compensated @mollfpr $8000 total - $8000 job minus half for the regression (per C+ policy) = $4000. Plus 50% for timeliness bonus = $4000

CONTRIBUTING.md has been updated to reflect the below so, going forward, no timeliness bonuses will be issued if there are regressions.

If the PR causes a regression within 7 days of being deployed to production, contributors are not eligible for the 50% bonus.

Rehired with a new price @mollfpr . Thanks for correcting me!

@kevinksullivan I think my payment is half of the bounty since it has a regression (posted https://github.com/Expensify/App/issues/14121 and reported by @jatinsonijs https://github.com/Expensify/App/issues/13146#issuecomment-1374794996). Can you change the contract? Sorry for the inconvenience.

Sorry had linked it incorrectly @mollfpr , but I found you on upwork. You can just accept the offer 👍 .

Ok, thanks. I wasn’t aware that @getusha already raised…

@s77rt You’re right. So please update the description and add videos for the solved issue.

@getusha You are doing something wrong, double check from your side

@jatinsonijs Technically you found a regression, so I think you are eligible for the $250 compensation. Let @francoisl confirm on this

@jatinsonijs thank you

@getusha AvatarCropModal using Modal so happening in this case.

@getusha AvatarCropModal using Modal so happening in this case.

There is regression after merged this branch, I have reported this issue in slack @s77rt plz confirm https://expensify.slack.com/archives/C01GTK53T8Q/p1673116732450659?thread_ts=1672933287.271659&cid=C01GTK53T8Q

In image crop Drag, zoom, rotate your image… text is not selectable. cc @mollfpr

@mollfpr

1. The issue is that the element is being focused while it’s behind the modal. The tooltip showing is a conclusion of focusing the element. We should prevent focusing elements that are behind modals (implement focus trap). This issue is already on staging.
2. I’m not sure I understand your point here. When the tooltip has only one child and that child is a button, we should (for accessibility reasons) pass absolute with true. Even if we don’t, that won’t be a major problem but it’s better if we do. I just wanted to be clear on this point.

---

Yes, we don’t need resetsOnClickOutside in the tooltip. This prop has been removed. We are only “reverting” https://github.com/Expensify/App/pull/12572 (and properly fixing https://github.com/Expensify/App/issues/12025). There would be no more resetsOnClickOutside prop so reverting https://github.com/Expensify/App/pull/12677 is pointless.

~@mollfpr reproducible in production~

@getusha I tested your updated proposal and can confirm the navigation issue seems to be fixed. I like the approach overall! @mollfpr do you have a second to take another look?

@getusha Not much difference in your proposal, and I don’t think having a hundred listeners create from a tooltip is a good idea. The callback will call hideTooltip on every callback listener.

@francoisl If we want to go with the controlled resetsOnClickOutside on the tooltip, I will pick @Pujan92 proposal’s in the first place.

But in this way, we just patch the https://github.com/Expensify/App/issues/12025 regression not try to fix the root cause.

@s77rt thanks for the updated proposal, I had a few questions though:

1. Applied absolute when it’s more a fit (usually when the child is a button)

Are these changes necessary? I know you mentioned the intention is to avoid wrapping elements unnecessarily before, but… in the end, do we need to make these changes at all here?

1. In Hoverable, you edited the onMouseEnter and onMouseLeave functions with this:

+                    // Call the original onMouseEnter, if any
+                    const {onMouseEnter} = this.props.children;
+                    if (_.isFunction(onMouseEnter)) {
+                        onMouseEnter(el);
+                    }

similarly, why is that change required? Can you point to an example where it’s required please?

1. In BaseModal, this is new since your previous proposal, and I’m not seeing why it’s needed either:

onModalWillShow={DomUtils.blurActiveElement}

Feel free to explain your proposal in plain English first (without code) if that’s helpful - the idea is to make sure we all understand and agree on why any given change is required, and why we’re making it that way. Thanks!

@francoisl can you take a look at my proposal?

IMO resetsOnClickOutside isn’t avoidable, we have seen that on many instances. i think i have addressed the concern of toggled Tooltip not being dismissed while navigating.

Thanks, @getusha! I also will review your proposal later.

@mollfpr Besides document.activeElement.blur(); any thoughts on the proposal?

So far I like the idea of having focus/blur on the tooltip. I’ll review more after I test it again.

@francoisl

1. Yes, sorry I copy pasted and forgot to change the function name. I have updated my proposal

2. Okay, so here I need to explain, there are 2 cases where Tooltips are used with buttons (or Tooltip + Navigation)

   1. Case 1: Tooltip outside

       <Tooltip>
            <Pressable></Pressable>
       </Tooltip>
      

   2. Case 2: Tooltip inside

       <Pressable>
            <Tooltip></Tooltip>
       </Pressable>
      

   On case 1, if we click on <Pressable /> and it gets blurred, the blur event will bubble up to <Tooltip /> and we hide the tooltip, all good here. However on case 2, if we click on <Tooltip /> we can’t actually blur it since it’s not focus-able and even if we blur the <Pressable /> the blur event will never reach the tooltip (events never reach the children of the target). Making the tooltip focus able solves the issue since we can blur it now. There is also case 3 which I didn’t mention, which is <Pressable with Tooltip></Pressable> i.e. apply the tooltip functionality but without wrapping the element (absolute set to true)

   Taking mui-ui as a reference, case 3 is the default.

   You may wonder, can’t we switch all our Case 2 scenarios into Case 1 (or Case 3). No we can’t, some hierarchies are just too complex to make such a change, besides Tooltip is valid to be used in either way, after all Tooltip can and are used on non-buttons (texts, etc.).

3. Not really, the use of that workaround is not really related to <Freeze /> but to Safari in general. Take this example After you click the Pin/Unpin button, a modal will show up and cover it. on Chrome the mouseleave event will fire so the tooltip will hide. But on Safari, no event is fired (as explained the reason before), so we fire the blur event instead (since both are used to hide the tooltip). Now you may ask why we don’t fire the mouseleave since this is the missing event and not the blur event, well this part may be related to <Freeze /> since the mouseleave event is handled by <Hoverable /> and we hide the tooltip asynchronously which may get blocked by freeze taking effect while the blur event is handled by <Tooltip /> and we hide the tooltip synchronously (freeze wont take effect until we are done with hiding the tooltip). Maybe we can fire mouseleave event afterall, I just went with the more clearer way. As for the performance, it’s a cheap one call.

---

@jatinsonijs Thanks for testing. I will explain the behaviours:

1. Regarding modals (with RN at least). After you close a modal, it will focus on the previously focused element. You can verify by applying a custom style on focus. We can prevent this by not showing the tooltip on focus, but this is really a minor issue that does not deserve disabling a feature. Showing tooltips on focus is part of the natural tooltip behaviour.
2. As you stated, this is a tab accessibility issue, as long as you can focus the element the tooltip will show. Same note as above we can prevent this by not showing the tooltip on focus but it’s not our problem here (fixing tab navigation should auto-fix this)

---

@getusha The open prop of <Tooltip /> is used for whether the tooltip should be open(visible) or not (controlled tooltip) and not for whether we should hide the tooltip on mouse down or not.

@Pujan92 So changing resetsOnClickOutside to dismissOnClick and passing it through the component outside Tooltip will be the same?

@mollfpr Yes,resetsOnClickOutside property in the Hoverable won’t be required as it will be the default and must thing to do and doesn’t require any dynamic value. So we can replace it with the dismissOnClick and pass it within the Tooltip component.

Updated Proposal

diff --git a/src/components/ContextMenuItem.js b/src/components/ContextMenuItem.js
index c87fc5e9b..458019bb8 100644
--- a/src/components/ContextMenuItem.js
+++ b/src/components/ContextMenuItem.js
@@ -75,7 +75,7 @@ class ContextMenuItem extends Component {
         return (
             this.props.isMini
                 ? (
-                    <Tooltip text={text}>
+                    <Tooltip text={text} dismissOnClick={false}>
                         <Pressable
                             focusable
                             accessibilityLabel={text}
diff --git a/src/components/Hoverable/hoverablePropTypes.js b/src/components/Hoverable/hoverablePropTypes.js
index 07b8a8741..45a153d5e 100644
--- a/src/components/Hoverable/hoverablePropTypes.js
+++ b/src/components/Hoverable/hoverablePropTypes.js
@@ -20,8 +20,8 @@ const propTypes = {
     /** Function that executes when the mouse leaves the children. */
     onHoverOut: PropTypes.func,
 
-    // If the mouse clicks outside, should we dismiss hover?
-    resetsOnClickOutside: PropTypes.bool,
+    // Property to decide whether to reset the hover or not
+    dismissOnClick: PropTypes.bool,
 };
 
 const defaultProps = {
@@ -29,7 +29,7 @@ const defaultProps = {
     containerStyles: [],
     onHoverIn: () => {},
     onHoverOut: () => {},
-    resetsOnClickOutside: false,
+    dismissOnClick: true,
 };
 
 export {
diff --git a/src/components/Hoverable/index.js b/src/components/Hoverable/index.js
index f06ed5602..ec42b1931 100644
--- a/src/components/Hoverable/index.js
+++ b/src/components/Hoverable/index.js
@@ -17,11 +17,11 @@ class Hoverable extends Component {
 
         this.wrapperView = null;
 
-        this.resetHoverStateOnOutsideClick = this.resetHoverStateOnOutsideClick.bind(this);
+        this.resetHoverStateOnClick = this.resetHoverStateOnClick.bind(this);
     }
 
     componentDidMount() {
-        document.addEventListener('mousedown', this.resetHoverStateOnOutsideClick);
+        document.addEventListener('mousedown', this.resetHoverStateOnClick);
 
         // we like to Block the hover on touch devices but we keep it for Hybrid devices so
         // following logic blocks hover on touch devices.
@@ -38,7 +38,7 @@ class Hoverable extends Component {
     }
 
     componentWillUnmount() {
-        document.removeEventListener('mousedown', this.resetHoverStateOnOutsideClick);
+        document.removeEventListener('mousedown', this.resetHoverStateOnClick);
         document.removeEventListener('touchstart', this.disableHover);
         document.removeEventListener('touchmove', this.enableHover);
     }
@@ -68,15 +68,19 @@ class Hoverable extends Component {
      *
      * @param {Object} event - A click event
      */
-    resetHoverStateOnOutsideClick(event) {
+    resetHoverStateOnClick(event) {
         if (!this.state.isHovered) {
             return;
         }
-        if (this.props.resetsOnClickOutside) {
+
+        // If it is outside click, reset the hover state
+        if (this.wrapperView && !this.wrapperView.contains(event.target)) {
             this.setIsHovered(false);
             return;
         }
-        if (this.wrapperView && !this.wrapperView.contains(event.target)) {
+
+        // If it is inside click, reset based on the dismissOnClick value
+        if (this.props.dismissOnClick) {
             this.setIsHovered(false);
         }
     }
diff --git a/src/components/Tooltip/index.js b/src/components/Tooltip/index.js
index 8d3a34549..c6af41e1e 100644
--- a/src/components/Tooltip/index.js
+++ b/src/components/Tooltip/index.js
@@ -189,7 +189,7 @@ class Tooltip extends PureComponent {
                     containerStyles={this.props.containerStyles}
                     onHoverIn={this.showTooltip}
                     onHoverOut={this.hideTooltip}
-                    resetsOnClickOutside
+                    dismissOnClick={this.props.dismissOnClick}
                 >
                     {child}
                 </Hoverable>
diff --git a/src/components/Tooltip/tooltipPropTypes.js b/src/components/Tooltip/tooltipPropTypes.js
index 627f1c017..519148deb 100644
--- a/src/components/Tooltip/tooltipPropTypes.js
+++ b/src/components/Tooltip/tooltipPropTypes.js
@@ -32,6 +32,9 @@ const propTypes = {
 
     /** Maximum number of lines to show in tooltip */
     numberOfLines: PropTypes.number,
+
+    // Property which needs to pass to Hoverable to decide whether to reset hover state or not
+    dismissOnClick: PropTypes.bool,
 };
 
 const defaultProps = {
@@ -42,6 +45,7 @@ const defaultProps = {
     text: '',
     maxWidth: variables.sideBarWidth,
     numberOfLines: CONST.TOOLTIP_MAX_LINES,
+    dismissOnClick: true,
 };
 
 export {
diff --git a/src/pages/home/HeaderView.js b/src/pages/home/HeaderView.js
index 2e048ad18..bcb71d128 100644
--- a/src/pages/home/HeaderView.js
+++ b/src/pages/home/HeaderView.js
@@ -153,7 +153,7 @@ const HeaderView = (props) => {
                             )}
 
                             {shouldShowCallButton && <VideoChatButtonAndMenu isConcierge={isConcierge} />}
-                            <Tooltip text={props.report.isPinned ? props.translate('common.unPin') : props.translate('common.pin
')}>
+                            <Tooltip text={props.report.isPinned ? props.translate('common.unPin') : props.translate('common.pin
')} dismissOnClick={false}>
                                 <Pressable
                                     onPress={() => Report.togglePinnedState(props.report)}
                                     style={[styles.touchableButtonImage, styles.mr0]}
diff --git a/src/pages/home/report/ReportActionItem.js b/src/pages/home/report/ReportActionItem.js
index 7158f15f3..5adf47989 100644
--- a/src/pages/home/report/ReportActionItem.js
+++ b/src/pages/home/report/ReportActionItem.js
@@ -184,7 +184,7 @@ class ReportActionItem extends Component {
                 onSecondaryInteraction={this.showPopover}
                 preventDefaultContentMenu={!this.props.draftMessage}
             >
-                <Hoverable>
+                <Hoverable dismissOnClick={false}>
                     {hovered => (
                         <View accessibilityLabel="Chat message">
                             {this.props.shouldDisplayNewIndicator && (

Additional Info

• Why we are hiding the tooltip on both onBlur and onClick events, won’t onBlur be enough?
• Answer: Usually yes, onBlur would be enough, but we have many elements that does not get focused (probably because they are not buttons) thus they do not emit onBlur event but still do navigation. Without onClick we would have to rely on onMouseLeave event which may not fire unless you move the mouse especially on Safari (demo).

Proposal 4

diff --git a/src/components/Hoverable/hoverablePropTypes.js b/src/components/Hoverable/hoverablePropTypes.js
index 07b8a8741e..c17fa804b6 100644
--- a/src/components/Hoverable/hoverablePropTypes.js
+++ b/src/components/Hoverable/hoverablePropTypes.js
@@ -19,9 +19,6 @@ const propTypes = {
 
     /** Function that executes when the mouse leaves the children. */
     onHoverOut: PropTypes.func,
-
-    // If the mouse clicks outside, should we dismiss hover?
-    resetsOnClickOutside: PropTypes.bool,
 };
 
 const defaultProps = {
@@ -29,7 +26,6 @@ const defaultProps = {
     containerStyles: [],
     onHoverIn: () => {},
     onHoverOut: () => {},
-    resetsOnClickOutside: false,
 };
 
 export {
diff --git a/src/components/Hoverable/index.js b/src/components/Hoverable/index.js
index f06ed56027..13f3e73828 100644
--- a/src/components/Hoverable/index.js
+++ b/src/components/Hoverable/index.js
@@ -72,10 +72,6 @@ class Hoverable extends Component {
         if (!this.state.isHovered) {
             return;
         }
-        if (this.props.resetsOnClickOutside) {
-            this.setIsHovered(false);
-            return;
-        }
         if (this.wrapperView && !this.wrapperView.contains(event.target)) {
             this.setIsHovered(false);
         }
diff --git a/src/components/Tooltip/index.js b/src/components/Tooltip/index.js
index 8d3a345492..a2324a4b4e 100644
--- a/src/components/Tooltip/index.js
+++ b/src/components/Tooltip/index.js
@@ -148,6 +148,8 @@ class Tooltip extends PureComponent {
             <View
                 ref={el => this.wrapperView = el}
                 style={this.props.containerStyles}
+                onClick={this.hideTooltip}
+                onBlur={this.hideTooltip}
             >
                 {this.props.children}
             </View>
@@ -165,6 +167,8 @@ class Tooltip extends PureComponent {
                         ref(el);
                     }
                 },
+                onClick: this.hideTooltip,
+                onBlur: this.hideTooltip,
             });
         }
         return (
@@ -189,7 +193,6 @@ class Tooltip extends PureComponent {
                     containerStyles={this.props.containerStyles}
                     onHoverIn={this.showTooltip}
                     onHoverOut={this.hideTooltip}
-                    resetsOnClickOutside
                 >
                     {child}
                 </Hoverable>

Details

In this proposal, we only hide the tooltip on blur or on click.

The events bubbles per default, so the onClick will only occur if we don’t stop the propagation. And we do stop the propagation if <Pressable /> is used as in the case with the context menu tooltips (so the text changes smoothly). And if we do navigation (or anything else) we have onBlur that takes effect to prevent the tooltips from getting stuck or being active while out of view in general. Edit: Read https://github.com/Expensify/App/issues/13146#issuecomment-1368377652 for more info

However, our friend Safari is not very pleased, while it works okay there is one edge case that I found: If you use Safari on a minimal window view, open a chat and click the back button, the tooltip of that back button will stay visible until you click anywhere, for some reasons the blur events does not fire immediately, again on Safari that may be the spec. But it works fine on other places such the FAB, the emoji button, etc. Even though the number of users who may be using Safari in minimal view is very slim IMO, we can still fix this or at least make it better, by applying this change in : https://github.com/Expensify/App/blob/main/src/components/Hoverable/index.js#L53 Edit: After applying the change, the tooltip will hide after animation (on drawer fully opened). Edit2: That change is outdated, check the below comment (https://github.com/Expensify/App/issues/13146#issuecomment-1368257894)

-            this.setState({isHovered}, isHovered ? this.props.onHoverIn : this.props.onHoverOut);
+            this.setState({isHovered});
+            if (isHovered) {
+                this.props.onHoverIn();
+            } else {
+                this.props.onHoverOut();
+            }

https://user-images.githubusercontent.com/16493223/210133546-f8679c6c-5abc-4288-abf1-8050bb87cd11.mp4

@getusha Why does the Tooltip have two different diffs?

index 8d3a345492..4ed8827df8 100644
--- a/src/components/Tooltip/index.js
+++ b/src/components/Tooltip/index.js
@@ -190,6 +190,7 @@ class Tooltip extends PureComponent {
                     onHoverIn={this.showTooltip}
                     onHoverOut={this.hideTooltip}
                     resetsOnClickOutside
+                    successText={this.props.successText}
                 >
                     {child}
                 </Hoverable>

--- a/src/components/Tooltip/index.js
+++ b/src/components/Tooltip/index.js
@@ -190,6 +190,7 @@ class Tooltip extends PureComponent {
                     onHoverIn={this.showTooltip}
                     onHoverOut={this.hideTooltip}
-                    resetsOnClickOutside
+                    resetsOnClickOutside={_.isUndefined(this.props.successText)}
                 >
                     {child}
                 </Hoverable>

Just updated it you can ignore the first one.

@getusha Your solution is not global it is a workaround. And it’s the same as @Pujan92 I have said that enough time.

the tooltip is dismissed successfully and won’t show on chrome and on safari it get’s dismissed on mouse move.

Wrong. The tooltip will only be dismissed on mouse move if you do modal navigation. If you do drawer navigation where <Freeze /> kicks in, the tooltip will become stuck.

Ref: https://github.com/Expensify/App/issues/13146#issuecomment-1366761645 and https://github.com/Expensify/App/issues/13146#issuecomment-1366767105

---

I think it’s important to take in consideration future use cases, maybe there is none now, but later we can have a complex design, if we can find a solution that works even in the worst cases then we should be safe.

Ref: https://github.com/Expensify/App/issues/13146#issuecomment-1363280002

I’m not saying Navigation && Change tooltip text, this does not make sense because we won’t see the new text. I’m saying Navigation || Change tooltip text, where a button may do navigation or change the tooltip text based on some condition.

Ref: https://github.com/Expensify/App/issues/13146#issuecomment-1364537611

---

I think we are looking for a global solution (and not a workaround) with no regressions.

Ref: https://github.com/Expensify/App/issues/13146#issuecomment-1363235534

---

I think my latest proposal (proposal 3) is the best so far, and this is based on facts, not just because it’s my proposal. Correct me if I’m wrong.

My proposal is:

• Global
• Able to handle future cases (Navigation + Tooltip)
• No regression (except the one that @mollfpr and I mentioned and I have provided 2 complementary proposals for)

My proposal also relies on hiding the tooltip then showing it again, and we are discussing if we should hide the tooltip in the first place on the slack thread.

---

So far, I say we should hide the tooltip on mouse down otherwise we will have those issues:

1. The tooltip won’t hide on Safari until you move the mouse. There is no solution to this. Safari simply does not emit mouseenter/mouseleave events until you actually move the mouse.
2. The tooltip won’t hide on modal navigation until you move the mouse. We can fix this for Chrome, but Safari will still suffer for the same reason as above. ( we can reproduce this minimal example https://stackoverflow.com/questions/57507374/mouseleave-event-is-not-fired-when-element-transformed )
3. The tooltip will get stuck (even if you move the mouse, the tooltip will stay visible) on navigation due to <Freeze /> reopening this issue https://github.com/Expensify/App/issues/12025

---

That was a recap from my point of view.

In the meantime, I will be opening a discussion on the Slack channel about the expected behavior of each of the tooltip cases.

Yes @mollfpr first we have to clear the requirement so that we can test properly all the cases before submit proposal. Like some latest proposal have regression on safari. Tooltip will not hide until mouse move with toggled tooltip + navigation. Is it acceptable or not. I think first we should find out the all point which must need to consider.

@mollfpr Hi, just stumbled upon it. IMO, if there is no spec yet, it’s good to communicate with the design team to determine how many scenarios and corresponding expected behaviors. Please ignore my words if I’m wrong. 😂

@Pujan92 Found an issue with your proposal. The hovered state is not reset after open closing a modal.

https://user-images.githubusercontent.com/25520267/210084099-fc41beeb-f3e0-4a4d-8967-74234f0250d9.mov

@Pujan92 Yup, review it now!

Will review both proposals tonight!

wow @s77rt

Yes, i explained that in my comment you might seen that, which was if the tooltip updates and the user doesn’t have the chance to see it due to navigation / opened modal does the successText have any value? does this make sense to you? And thank you very much for the feedbacks.

@getusha Your proposal seems working fine for this issue but it can create regression if we have successText and navigation/modal action. Yes, we might be don’t have it right now but there’s a chance.

@JmillsExpensify I will start reviewing ASAP, thanks!

@getusha Your proposal causes a regression by not hiding the tooltip if the tooltip button opens a modal because your code won’t reach this part. I’m not a C+ but your code does not make sense, there is a condition to check the hover state if it’s true, why you are setting the state to true even if it’s already true, you can just do if (this.props.resetsOnClickOutside && !this.props.successText) { and this can be simplified to only use resetsOnClickOutside which gets us back to @Pujan92 proposal.

TLDR; Excluding the regression, they are the same.

Yes right its getting stuck on toggled tooltip + navigation + Freeze

Actually @getusha solution is the same as @Pujan92 here https://github.com/Expensify/App/issues/13146#issuecomment-1332161311

I am travelling for a few days and I can only check the updated conversations by day after tomorrow. Thanks for the patience and the active convseration folks.

Agree, that’s why I was trying to fix the issue without using event listeners…

yeah mousedown and mouseup are also event listener, in my proposal I have added condition on navigation event listen when we have toggled tooltip only then it should listen for state change otherwise not. so for normal tooltip it will work same as working now.

@s77rt may be I have used wrong name, I mean PR checklist something related to this like if you have added toggled tooltip in your work must check in safari and chrome etc… I saw for Input there is issue occur with emoji due to line height so they have added this in in their test list.

Hoverable component is low level component so we shouldn’t do much work on it. It may affect performance of app.

@jatinsonijs I’m still looking into this. We can differently fix this using event listeners, just looking if we can get it even better with less code being changed.

@jatinsonijs I see the problem Thanks! Shouldn’t be hard to fix. I hope

@getusha https://github.com/Expensify/App/issues/13146#issuecomment-1364528668 I have already explained, it’s just an example to catch future regressions

Here is a more imaginable scenario: A button tooltip that has successText but it will only show that successText in case a condition is met, otherwise redirection… Or the other way around A button tooltip that has errorText but it will only show that errorText in case a condition is not met, otherwise redirection… (a reasonable scenario to be found in a back button)

Makes sense now?

@s77rt your solution will not work with safari bcz safari do not change hover state also until mouse move I have already tested it. So with toggled tooltip + navigation it will not work

@getusha Navigation was just an example, the issue will happen if Freeze is triggered in any other ways. I like your proposal being simple but you are basically reverting the PR mentioned above (without providing an alt solution). I think that your proposal will likely cause future regressions. Besides it’s not global (and it’s impossible to make it global), we will have to pass successText={this.props.successText} to every Tooltip that may change it’s text.

I have been working on another proposal (following another approach) that should work exactly as expected and hopefully it’s bullet-proof. Posting in few mins…

@jatinsonijs Can provide an example?

Global plus button, composer plus button, emoji picker etc…

Thanks for the comments team.

@jatinsonijs You’re right that we have toggled Tooltip and hence here I am looking for a solution that works for both the cases (toggled tooltip + navigation). I don’t want to fix this as a workaround or one off and then open another issue in the future for a similar case. I hope it makes sense. Let’s try to solve the root cause.

---

@s77rt Of all the proposals I was almost okay with your proposal. But I have my concerns related to race condition and also updating the modal styles (sorry I just saw I didn’t mention in the previous summary).

https://github.com/Expensify/App/blob/70d87b0e6662c9fc06e85c0df35e3766fa4e1d04/src/components/Hoverable/index.js#L63-L67

If you read the comment, Modal is just an example. Are we sure we’re not breaking anything at all in any scenario?

---

I’ll also let @francoisl review the proposal.

Thanks for the patience here team.

Here’s the summary of all the proposals that have come so far.

1. Passing resetsOnClickOutside=false to specific tooltips or removing it completely. While this works, it is more of a workaround. As you can see this is reproducible to more than one places and I don’t think disabling this is the right way to handle this.

2. Replacing with click event Definitely not the way to go. It breaks things and I don’t see how click event will help for the hover components.

3. Adding setTimeout Let’s avoid workaround solutions for this one.

4. Proposals related to adding text or border/edges Don’t work and address the issue mentioned

5. keydown event Again not sure how it helps for hover.

6. Removing resetsOnClickOutside and removing props hover callbacks outside of the state callback, would lead to race conditions. I tested this and along with the other aspects of removing the code, moving to the right instead of transform, it didn’t work for me. I could’ve missed something, I couldn’t get enough evidence that Freeze stops the callbacks, and as I mentioned, let’s not add race conditions by calling props before the state is updated.

---

@francoisl What are your thoughts?

@kavimuru it’s reproducible in Pin/unpin chat too . It’s the same issue

Yes @mananjadhav right creating problem on safari even when cursor is on icon and on chrome (edges).

Proposal

diff --git a/src/components/ContextMenuItem.js b/src/components/ContextMenuItem.js
index c87fc5e9b4..458019bb8b 100644
--- a/src/components/ContextMenuItem.js
+++ b/src/components/ContextMenuItem.js
@@ -75,7 +75,7 @@ class ContextMenuItem extends Component {
         return (
             this.props.isMini
                 ? (
-                    <Tooltip text={text}>
+                    <Tooltip text={text} dismissOnClick={false}>
                         <Pressable
                             focusable
                             accessibilityLabel={text}
diff --git a/src/components/Tooltip/index.js b/src/components/Tooltip/index.js
index 8d3a345492..1f35d55f04 100644
--- a/src/components/Tooltip/index.js
+++ b/src/components/Tooltip/index.js
@@ -189,7 +189,7 @@ class Tooltip extends PureComponent {
                     containerStyles={this.props.containerStyles}
                     onHoverIn={this.showTooltip}
                     onHoverOut={this.hideTooltip}
-                    resetsOnClickOutside
+                    resetsOnClickOutside={this.props.dismissOnClick}
                 >
                     {child}
                 </Hoverable>
diff --git a/src/components/Tooltip/tooltipPropTypes.js b/src/components/Tooltip/tooltipPropTypes.js
index 627f1c0171..03f1707320 100644
--- a/src/components/Tooltip/tooltipPropTypes.js
+++ b/src/components/Tooltip/tooltipPropTypes.js
@@ -32,6 +32,9 @@ const propTypes = {
 
     /** Maximum number of lines to show in tooltip */
     numberOfLines: PropTypes.number,
+
+    /** Should hide tooltip on inside (hoverable) area press */
+    dismissOnClick: PropTypes.bool,
 };
 
 const defaultProps = {
@@ -42,6 +45,7 @@ const defaultProps = {
     text: '',
     maxWidth: variables.sideBarWidth,
     numberOfLines: CONST.TOOLTIP_MAX_LINES,
+    dismissOnClick: true,
 };
 
 export {

I think resetsOnClickOutside not correct name for tooltip bcz its not clear to code reader why tooltip should be reset. If we pass false in it why it should not reset on outside click but actually its works different in code.

We can use new props dismissOnClick or dissmissOnInsideClick. And we can also update prop name on Hoverable also resetOnClick or resetOnInsideClick it’s using only for Tooltip.

Reason why it’s behaving different on chrome and safari is still not clear. Even on Safari it’s not working with key prop.

hi @francoisl I am going to do that today.

@getusha Yes indeed, I have noticed that, I had an idea to solve this already. Will get back to this soon

There is no need of resetHoverStateOnOutsideClick This must be removed, it’s only a workaround. The real problem is on the fact that the element is suspended and callback setState does not get called.

Proposal

As resetsOnClickOutside added to solve this issue, we can pass value false for our use case(for copy actions)

https://github.com/Expensify/App/blob/1527fdcf443713bb058e6e76504006d74f6b1aa8/src/components/Tooltip/index.js#L192

resetsOnClickOutside={!_.isUndefined(this.props.resetsOnClickOutside) ? this.props.resetsOnClickOutside : true}

https://github.com/Expensify/App/blob/1527fdcf443713bb058e6e76504006d74f6b1aa8/src/components/ContextMenuItem.js#L77

<Tooltip text={text} resetsOnClickOutside={false}>