refactor tooltip to fix double focusable component

[bernhardoj]

Details

We want to fix double highlight of component with tooltip.

Fixed Issues

$ #15796
PROPOSAL: #15796 (comment)

Tests

Same as QA Steps

• Verify that no errors appear in the JS console

Offline tests

Same as QA Steps

QA Steps

Web & Desktop only

1. Open any report
2. Send a message
3. Hover over the message and react with the first emoji
4. Press tab to navigate through the emoji reactions
5. Verify each emoji reaction is only highlighted once

• Verify that no errors appear in the JS console

PR Author Checklist

• I linked the correct issue in the ### Fixed Issues section above
• I wrote clear testing steps that cover the changes made in this PR
  • I added steps for local testing in the Tests section
  • I added steps for the expected offline behavior in the Offline steps section
  • I added steps for Staging and/or Production testing in the QA steps section
  • I added steps to cover 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 tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
• I included screenshots or videos for tests on all platforms
• I ran the tests on all platforms & verified they passed on:
  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop
• I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
• I followed proper code patterns (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
    • If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
  • 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 followed the guidelines as stated in the Review Guidelines
• I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
• 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
• I verified that if a function's arguments changed that all usages have also been updated correctly
• 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.
• If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
• I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

Screen.Recording.2023-03-16.at.03.14.34.mov

Mobile Web - Chrome

Mobile Web - Safari

Desktop

Screen.Recording.2023-03-16.at.03.39.56.mov

iOS

Android

[MelvinBot]

@pecanoro @parasharrajat 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]

[bernhardoj]

Some additional screenshot/videos to make sure other component is fine.

Screen.Recording.2023-04-14.at.17.22.34.mov

Screen.Recording.2023-04-14.at.14.23.34.mov

Screen.Recording.2023-03-16.at.03.15.22.mov

[bernhardoj]

Summary of the changes:

1. Only allow Tooltip to have a single children
2. Remove all absolute and focusable prop from Tooltip

Why I remove the display inline container style?
Because the text has an inline display by default, so we don't need that because we don't wrap the text by view anymore.

Why are some Tooltip position switched with its parent? For example:

<View>
  <Tooltip>
    <Avatar />
  </Tooltip>
</View>

to

<Tooltip>
  <View>
    <Avatar />
  </View>
</Tooltip>

It is because some component like Avatar can't receive the onMouseEnter, etc. callback because Avatar is wrapped with context provider component, so we need to wrap it with a view. Also, with this concern, we can't decide whether the Tooltip children need a wrapper view or not, so it's better to take out that responsibility as mentioned on my proposal.

Let me know if you have any other question. Sorry for taking it long!

[parasharrajat]

Let's revert this change as it is not related to this PR.

[parasharrajat]

Please revert..

[parasharrajat]

I think you didn't see my comments during the proposal approval.

add the wrapper only when children do not fit into the criteria for the absolute prop.

Here is what I think should be done.

1. Keep the Wrapper View.
2. First check for a valid single child. If yes, apply the absolute Tooltip logic.
3. otherwise, render the wrapper View as before.

We don't want an explicit dependency for using tooltips. That the child has to be a validELement. The Wrapper View logic is working great all over the app. It does not have to be changed. For example, #16052 (comment)

[parasharrajat]

Once this is done, I will re-review it.

[bernhardoj]

We can't just simply check if it's a single child and decide to not wrap it with a view as I explained here #16052 (comment). So, if we have this component.

<Tooltip>
  <Avatar />
</Tooltip>

it will become like this

<Context.Provider onBlur onMouseEnter onMouseLeave etc..>
  <Avatar />
</Context.Provider>

and the callback does not work for context provider, so the tooltip won't show.

[parasharrajat]

