fix: tooltips not working as expected by pac-guerreiro · Pull Request #3765 · callstack/react-native-paper

pac-guerreiro · 2023-03-20T11:05:11Z

Summary

This PR fixes issues related to the tooltip component not working correctly on web and native, as reported on #3729.

The changes are as follows:

Fixed console warnings when using tooltip component with components that didn't forward refs. I introduced changes to how the tooltip component handles hovering/pressing events, so now there's no need to pass refs to wrapped components.
Fixed tooltip not working with non-pressable components, like avatar text, on both web and native.
Fixed tooltip not working with FAB component. First part of the problem was that FAB didn't use handlers for hover/press events, so the tooltip would not be triggered to show. Second part of the problem was that when using tooltip with a component that has a position: 'absolute' the tooltip isn't placed correctly on the screen. The solution was to take the component styling into account when calculating the right placement for the tooltip.
Added reproducibility sample from Not all componetns can receive ref #3729 to Tooltips screen/page on the example app.
Changed how tooltips are triggered:
- on web, the tooltip is triggered if the user mouse pointer hovers the wrapped component;
- on native, the tooltip is triggered by long pressing the wrapped component.

Test plan

These changes can be tested on the Tooltips screen/page of the example app.

Screen recordings

Web:

Screen.Recording.2023-03-20.at.10.29.44.mov

Native:

Screen.Recording.2023-03-20.at.10.34.55.mov

github-actions · 2023-03-20T11:11:19Z

The mobile version of example app from this branch is ready! You can see it here.

src/components/Appbar/AppbarHeader.tsx

src/react-navigation/useInternalNavigation.tsx

tjaniczek · 2023-03-22T10:02:34Z

Any chance we can implement immediate tooltip hiding on something like onMouseLeave on web, to avoid this situation?

pac-guerreiro · 2023-04-01T14:12:42Z

Any chance we can implement immediate tooltip hiding on something like onMouseLeave on web, to avoid this situation?

There's already a 'leaveTouchDelay' prop but it has a default value of 1500. I can change the value to 0 when Platform.OS === 'web', what do you think?

DimitarNestorov · 2023-04-01T16:41:16Z

There's already a 'leaveTouchDelay' prop but it has a default value of 1500. I can change the value to 0 when Platform.OS === 'web', what do you think?

Platform.OS === 'web' doesn't necessarily mean that the user has a mouse pointer, you could be browsing on your smartphone. Platform.OS === 'ios' || Platform.OS === 'android' also doesn't necessarily mean touch, you may have a mouse connected. Both hover and touch tooltips should be available on all platforms. I say we don't modify the current behaviour but definitely change this with Paper 6.0.
Pressable has onHoverIn and onHoverOut since RN 0.68.
Pointer events are available in the new architecture since RN 0.71.
leaveTouchDelay is a bad name for the prop. Maybe we can rename it to delayPressOut (inspired by TouchableWithoutHighlight which has had a prop with the same name since RN 0.60 at least).
And also a separate delayHoverOut/delayPointerLeave would be nice, that one can be 0 by default.

DimitarNestorov · 2023-04-01T17:12:52Z

src/components/Tooltip/Tooltip.tsx

        {React.cloneElement(children, {
          ...rest,
-          ref: childrenWrapperRef,
          ...(isWeb ? webPressProps : mobilePressProps),
        })}


- {React.cloneElement(children, { - ...rest, - ...(isWeb ? webPressProps : mobilePressProps), - })} + {children}

If the container has the press handlers the child doesn't need them.

In case the child is a button, it is needed, otherwise the tooltip wont be triggered

pac-guerreiro · 2023-04-03T10:00:05Z

There's already a 'leaveTouchDelay' prop but it has a default value of 1500. I can change the value to 0 when Platform.OS === 'web', what do you think?

Platform.OS === 'web' doesn't necessarily mean that the user has a mouse pointer, you could be browsing on your smartphone. Platform.OS === 'ios' || Platform.OS === 'android' also doesn't necessarily mean touch, you may have a mouse connected. Both hover and touch tooltips should be available on all platforms. I say we don't modify the current behaviour but definitely change this with Paper 6.0. Pressable has onHoverIn and onHoverOut since RN 0.68. Pointer events are available in the new architecture since RN 0.71. leaveTouchDelay is a bad name for the prop. Maybe we can rename it to delayPressOut (inspired by TouchableWithoutHighlight which has had a prop with the same name since RN 0.60 at least). And also a separate delayHoverOut/delayPointerLeave would be nice, that one can be 0 by default.

Wow these are very good valid points! Thank you for mentioning these @DimitarNestorov!

I can apply these changes you suggested 😄

callstack-bot · 2023-05-10T09:49:36Z

Hey @pac-guerreiro, thank you for your pull request 🤗. The documentation from this branch can be viewed here.

tjaniczek

Please keep the Avatar component in the example page, since this was the core issue reported: #3729

example/src/Examples/TooltipExample.tsx

tjaniczek · 2023-06-05T08:02:52Z

Closes #3729

brunohkbx · 2023-06-05T13:31:37Z

src/react-navigation/types.tsx

  >
 >;
+
+export type NavigationParams = {


✂️ as @lukewalczak pointed out previously there's no need to touch react navigation.

brunohkbx · 2023-06-05T13:32:00Z

testSetup.js

+jest.mock('@react-navigation/native', () => ({
+  ...jest.requireActual('@react-navigation/native'),
+  useRoute: () => ({}),
+  useNavigation: () => ({ setParams: jest.fn() }),
+}));


✂️ same

Suggested change

jest.mock('@react-navigation/native', () => ({

...jest.requireActual('@react-navigation/native'),

useRoute: () => ({}),

useNavigation: () => ({ setParams: jest.fn() }),

}));

brunohkbx · 2023-06-05T13:35:20Z

src/components/__tests__/Tooltip.test.tsx

-  describe('Unmount', () => {
-    beforeAll(() => jest.spyOn(global, 'clearTimeout'));
-    afterEach(() => jest.clearAllMocks());
+  describe('Mobile', () => {


What do you think about splitting this file in two since there's a lot here already? One test file for mobile and another for web?
Also, can we have a test where the children component has absolute position?

Yeah absolute positioning is still a bit wonky at the moment. I think in the interest of time we'll merge this piece now and create an issue to rework the positioning. We can address the tests files as well there.

brunohkbx · 2023-06-05T13:36:50Z

src/components/FAB/FAB.tsx

          accessibilityState={newAccessibilityState}
          testID={testID}
          style={{ borderRadius }}
+          {...rest}


Do we need this change?

@pac-guerreiro remember why it was added there?

Looks like without added {...rest}, Tooltip is not displayed over the FAB on the web.

brunohkbx · 2023-06-05T13:53:47Z

src/components/Tooltip/Tooltip.tsx


-    showTooltipTimer.current = setTimeout(() => {
+    if (isWeb) {
+      showTooltipTimer.current = setTimeout(() => {


What if I want to have an enterTouchDelay on mobile? Since the base component is a Pressable now, would it be possible to replace this prop enterTouchDelay with delayLongPress?

Changing prop would be a breaking change and require releasing new version. enterTouchDelay is translated to delayLongPress on mobile:

react-native-paper/src/components/Tooltip/Tooltip.tsx

Line 161 in 39f257b

delayLongPress: enterTouchDelay,

brunohkbx · 2023-06-05T13:55:24Z

src/components/Tooltip/utils.ts

  childrenHeight: number,
-  tooltipHeight: number
+  tooltipHeight: number,
+  headerHeight: number = 0


This one isn't being used

Suggested change

headerHeight: number = 0

pac-guerreiro added bug example app Tooltip labels Mar 20, 2023

pac-guerreiro requested review from brunohkbx, lukewalczak, satya164 and tjaniczek March 20, 2023 11:05

lukewalczak changed the title ~~Fix/tooltips not working as expected~~ fix: tooltips not working as expected Mar 20, 2023

lukewalczak reviewed Mar 20, 2023

View reviewed changes

src/components/Appbar/AppbarHeader.tsx Outdated Show resolved Hide resolved

src/react-navigation/useInternalNavigation.tsx Outdated Show resolved Hide resolved

DimitarNestorov requested changes Apr 1, 2023

View reviewed changes

pac-guerreiro force-pushed the fix/tooltips-not-working-as-expected branch from 4e7faa0 to ce32750 Compare May 12, 2023 09:30

pac-guerreiro added 5 commits May 12, 2023 10:33

docs: add avatar tooltip to example app

fac9c4e

fix: tooltip component not working with non pressable components

8f1e990

fix: tooltip not working with floating action button

122de5d

fix: tooltip positioning for components using position absolute

46c808d

chore: update tooltip tests for native and web

92b5b3a

pac-guerreiro force-pushed the fix/tooltips-not-working-as-expected branch from ce32750 to 92b5b3a Compare May 12, 2023 09:37

teneeto added 2 commits May 17, 2023 10:06

fix: remove navigation implemention

158c77c

fix: temp fix for FAB not visibly positioned with tooltip

d38bd04

tjaniczek requested changes May 31, 2023

View reviewed changes

example/src/Examples/TooltipExample.tsx Outdated Show resolved Hide resolved

fix: add avatar to Examples page

a1c30a6

tjaniczek approved these changes Jun 5, 2023

View reviewed changes

DimitarNestorov approved these changes Jun 5, 2023

View reviewed changes

brunohkbx reviewed Jun 5, 2023

View reviewed changes

fix: remove unused code

39f257b

lukewalczak added this to the 5.9.0 milestone Jun 16, 2023

lukewalczak merged commit c724ad9 into main Jun 26, 2023

lukewalczak deleted the fix/tooltips-not-working-as-expected branch June 26, 2023 11:23

Uh oh!

Conversation

pac-guerreiro commented Mar 20, 2023 • edited by DimitarNestorov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Uh oh!

github-actions bot commented Mar 20, 2023

Uh oh!

Uh oh!

Uh oh!

tjaniczek commented Mar 22, 2023

Uh oh!

pac-guerreiro commented Apr 1, 2023

Uh oh!

DimitarNestorov commented Apr 1, 2023

Uh oh!

DimitarNestorov Apr 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pac-guerreiro commented Apr 3, 2023

Uh oh!

callstack-bot commented May 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tjaniczek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tjaniczek commented Jun 5, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

pac-guerreiro commented Mar 20, 2023 •

edited by DimitarNestorov

Loading

DimitarNestorov Apr 1, 2023 •

edited

Loading

callstack-bot commented May 10, 2023 •

edited

Loading