Refactor tooltip #14048

s77rt · 2023-01-06T01:00:55Z

@francoisl @mollfpr

Details

Refactored the tooltips and achieved natural tooltip behaviour.

Show tooltip on hover in ~~or focus~~
Hide tooltip on hover out or blur

Fixed Issues

$ #13146
PROPOSAL: #13146 (comment)

Tests

Login to any account
Open any chat (or any page that contain the context menu actions (copy to clipboard))
Click the copy to clipboard button
On Web and Desktop: Verify that before clicking you see "Copy to clipboard" tooltip and after clicking you see "Copied!" tooltip
On Native and mWeb: Verify that the button still works and no tooltip is shown

Verify that no errors appear in the JS console

Important: on Web, the test must be done on Safari (Chrome is already working fine).

Offline tests

n/a

QA Steps

Login to any account
Open any chat (or any page that contain the context menu actions (copy to clipboard))
Click the copy to clipboard button
On Web: Verify that before clicking you see "Copy to clipboard" tooltip and after clicking you see "Copied!" tooltip
On Native and mWeb: Verify that the button still works and no tooltip is shown

Verify that no errors appear in the JS console

Important: on Web, the test must be done on Safari (Chrome is already working fine).

PR Author Checklist

Screenshots/Videos

Web

web.mp4

Mobile Web - Chrome

mweb-chrome.mp4

Mobile Web - Safari

mweb-safari.mp4

Desktop

desktop.mp4

iOS

ios.mp4

Android

android.mp4

melvin-bot · 2023-01-06T01:01:02Z

@francoisl @mollfpr One of you needs to 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]

mollfpr · 2023-01-06T03:57:04Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- Android / native
- Android / Chrome
- iOS / native
- iOS / Safari
- MacOS / Chrome / Safari
- MacOS / Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

14048.-.Web.mov

Mobile Web - Chrome

14048.-.mWeb-Chrome.mov

Mobile Web - Safari

14048.-.mWeb-Safari.mov

Desktop

14048.-.Desktop.mov

iOS

14048.-.iOS.mov

Android

14048.-.Android.mov

mollfpr · 2023-01-06T08:12:58Z

@s77rt can you also add the action performed from the issue? We also should make sure that the initial issue is fixed.

s77rt · 2023-01-06T08:41:07Z

@mollfpr I didn't notice it was from the participant page, the issue was too old to remember but I used the context menu from the chat messages (both pages use <ContextMenuItem />)
Can you do the copy to clipboard action on your videos on that page? and keep my videos as they are?

mollfpr · 2023-01-06T08:43:04Z

Yeah, I'll do the video for the participant's page, we can keep your video as is.

Just need to update the test case with the action performed on the issue. Also, give note that the web test should be using the Safari browser for that case.

s77rt · 2023-01-06T09:59:45Z

Added a note about Web to test on Safari.

mollfpr

LGTM 👍

The checklist is here

s77rt · 2023-01-06T20:15:59Z

Regarding the latest commit we have agreed to remove the newly added feature onFocus since it's not required now.
@francoisl

getusha · 2023-01-06T20:49:47Z

Screen.Recording.2023-01-06.at.12.44.59.PM.mov

@s77rt It's not a big issue but we can improve that by applying the following if you want