How about this

 if (this.props.absolute && React.isValidElement(this.props.children)) {
            return React.cloneElement(React.Children.only(this.props.children),

For singular, we can use https://legacy.reactjs.org/docs/react-api.html#reactchildrencount

[bernhardoj]

Wait, so we still use the absolute props? Sorry if I misunderstood your intention. Btw, I will continue to look at this tomorrow as it's EOD for me.

[parasharrajat]

No, I just posted the code for reference. I think the condition might be something like

 React.Children.count(children) === 1 && React.isValidElement(this.props.children)

But I just noticed that a Custom component will also be true for valid elements and that was the reason I had to create an absolute prop instead of applying it by default in the initial implementation. Damn, back to basics.

But I don't like the isFocusable technique. It is very hacky.

[parasharrajat]

May be check for specific elements such as View, Text...

[parasharrajat]

Example link.

[parasharrajat]

Only allow Tooltip to have a single children

Why does it have to be like that?

so it's better to take out that responsibility as mentioned on my proposal.

How is it better? The more your put decisional choices to the developers the more errors they will make. Currently, you don't have to even think about anything for using Tooltips. The only confusion was around absolute but it had a clear purpose. Now we are removing that confusion of absolute. I think this is best.

[bernhardoj]

Why does it have to be like that?

Actually, current Tooltip will always have one child because we will wrap the children with a View at the end. But with this PR, we can't do this anymore.

<Tooltip>
  <Component1 />
  <Component2 />
</Tooltip>

We need to wrap both component 1&2 with a View.

<Tooltip>
  <View>
    <Component1 />
    <Component2 />
  </View>
</Tooltip>

It will not become the Tooltip responsibility to add the View wrapper anymore.

How is it better?

My initial thought is that we can pass all view related props to the view wrapper we put manually instead of passing it all to the Tooltip. I have no problem to do the wrapping inside the Tooltip, but my main concern is at above #16052 (comment).

EDIT: I just realized you already comment there after refreshing 😄

[parasharrajat]

My initial thought is that we can pass all view related props to the view wrapper we put manually instead of passing it all to the Tooltip

Ok. It might be needed in the future but as of now, I haven't seen any such need.

It will not become the Tooltip responsibility to add the View wrapper anymore.

Which one is better? The user manually puts Views and decides if it is needed or if Tooltip automatically applies it where needed.

For example, if I create a modal viewer lib for you and ask you to use it. What will be your preference.

1. Use a simple Modal.show(el).
2. Or first create a target where the modal will be rendered in dOM and then use Modal.show(el). Now manage styling for the target.

#16052 (comment)

Let me know if that does not work.

[bernhardoj]

Both approaches have its pro and con, I just think it's more customizable with putting the View wrapper manually and also the concerns that we've been discussing 😄.

Let me know if that does not work.

Yeah, it does not work because context provider is still a valid element.

May be check for specific elements such as View, Text...

That's actually what isFocusable is doing. It checks if the children is either TouchableOpacity, TouchableWithoutFeedback, Pressable, and a pressable Text. Maybe we can remove the Text part because by default, a pressable Text is not focusable. I also remember that we are planning to replace all touchable with our own Pressable, so when that is done, we can just check if the children is a Pressable.

[parasharrajat]

Both approaches have its pro and con, I just think it's more customizable with putting the View wrapper manually and also the concerns that we've been discussing smile.

Yup but I don't see a need for customizing the wrapper currently in the app. That's why I don't want to put extra handling for Tooltip at this stage.

I think the focusable part is hacky. You are calling the function isFocusable but you are not checking the focusable prop on the element. It is confusing.

Let's try to find a way to do #16052 (comment).
Especially, First check for a valid single child. If yes, apply the absolute Tooltip logic.

[bernhardoj]

First check for a valid single child. If yes, apply the absolute Tooltip logic.

I think this is the hardest thing to achieve. I am thinking that we should check the available props of the children, but I don't find a way to do that. I have printed the children object and does not find any property indicating the available props.

You're right we should check the focusable props. I looked at RN TouchableOpacity and it uses this condition to check the focusable. We can use it too like this this.props.children.focusable !== false && this.props.children.onPress !== undefined.

I also just realized. Whatever approaches we take, we still need to know whether the children should be focusable or not.

[parasharrajat]

Thanks for looking into this. But these focusable checks do not seem very robust.

OK, at this stage, I will say we just apply the focusable part of the proposal which is to add focusable={False} to the Tooltip wrapper.

[bernhardoj]

Do you mean do it like this?

        let child = (
            <View
                ref={el => this.wrapperView = el}
                onBlur={this.hideTooltip}
+               focusable={false}
-               focusable={this.props.focusable}
                style={this.props.containerStyles}
            >
                {this.props.children}
            </View>
        );

But what about the focusable for non-wrapper?

        if (this.props.absolute && React.isValidElement(this.props.children)) {
            child = React.cloneElement(React.Children.only(this.props.children), {
                ref: (el) => {
                    this.wrapperView = el;

                    // Call the original ref, if any
                    const {ref} = this.props.children;
                    if (_.isFunction(ref)) {
                        ref(el);
                    }
                },
                onBlur: (el) => {
                    this.hideTooltip();

                    // Call the original onBlur, if any
                    const {onBlur} = this.props.children;
                    if (_.isFunction(onBlur)) {
                        onBlur(el);
                    }
                },
                focusable: true,
            });
        }

But these focusable checks do not seem very robust.

Is there any case for the condition to "fails"? The case that I thought would fail is when the children have onPress prop even though the children does not support those props. But I don't think that's a problem because why in the first place we add those props if the component does not support it, right? Let say somehow the condition fails and we set focusable to true to a custom component. Nothing will happen because custom component does not support focusable props just like onBlur and any other tooltip related callback. Setting focusable props to a component that does not support it, does not affect anything. The purpose of the condition is just to make sure a component that is focusable stays focusable.

The alternative I could think is to pass focusable as props to the Tooltip for non-wrapper. If the children is focusable, we set the props to true, otherwise false.

[parasharrajat]

But these focusable checks do not seem very robust.

By this, I mean the isFocusable function. In general, I was thinking to set the focusable to false for the Tooltip wrapper. If the tooltip is used with absolute, then it might be fine that the target is focusable. The purpose is to disable double focus. The double focus is generated because of the focusable wrapper.

Is there an advantage of binding the focusable to the prop?

[bernhardoj]

Ah, I see. But if the children is a normal Text for example, it will become focusable. Is that okay? With the focusable props, we can prevent that to happen, but we need to pass that props to every Tooltip that uses absolute 😄.

Edit: okay, I just realized again, what if the tooltip children already have its own View wrapper and we apply the absolute logic to it with focusable set to true. The double highlight will still happen. At the end, we need the isFocusable function 😄. I think we can combine both condition (check the component name & focusable + onPress props).

children.focusable || (children.focusable !== false && children.onPress !== undefined && ['TouchableOpacity', 'Pressable', 'TouchableWithoutFeedback'].includes(children.type.displayName))
^
this is to respect children focusable props

Case 1: Pressable

<Tooltip>
  <Pressable />
</Tooltip>

isFocusable is true

Case 2: Non-focusable Pressable

<Tooltip>
  <Pressable focusable={false}/>
</Tooltip>

isFocusable is false

Case 3: Component that is not focusable by default

<Tooltip>
  <Text />
</Tooltip>

isFocusable is false

Case 4: Component with focusable props

<Tooltip>
  <Text focusable/>
</Tooltip>

isFocusable is true

<Tooltip>
  <Text focusable={false}/>
</Tooltip>

isFocusable is false

Case 5: Component that does not support focusable

<Tooltip>
  <CustomComponent />
</Tooltip>

isFocusable is false

Case 6: Component that does support focusable (except touchable and pressable)
Now, here is the possible fail case. Let say we have a custom component (either we made it or from library) that accepts focusable props with the default value as true.

<Tooltip>
  <CustomComponent />
</Tooltip>

isFocusable will be false even though the default value is true. In this case, we can just wrap it with a View, and we will go back to case 3/4.

Both case 5 & 6 assuming custom component accepts onBlur, onMouseEnter, and onMouseLeave callback.
So far, searching all Tooltip usage, no case for this yet and it will very less likely to happen and at the end we can wrap the custom component with a View which will go back to case 3/4.

[parasharrajat]

I will pull down the branch and try to see what is best.

[parasharrajat]

Can you please merge main into this?

[bernhardoj]

Merged with main.

[parasharrajat]

Hmm, It is hard to find a proper way of detecting a focusable element statically not counting your isFocusable approach.

Why do I think that isFocusable sucks?

This list of components can not be fixed. A slightly smart component that manages focusable prop will break it.

[parasharrajat]

@bernhardoj Once conflicts are resolved, please ping us so that we can review them asap. This PR is likely to get conflicts very frequently.

[bernhardoj]

@pecanoro @parasharrajat Solved. I will try to check this regularly to see if there is any conflict.

[pecanoro]

Reviewing now before we get more conflicts!

[pecanoro]

Ok! Tested! Merging! 🤞

[OSBotify]

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

[fedirjh]

Just noticed a weird bug with tooltip on main while testing another PR. There is a potential regression from this PR, for small view on desktop or web , the tooltip doesn’t not hide on navigation :

cc @bernhardoj, @parasharrajat let’s fix it before it reach staging

bugTooltip.mov

[parasharrajat]

Let me check.

[parasharrajat]

Oh, it does seem like the issue originated from this PR.

[stitesExpensify]

Related thread https://expensify.slack.com/archives/C01GTK53T8Q/p1685030189698979

[bernhardoj]

That is the expected issue we discussed here #16052 (comment) before which will be gone after navigation reboot. cc: @parasharrajat

I will look into other linked issue

[bernhardoj]

Found the issue of emoji picker. Trying to find a solution.

[bernhardoj]

Here is the cause of the issue:

App/src/components/Reactions/AddReactionBubble.js

Lines 48 to 71 in 0925c25

	const onPress = () => {
	const openPicker = (refParam, anchorOrigin) => {
	EmojiPickerAction.showEmojiPicker(
	() => {},
	(emojiCode, emojiObject) => {
	props.onSelectEmoji(emojiObject);
	},
	refParam || ref.current,
	anchorOrigin,
	props.onWillShowPicker,
	);
	};
	if (props.onPressOpenPicker) {
	props.onPressOpenPicker(openPicker);
	} else {
	openPicker();
	}
	};
	return (
	<Tooltip text={props.translate('emojiReactions.addReactionTooltip')}>
	<Pressable
	ref={ref}

The emoji picker requires ref when showing it. However, the ref is undefined. If we look at both Tooltip and Hoverable, we only accept a callback ref.

App/src/components/Tooltip/index.js

Lines 124 to 132 in 0925c25

	ref: (el) => {
	this.wrapperView = el;
	// Call the original ref, if any
	const {ref} = this.props.children;
	if (_.isFunction(ref)) {
	ref(el);
	}
	},

In our case, the emoji add reaction ref is a ref object.

It works fine before because we have a View wrapper which won't get conflicted with it's children.

Solution:

1. Remove the element clone from Tooltip. The purpose of the clone is to get the ref and store it in wrapperView. However, wrapperView is not being used anymore after this PR is merged.
2. Handle ref object in Hoverable.

ref: (el) => {
    this.wrapperView = el;

    // Call the original ref, if any
    const {ref} = child;
    if (_.isFunction(ref)) {
        ref(el);
+       return;
    }
+
+   if (_.isObject(ref)) {
+       ref.current = el;
+   }
},

No need to check if current property exist or not. If it does not exist, then we can know that the component pass random object to the ref.

Let me know if you agree @parasharrajat @pecanoro and I will open the PR.
and should we also handle this in one PR?

[parasharrajat]

Please raise the PR for theses issues. This is more urgent so let's get This fixed first.

[bernhardoj]

@parasharrajat @pecanoro PR is ready #19659. Please have a review 🙇

[OSBotify]

🚀 Deployed to staging by https://github.com/pecanoro in version: 1.3.19-0 🚀

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

[pecanoro]

@bernhardoj Let's fix both in the same PR, but let's be proactive about it since it will cause a deploy blocker.

[bernhardoj]

Added tooltip wrong position fix in the PR. Here is what the PR includes:

1. Emoji picker wrong position
2. Tooltip wrong position
3. Added shiftVertical back

[OSBotify]

🚀 Deployed to production by https://github.com/luacmartins in version: 1.3.19-7 🚀

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