-
-
Notifications
You must be signed in to change notification settings - Fork 32.6k
[tooltip] Improve support for disabled button triggers #48622
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
cb7a270
f30d046
5a10fee
f57565b
fe719f8
1ef6079
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| <Tooltip describeChild title="You don't have permission to do this"> | ||
| <span> | ||
| <Button disabled>A Disabled Button</Button> | ||
| </span> | ||
| <Button disabled style={{ pointerEvents: 'auto' }}> | ||
| A Disabled Button | ||
| </Button> | ||
| </Tooltip> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -264,6 +264,7 @@ const Tooltip = React.forwardRef(function Tooltip(inProps, ref) { | |
| const [childNode, setChildNode] = React.useState(); | ||
| const [arrowRef, setArrowRef] = React.useState(null); | ||
| const ignoreNonTouchEvents = React.useRef(false); | ||
| const openedByDisabledTriggerRef = React.useRef(false); | ||
|
|
||
| const disableInteractive = disableInteractiveProp || followCursor; | ||
|
|
||
|
|
@@ -280,34 +281,7 @@ const Tooltip = React.forwardRef(function Tooltip(inProps, ref) { | |
| }); | ||
|
|
||
| let open = openState; | ||
|
|
||
| if (process.env.NODE_ENV !== 'production') { | ||
| // TODO: uncomment once we enable eslint-plugin-react-compiler // eslint-disable-next-line react-compiler/react-compiler | ||
| // eslint-disable-next-line react-hooks/rules-of-hooks -- process.env never changes | ||
| const { current: isControlled } = React.useRef(openProp !== undefined); | ||
|
|
||
| // TODO: uncomment once we enable eslint-plugin-react-compiler // eslint-disable-next-line react-compiler/react-compiler | ||
| // eslint-disable-next-line react-hooks/rules-of-hooks -- process.env never changes | ||
| React.useEffect(() => { | ||
| if ( | ||
| childNode && | ||
| childNode.disabled && | ||
| !isControlled && | ||
| title !== '' && | ||
| childNode.tagName.toLowerCase() === 'button' | ||
| ) { | ||
| console.warn( | ||
| [ | ||
| 'MUI: You are providing a disabled `button` child to the Tooltip component.', | ||
| 'A disabled element does not fire events.', | ||
| "Tooltip needs to listen to the child element's events to display the title.", | ||
| '', | ||
| 'Add a simple wrapper element, such as a `span`.', | ||
| ].join('\n'), | ||
| ); | ||
| } | ||
| }, [title, childNode, isControlled]); | ||
| } | ||
| const { current: isControlled } = React.useRef(openProp !== undefined); | ||
|
|
||
| const id = useId(idProp); | ||
|
|
||
|
|
@@ -341,6 +315,7 @@ const Tooltip = React.forwardRef(function Tooltip(inProps, ref) { | |
| * @param {React.SyntheticEvent | Event} event | ||
| */ | ||
| (event) => { | ||
| openedByDisabledTriggerRef.current = false; | ||
| hystersisTimer.start(800 + leaveDelay, () => { | ||
| hystersisOpen = false; | ||
| }); | ||
|
|
@@ -357,9 +332,6 @@ const Tooltip = React.forwardRef(function Tooltip(inProps, ref) { | |
| ); | ||
|
|
||
| const handleMouseOver = (event) => { | ||
| if (childNode?.disabled) { | ||
| return; | ||
| } | ||
| if (ignoreNonTouchEvents.current && event.type !== 'touchstart') { | ||
| return; | ||
| } | ||
|
|
@@ -382,6 +354,31 @@ const Tooltip = React.forwardRef(function Tooltip(inProps, ref) { | |
| } | ||
| }; | ||
|
|
||
| const handleTriggerMouseOver = (event) => { | ||
| if (childNode?.disabled && !isControlled) { | ||
| // A disabled trigger can open the tooltip if it receives pointer events. | ||
| // However, if the trigger became disabled while the tooltip was already open, | ||
| // stray mouseover events must not cancel the pending close. | ||
|
Comment on lines
+360
to
+361
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added the
This comment was marked as resolved.
Sorry, something went wrong. |
||
| if (open && !openedByDisabledTriggerRef.current) { | ||
| return; | ||
| } | ||
|
|
||
| openedByDisabledTriggerRef.current = true; | ||
| } else { | ||
| openedByDisabledTriggerRef.current = false; | ||
| } | ||
|
|
||
| handleMouseOver(event); | ||
| }; | ||
|
|
||
| const handleInteractiveWrapperMouseOver = (event) => { | ||
| if (childNode?.disabled && !isControlled && !openedByDisabledTriggerRef.current) { | ||
| return; | ||
| } | ||
|
|
||
| handleMouseOver(event); | ||
| }; | ||
|
|
||
| const handleMouseLeave = (event) => { | ||
| enterTimer.clear(); | ||
| leaveTimer.start(leaveDelay, () => { | ||
|
|
@@ -418,6 +415,8 @@ const Tooltip = React.forwardRef(function Tooltip(inProps, ref) { | |
| setChildNode(event.currentTarget); | ||
| } | ||
|
|
||
| openedByDisabledTriggerRef.current = false; | ||
|
|
||
| if (isFocusVisible(event.target)) { | ||
| // Workaround for https://github.com/facebook/react/issues/9142. | ||
| // React does not fire blur when a focused element becomes disabled. | ||
|
|
@@ -455,7 +454,7 @@ const Tooltip = React.forwardRef(function Tooltip(inProps, ref) { | |
|
|
||
| touchTimer.start(enterTouchDelay, () => { | ||
| document.body.style.WebkitUserSelect = prevUserSelect.current; | ||
| handleMouseOver(event); | ||
| handleTriggerMouseOver(event); | ||
| }); | ||
| }; | ||
|
|
||
|
|
@@ -559,11 +558,14 @@ const Tooltip = React.forwardRef(function Tooltip(inProps, ref) { | |
| } | ||
|
|
||
| if (!disableHoverListener) { | ||
| childrenProps.onMouseOver = composeEventHandler(handleMouseOver, childrenProps.onMouseOver); | ||
| childrenProps.onMouseOver = composeEventHandler( | ||
| handleTriggerMouseOver, | ||
| childrenProps.onMouseOver, | ||
| ); | ||
| childrenProps.onMouseLeave = composeEventHandler(handleMouseLeave, childrenProps.onMouseLeave); | ||
|
|
||
| if (!disableInteractive) { | ||
| interactiveWrapperListeners.onMouseOver = handleMouseOver; | ||
| interactiveWrapperListeners.onMouseOver = handleInteractiveWrapperMouseOver; | ||
| interactiveWrapperListeners.onMouseLeave = handleMouseLeave; | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,6 +15,7 @@ import { | |
| } from '@mui/internal-test-utils'; | ||
| import { camelCase } from 'es-toolkit/string'; | ||
| import Tooltip, { tooltipClasses as classes } from '@mui/material/Tooltip'; | ||
| import Button from '@mui/material/Button'; | ||
| import { ThemeProvider, createTheme } from '@mui/material/styles'; | ||
| import { testReset } from './Tooltip'; | ||
| import describeConformance from '../../test/describeConformance'; | ||
|
|
@@ -387,6 +388,77 @@ describe('<Tooltip />', () => { | |
| expect(screen.queryByRole('tooltip')).to.equal(null); | ||
| }); | ||
|
|
||
| it('opens when a disabled native button receives mouse events', async () => { | ||
| clock.restore(); | ||
|
|
||
| const { user } = render( | ||
| <Tooltip title="Hello World" enterDelay={0} slotProps={{ transition: { timeout: 0 } }}> | ||
| <button disabled type="button"> | ||
| Hello World | ||
| </button> | ||
| </Tooltip>, | ||
| ); | ||
|
|
||
| await user.hover(screen.getByRole('button')); | ||
|
|
||
| expect(screen.getByRole('tooltip')).toBeVisible(); | ||
| }); | ||
|
|
||
| it('opens when a disabled Button receives mouse events', async () => { | ||
| clock.restore(); | ||
|
|
||
| const { user } = render( | ||
| <Tooltip title="Hello World" enterDelay={0} slotProps={{ transition: { timeout: 0 } }}> | ||
| <Button disabled style={{ pointerEvents: 'auto' }}> | ||
| Hello World | ||
| </Button> | ||
| </Tooltip>, | ||
| ); | ||
|
|
||
| await user.hover(screen.getByRole('button')); | ||
|
|
||
| expect(screen.getByRole('tooltip')).toBeVisible(); | ||
| }); | ||
|
|
||
| it('keeps a disabled-trigger tooltip open when the tooltip is hovered', async () => { | ||
| clock.restore(); | ||
|
|
||
| const { user } = render( | ||
| <Tooltip | ||
| title="Hello World" | ||
| enterDelay={0} | ||
| leaveDelay={500} | ||
| slotProps={{ transition: { timeout: 0 } }} | ||
| > | ||
| <button disabled type="button"> | ||
| Hello World | ||
| </button> | ||
| </Tooltip>, | ||
| ); | ||
|
|
||
| const button = screen.getByRole('button'); | ||
|
|
||
| await user.hover(button); | ||
| expect(screen.getByRole('tooltip')).toBeVisible(); | ||
|
|
||
| await user.unhover(button); | ||
| await act(async () => { | ||
| await new Promise((resolve) => { | ||
| // Wait a bit but not long enough for the tooltip to disappear | ||
| setTimeout(resolve, 250); | ||
| }); | ||
| }); | ||
| await user.hover(screen.getByRole('tooltip')); | ||
| await act(async () => { | ||
| await new Promise((resolve) => { | ||
| // Wait out the close timeout until the tooltip should have disappeared | ||
| setTimeout(resolve, 300); | ||
| }); | ||
| }); | ||
|
Comment on lines
+444
to
+457
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. shouldn't the flow be: unhover button, wait for some time not enough for the tooltip to disappear, hover the tooltip, then check?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mj12albert Since you wrote this test, would you be able to help with this? Edit: Addressed in |
||
|
|
||
| expect(screen.getByRole('tooltip')).toBeVisible(); | ||
| }); | ||
|
|
||
| it('opens on the next task when reduced motion is always', () => { | ||
| const handleEntered = spy(); | ||
| const theme = createTheme({ | ||
|
|
@@ -794,44 +866,6 @@ describe('<Tooltip />', () => { | |
| }); | ||
| }); | ||
|
|
||
| describe('disabled button warning', () => { | ||
| it('should not raise a warning if title is empty', () => { | ||
| expect(() => { | ||
| render( | ||
| <Tooltip title=""> | ||
| <button type="submit" disabled> | ||
| Hello World | ||
| </button> | ||
| </Tooltip>, | ||
| ); | ||
| }).not.toErrorDev(); | ||
| }); | ||
|
|
||
| it('should raise a warning when we are uncontrolled and can not listen to events', () => { | ||
| expect(() => { | ||
| render( | ||
| <Tooltip title="Hello World"> | ||
| <button type="submit" disabled> | ||
| Hello World | ||
| </button> | ||
| </Tooltip>, | ||
| ); | ||
| }).toWarnDev('MUI: You are providing a disabled `button` child to the Tooltip component'); | ||
| }); | ||
|
|
||
| it('should not raise a warning when we are controlled', () => { | ||
| expect(() => { | ||
| render( | ||
| <Tooltip title="Hello World" open> | ||
| <button type="submit" disabled> | ||
| Hello World | ||
| </button> | ||
| </Tooltip>, | ||
| ); | ||
| }).not.toErrorDev(); | ||
| }); | ||
| }); | ||
|
|
||
| describe('prop: disableInteractive', () => { | ||
| it('when false should keep the overlay open if the popper element is hovered', () => { | ||
| render( | ||
|
|
@@ -1161,9 +1195,9 @@ describe('<Tooltip />', () => { | |
| expect(handleClose.callCount).to.equal(1); | ||
| }); | ||
|
|
||
| it('stays closed when a stray mouseover lands while the disabled child is closing', async () => { | ||
| it('stays closed when a stray mouseover lands while the disabled trigger is closing', async () => { | ||
| // Deterministic regression test for the flaky "stuck open" tooltip: | ||
| // when the focused child becomes disabled the close is scheduled via the React | ||
| // when the focused trigger becomes disabled the close is scheduled via the React | ||
| // #9142 native-blur workaround, but a layout-shift `mouseover` on the interactive | ||
| // popper used to cancel that pending close and reopen the tooltip. A disabled | ||
| // anchor must never (re)open. `leaveDelay` opens a deterministic window in which to | ||
|
|
@@ -1195,11 +1229,11 @@ describe('<Tooltip />', () => { | |
| expect(screen.getByRole('tooltip')).toBeVisible(); | ||
| }); | ||
|
|
||
| // Disabling the focused child schedules the close (leaveDelay window still pending). | ||
| // Disabling the focused trigger schedules the close (leaveDelay window still pending). | ||
| await user.keyboard('{Enter}'); | ||
|
|
||
| // A stray `mouseover` reaches the interactive popper before the close fires. | ||
| fireEvent.mouseOver(screen.getByRole('tooltip')); | ||
| await user.hover(screen.getByRole('tooltip')); | ||
|
|
||
| // The disabled anchor must still close (and not reopen). | ||
| await waitFor(() => { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needed to be removed as well in addition to the warning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! I also found this when testing #48623 (comment)