index 580b8f9ab..aa7fbf232 100644
--- a/src/components/Modal/BaseModal.js
+++ b/src/components/Modal/BaseModal.js
@@ -92,6 +92,7 @@ class BaseModal extends PureComponent {
                 // eslint-disable-next-line react/jsx-props-no-multi-spaces
                 onBackButtonPress={this.props.onClose}
                 onModalWillShow={DomUtils.blurActiveElement}
+                onModalWillHide={DomUtils.blurActiveElement}
                 onModalShow={() => {
                     if (this.props.shouldSetModalVisibility) {
                         Modal.setModalVisibility(true);

after

Screen.Recording.2023-01-06.at.12.43.36.PM.mov

s77rt · 2023-01-06T20:51:47Z

@getusha Thanks, I'm aware of that, didn't add that because it's not required, the tooltip will always hide anyway

github-actions · 2023-01-06T21:18:23Z

Performance Comparison Report 📊

Significant Changes To Duration

Name	Duration
TTI	800.915 ms → 681.706 ms (-119.209 ms, -14.9%) 🟢

Show details

Name	Duration
TTI	Baseline Mean: 800.915 ms Stdev: 30.855 ms (3.9%) Runs: 741.4358700001612 759.2510010004044 767.7089520003647 767.7957100002095 771.972110000439 773.5682720001787 775.76109299995 776.8703330000862 782.4140480002388 782.4565530000255 785.1991969998926 788.7975479997694 789.1238749995828 790.2920509995893 795.152212000452 796.8284189999104 800.9181519998237 802.3061950001866 802.7701120004058 804.1487699998543 805.2169059999287 809.5762040000409 812.0130679998547 813.209146999754 823.8206580001861 829.9311009999365 845.8183639999479 845.8494189996272 846.8571939999238 859.6369510004297 881.6584839997813 Current Mean: 681.706 ms Stdev: 23.249 ms (3.4%) Runs: 635.2819010000676 642.6508900001645 643.6986689995974 654.0974909998477 657.8383440002799 659.7353750001639 664.5104470001534 667.7914709998295 670.4075999995694 675.8723769998178 675.9740439997986 677.7987580001354 678.5583349997178 679.2152519999072 679.2219789996743 679.2861249996349 681.0771070001647 683.5545009998605 689.3329769996926 689.3438900001347 690.2008370002732 692.6648719999939 692.9358670003712 701.7486190004274 702.5484090000391 702.9730759998783 703.2281740000471 711.8243429996073 732.7788100000471 735.0216110004112

Meaningless Changes To Duration

Show entries

Name	Duration
runJsBundle	179.688 ms → 182.125 ms (+2.438 ms, +1.4%)
nativeLaunch	12.000 ms → 12.719 ms (+0.719 ms, +6.0%)
regularAppStart	0.014 ms → 0.015 ms (+0.001 ms, +7.3%)

Show details

Name	Duration
runJsBundle	Baseline Mean: 179.688 ms Stdev: 16.680 ms (9.3%) Runs: 154 154 161 162 163 165 166 166 168 168 169 169 169 171 176 177 178 178 183 185 186 187 187 189 191 191 194 199 201 208 210 225 Current Mean: 182.125 ms Stdev: 19.400 ms (10.7%) Runs: 155 156 158 160 162 163 164 168 168 170 173 173 174 175 179 179 180 181 182 183 183 183 185 191 195 203 204 205 206 212 224 234
nativeLaunch	Baseline Mean: 12.000 ms Stdev: 2.610 ms (21.8%) Runs: 8 9 9 9 9 9 10 10 10 10 10 10 10 11 11 11 12 13 13 13 13 13 13 14 14 14 14 15 15 17 17 18 Current Mean: 12.719 ms Stdev: 3.457 ms (27.2%) Runs: 8 8 9 9 9 10 10 10 10 11 11 11 11 12 12 12 12 12 12 12 13 13 13 14 15 15 17 17 18 20 20 21
regularAppStart	Baseline Mean: 0.014 ms Stdev: 0.001 ms (5.1%) Runs: 0.012858000583946705 0.013020999729633331 0.013264999724924564 0.013345999643206596 0.0134680001065135 0.013671999797224998 0.013712000101804733 0.01371300034224987 0.013875999487936497 0.013915999792516232 0.013915999792516232 0.013955999165773392 0.013956000097095966 0.013957000337541103 0.014078999869525433 0.014201000332832336 0.014201000332832336 0.01428299956023693 0.01432300079613924 0.014485999941825867 0.014770000241696835 0.014973999932408333 0.014973999932408333 0.015054999850690365 0.015096000395715237 0.015137000009417534 0.01534000039100647 0.015381000004708767 0.015381000004708767 0.015463000163435936 Current Mean: 0.015 ms Stdev: 0.002 ms (10.9%) Runs: 0.01245100051164627 0.012899000197649002 0.013306000269949436 0.013630999252200127 0.013712000101804733 0.013753000646829605 0.013834000565111637 0.013915999792516232 0.013996999710798264 0.014201000332832336 0.014241000637412071 0.014485000632703304 0.014607999473810196 0.014852000400424004 0.014973999932408333 0.014973999932408333 0.015137000009417534 0.015300000086426735 0.015747000463306904 0.01582799945026636 0.015868999995291233 0.015868999995291233 0.016154000535607338 0.016193999908864498 0.016276000067591667 0.01696799974888563 0.017090000212192535 0.017902999185025692 0.018066000193357468 0.0188400000333786 0.019084000028669834

github-actions · 2023-01-06T21:18:25Z

@Expensify/mobile-deployers 📣 Please look into this performance regression as it's a deploy blocker.

francoisl · 2023-01-06T21:44:16Z

I don't think the above is an issue, but confirming with @AndrewGable since he's been working on this new perf comparison report a lot and he's familiar with it.

francoisl · 2023-01-06T21:52:19Z

Discussed internally, this seems to be a known issue that's being worked on in #14074. Removing the blocker label.

OSBotify · 2023-01-06T22:24:00Z

🚀 Deployed to staging by @francoisl in version: 1.2.50-6 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2023-01-09T18:04:53Z

🚀 Deployed to production by @Julesssss in version: 1.2.50-14 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

s77rt · 2023-02-27T21:57:32Z

This PR caused a regression #14858
This part of code is meant to be executed before navigation and not after.

This was my fault here, and to my defence I got confused by the comment here

App/src/libs/Navigation/NavigationRoot.js

Line 32 in d42e443

* Intercept navigation state changes and log it

the use of word "Intercept" made me think this is called before navigation while in fact it's not. Perhaps we should update the comment to prevent further related bugs.

getusha · 2023-02-28T04:32:24Z

I think this one is caused by this PR too #15271
There was expectation to fix this case but unfortunately it was missed.

parasharrajat · 2023-03-10T11:24:56Z

src/libs/Navigation/NavigationRoot.js

+    // and it's tooltip will stay visible.
+    // We blur the element manually to fix that (especially for Safari).
+    // More info: https://github.com/Expensify/App/issues/13146
+    DomUtils.blurActiveElement();


This can be a problem with this logic. For instance when the page is not changed on navigation. Normally, the navigation is handled by the framework, and clicking on links only changes the URL in the address bar without reloading the page.

This creates a possibility for an implementer to update the content of the page without changing the page.

If that happens, it will interfere with natural behavior. Thus this is dangerous. I always suggest restricting the impact of logic to a sufficient context. stateChange fires literally every change in state which could be anything(not only navigation) and thus this action might cause regressions.

Also, this change outdates the function name parseAndLogRoute. We should update that to reflect the new logic.

cc: C+

I think you are referencing to an outdated code, this has been moved to a better place #15249

Just saw one regression from it #14048 (comment)

Or does the new place also have the same effects? LMK I will check

I think you are referencing to an outdated code,

Oops, I see.

parasharrajat · 2023-03-10T11:35:06Z

This is not working for me on Firefox. When I tab focus on the MiniAction menu, tooltip is no shown.

screen-2023-03-10_17.04.40.mp4

cc: @mollfpr @s77rt

s77rt · 2023-03-10T11:42:39Z

IIRC, It was never being shown before, at some point we decided to show the tooltip on focus but caused a regression, turns out that regression is not from this PR but from RNW and we fixed that, so maybe we can bring back that feature again.

parasharrajat · 2023-03-10T11:54:52Z

Sorry, I didn't get it. Are you saying that we reverted the newly implemented behavior of this PR in some other PR due to regressions?

But I can still see code changes of this PR on main. Can you please share links to the PR/regressions you are mentioning? It will help me understand better.

s77rt · 2023-03-10T13:02:17Z

Can we take this to Slack? I think it would be better to explain what went in this PR as there is a lot to say. A conversation may be better and faster.

parasharrajat · 2023-03-10T13:11:51Z

Sure.

parasharrajat · 2023-03-10T16:44:51Z

Opened a new discussion https://expensify.slack.com/archives/C01GTK53T8Q/p1678466671693099

parasharrajat · 2023-03-10T17:23:02Z

Note: This PR caused a regression #15796.

cc: @mollfpr

parasharrajat · 2023-03-27T12:51:27Z

I was reading about this PR and its issue. But there are a lot of discussions and It seems like the proposal was changed around 10 times to reach this solution. But still, I haven't got the full context of it.

Can someone please help me with the following questions?
Questions:

What is the purpose behind making each tooltip target focusable? There was mention on mui-tooltip [$8000] Mac / Safari - Copy to clipboard Tooltip doesn't show copied on clicking the clipboard icon. #13146 (comment). I looked at the code and it does not seem they force focusable property to targets. I tested it as well.
Shouldn't it depend on the focusable property of the target instead of enforcing it to all?

cc: @s77rt @mollfpr

getusha · 2023-03-27T13:02:14Z

@parasharrajat initially we were hiding the tooltip by detecting mousedown and @s77rt proposed to make the tooltip target focusable so we can use onBlur which of course occurred a break in accessibility. so in short the focusable was added to enable onBlur and ensure that the tooltip hides on navigation and modal interactions which still don't. if we remove the focusable i think we will reintroduce #12025.

Hope it helps 🙂

parasharrajat · 2023-03-27T13:08:49Z

Thanks.

So if focus event on the target opens a tooltip, then blur should be fired automatically without the need for an explicit focusable prop. IMO, other nodes that do not trigger a tooltip with focus should be handled based on pointer events.

s77rt · 2023-03-27T17:20:09Z

@parasharrajat

What is the purpose behind making each tooltip target focusable? There was mention on mui-tooltip #13146 (comment). I looked at the code and it does not seem they force focusable property to targets. I tested it as well.

I have briefly explained the need of tooltips to be focusable (but not necessary tabbable) here: [HOLD for newdot C+ payment] [$1000] Web - Emoji Reaction - Reaction emoji is highlighted twice when using Tab key #15796 (comment)
mui-tooltip does not wrap elements i.e. in our code that would would mean to pass absolute prop

Shouldn't it depend on the focusable property of the target instead of enforcing it to all?

I think my explanation above answers this.

So if focus event on the target opens a tooltip, then blur should be fired automatically without the need for an explicit focusable prop

Blur event only bubbles to elements that are focusable.

parasharrajat · 2023-03-29T23:12:49Z

@s77rt I read that. Do you have any example of Tooltip in our app where blur events were not bubbling? I want to test over it the new solutions we are working on.

s77rt · 2023-03-29T23:24:55Z

@parasharrajat Basically the reported ones on #12025

Screen.Recording.2022-10-20.at.1.44.47.AM.mov

s77rt added 2 commits January 6, 2023 00:34

Refactored Tooltips

275b0ae

Do not show tooltips on mWeb

2c6bd3e

s77rt requested a review from a team as a code owner January 6, 2023 01:00

melvin-bot bot requested review from francoisl and mollfpr and removed request for a team January 6, 2023 01:01

s77rt mentioned this pull request Jan 6, 2023

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

Closed

jatinsonijs mentioned this pull request Jan 6, 2023

[HOLD for payment 2023-06-21] Tooltip getting stuck with combination of esc key and hover effect #13947

Closed

2 tasks

mollfpr previously approved these changes Jan 6, 2023

View reviewed changes

Removed onFocus functionality

6c91027

s77rt dismissed mollfpr’s stale review via 6c91027 January 6, 2023 20:13

francoisl approved these changes Jan 6, 2023

View reviewed changes

francoisl merged commit 54b4841 into Expensify:main Jan 6, 2023

github-actions bot added the DeployBlockerCash This issue or pull request should block deployment label Jan 6, 2023

OSBotify mentioned this pull request Jan 6, 2023

Deploy Checklist: New Expensify 2023-01-06 #14067

Closed

36 tasks

francoisl removed the DeployBlockerCash This issue or pull request should block deployment label Jan 6, 2023

s77rt mentioned this pull request Jan 7, 2023

Removed forgotten code that is not required after removing onFocus #14115

Merged

53 tasks

mollfpr mentioned this pull request Jan 8, 2023

[HOLD for payment 2023-01-17] The text on the edit photo profile modal is not selectable #14121

Closed

6 tasks

luacmartins mentioned this pull request Jan 18, 2023

Dialog close button's tooltip appears behind the modal #14179

Closed

2 tasks

s77rt mentioned this pull request Feb 12, 2023

[HOLD for payment 2023-03-06] [$1000] Sign out - There is no cursor in the input field after sign out #14858

Closed

6 tasks

Santhosh-Sellavel mentioned this pull request Feb 17, 2023

DOM: Blur activeElement before navigation #15249

Merged

54 tasks

aimane-chnaif mentioned this pull request Feb 26, 2023

[HOLD for payment 2023-03-14] [$1000] Report item remains hovered even when mouse is moved away from it. #15300

Closed

1 task

0xmiros mentioned this pull request Mar 6, 2023

feat: add/remove reaction #15210

Merged

56 tasks

bernhardoj mentioned this pull request Mar 10, 2023

[HOLD for newdot C+ payment] [$1000] Web - Emoji Reaction - Reaction emoji is highlighted twice when using Tab key #15796

Closed

6 tasks

parasharrajat reviewed Mar 10, 2023

View reviewed changes

parasharrajat mentioned this pull request Mar 27, 2023

refactor tooltip to fix double focusable component #16052

Merged

55 tasks

BeeMargarida mentioned this pull request Jun 30, 2023

[HOLD for payment 2023-07-14] [$1000] The tooltip disappears when the "Copy to clipboard" option is clicked multiple times. #19693

Closed

6 tasks

sobitneupane mentioned this pull request Oct 30, 2023

[HOLD for payment 2023-10-23] [HOLD for payment 2023-10-23] [HOLD for payment 2023-10-20] [HOLD for payment 2023-10-20] [$500] Dev: Web - focus lose on click ctrl+shift+k again #27860

Closed

6 tasks

Refactor tooltip #14048

Refactor tooltip #14048

Conversation

s77rt commented Jan 6, 2023 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented Jan 6, 2023

mollfpr commented Jan 6, 2023 • edited Loading

Reviewer Checklist

Screenshots/Videos

mollfpr commented Jan 6, 2023

s77rt commented Jan 6, 2023

mollfpr commented Jan 6, 2023 • edited Loading

s77rt commented Jan 6, 2023

mollfpr left a comment

Choose a reason for hiding this comment

s77rt commented Jan 6, 2023

getusha commented Jan 6, 2023

s77rt commented Jan 6, 2023

github-actions bot commented Jan 6, 2023

Performance Comparison Report 📊

Significant Changes To Duration

Meaningless Changes To Duration

github-actions bot commented Jan 6, 2023

francoisl commented Jan 6, 2023

francoisl commented Jan 6, 2023

OSBotify commented Jan 6, 2023

OSBotify commented Jan 9, 2023

s77rt commented Feb 27, 2023

getusha commented Feb 28, 2023

parasharrajat Mar 10, 2023

Choose a reason for hiding this comment

parasharrajat Mar 10, 2023 • edited Loading

Choose a reason for hiding this comment

s77rt Mar 10, 2023

Choose a reason for hiding this comment

parasharrajat Mar 10, 2023

Choose a reason for hiding this comment

s77rt Mar 10, 2023

Choose a reason for hiding this comment

parasharrajat Mar 10, 2023

Choose a reason for hiding this comment

parasharrajat commented Mar 10, 2023

s77rt commented Mar 10, 2023 • edited Loading

parasharrajat commented Mar 10, 2023

s77rt commented Mar 10, 2023

parasharrajat commented Mar 10, 2023

parasharrajat commented Mar 10, 2023

parasharrajat commented Mar 10, 2023

parasharrajat commented Mar 27, 2023 • edited Loading

getusha commented Mar 27, 2023

parasharrajat commented Mar 27, 2023

s77rt commented Mar 27, 2023

parasharrajat commented Mar 29, 2023 • edited Loading

s77rt commented Mar 29, 2023

s77rt commented Jan 6, 2023 •

edited

Loading

mollfpr commented Jan 6, 2023 •

edited

Loading

mollfpr commented Jan 6, 2023 •

edited

Loading

parasharrajat Mar 10, 2023 •

edited

Loading

s77rt commented Mar 10, 2023 •

edited

Loading

parasharrajat commented Mar 27, 2023 •

edited

Loading

parasharrajat commented Mar 29, 2023 •

edited

Loading