-
-
Notifications
You must be signed in to change notification settings - Fork 32.2k
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
[Tooltip] disableTouchListener not working #20245
Comments
@devhyunjae Thanks for the bug report. I can reproduce the problem. What do you think of this patch? diff --git a/packages/material-ui/src/Tooltip/Tooltip.js b/packages/material-ui/src/Tooltip/Tooltip.js
index 1d8154194..c16c11122 100644
--- a/packages/material-ui/src/Tooltip/Tooltip.js
+++ b/packages/material-ui/src/Tooltip/Tooltip.js
@@ -388,13 +388,16 @@ const Tooltip = React.forwardRef(function Tooltip(props, ref) {
}, leaveDelay);
};
- const handleTouchStart = (event) => {
+ const detectTouchStart = (event) => {
ignoreNonTouchEvents.current = true;
const childrenProps = children.props;
-
if (childrenProps.onTouchStart) {
childrenProps.onTouchStart(event);
}
+ }
+
+ const handleTouchStart = (event) => {
+ detectTouchStart(event);
clearTimeout(leaveTimer.current);
clearTimeout(closeTimer.current);
@@ -447,6 +450,7 @@ const Tooltip = React.forwardRef(function Tooltip(props, ref) {
...other,
...children.props,
className: clsx(other.className, children.props.className),
+ onTouchStart: detectTouchStart,
};
if (!disableTouchListener) { Do you want to submit a pull request? We would also need a new test case :). |
@oliviertassinari Nah i don't want to make more mess in here 😅Up to you mate. |
@oliviertassinari I can make a pull request for this. I just need to figure out how to make a test. |
@oliviertassinari I was working on this issue but I can't break my test diff --git a/packages/material-ui/src/Tooltip/Tooltip.test.js b/packages/material-ui/src/Tooltip/Tooltip.test.js
index f55e60ace..829dbd80c 100644
--- a/packages/material-ui/src/Tooltip/Tooltip.test.js
+++ b/packages/material-ui/src/Tooltip/Tooltip.test.js
@@ -175,6 +175,15 @@ describe('<Tooltip />', () => {
wrapper.update();
assert.strictEqual(wrapper.find(Popper).props().open, false);
});
+
+ it('should not respond with disableTouchListener', () => {
+ const wrapper = mount(<Tooltip {...defaultProps} disableTouchListener />);
+ const children = wrapper.find('#testChild');
+ children.simulate('touchStart');
+ clock.tick(1000);
+ wrapper.update();
+ assert.strictEqual(wrapper.find('[role="tooltip"]').exists(), false);
+ });
});
describe('mount', () => { |
@weslenng Start by replacing enzyme for testing-library. |
I made the test below, but the Tooltip keeps not rendering diff --git a/packages/material-ui/src/Tooltip/Tooltip.test.js b/packages/material-ui/src/Tooltip/Tooltip.test.js
index f55e60ace..94cd05c45 100644
--- a/packages/material-ui/src/Tooltip/Tooltip.test.js
+++ b/packages/material-ui/src/Tooltip/Tooltip.test.js
@@ -175,6 +175,21 @@ describe('<Tooltip />', () => {
wrapper.update();
assert.strictEqual(wrapper.find(Popper).props().open, false);
});
+
+ it('should not respond with disableTouchListener', () => {
+ const { baseElement } = render(
+ <Tooltip
+ {...defaultProps}
+ disableTouchListener
+ />,
+ );
+ const children = baseElement.querySelector("#testChild");
+ const firstTouch = new Touch({ target: children });
+ fireEvent.touchStart(children, { touches: [firstTouch] });
+ clock.tick(1500);
+ fireEvent.touchEnd(children, { touches: [firstTouch] });
+ expect(baseElement.querySelector('[role="tooltip"]')).to.equal(null);
+ });
});
describe('mount', () => { I tried to dispatch the touch event manually in the given reproduction and the behavior as the same For reference, the code who I used is here Note if I remove the disableTouchListener, the Tooltip renders normally, in both test and reproduction What do you think? |
@weslenng I think that we should reproduce the sequence of events that causes the problem, I suspect it's the focus event that makes the tooltip visible, but I'm not sure, need to console the triggered events. |
@devhyunjae What was your use case for disabling the tooltip on touch devices? |
Current Behavior 😯
demo:
Expected Behavior 🤔
given: Tooltip comp with disableTouchListener prop true
when: User touches button
then: Tooltip should not show
Steps to Reproduce 🕹
https://codesandbox.io/s/material-demo-seecb
The text was updated successfully, but these errors were encountered: