Skip to content
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] Long press select text on iOS #23232

Closed
2 tasks done
benneq opened this issue Oct 24, 2020 · 18 comments · Fixed by #23466
Closed
2 tasks done

[Tooltip] Long press select text on iOS #23232

benneq opened this issue Oct 24, 2020 · 18 comments · Fixed by #23466
Labels
bug 🐛 Something doesn't work component: tooltip This is the name of the generic UI component, not the React module! good first issue Great for first contributions. Enable to learn the contribution process.

Comments

@benneq
Copy link
Contributor

benneq commented Oct 24, 2020

  • The issue is present in the latest release.
  • I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

When opening an IconButton Tooltip on mobile (tested iPhone with iOS 14), it will show the Tooltip, but also show the "copy, define, share..." callout menu and selects some elements.

Expected Behavior 🤔

It should only show the tooltip.

Steps to Reproduce 🕹

Code Sandbox: https://codesandbox.io/s/material-ui-issue-forked-mqyl2?file=/src/Demo.js

Steps:

  1. Open Code Sandbox link on mobile (iPhone iOS 14 tested)
  2. Long press the IconButton
  3. It will show the Tooltip
  4. But it will also open callout menu and select some random elements

Ohne Titel

Context 🔦

See above

Your Environment 🌎

Tech Version
Material-UI v5.0.0-alpha.14
React 17.0.1
Browser Safari on iOS 14
TypeScript 4.1.0-beta
etc. ---

EDIT: I know this happens basically for all buttons. But for normal buttons there's no reason to long press them, though it's not a real issue there.

@benneq benneq added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Oct 24, 2020
@oliviertassinari oliviertassinari added component: tooltip This is the name of the generic UI component, not the React module! and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Oct 24, 2020
@oliviertassinari
Copy link
Member

@benneq Thanks for raising. The first thing that I could help myself noticed is how ugly it looks when the tooltip touch the edge. I think that we should add a bit of margin to avoid it:

Before
Capture d’écran 2020-10-24 à 18 43 43

After
Capture d’écran 2020-10-24 à 18 42 51

diff --git a/packages/material-ui/src/Tooltip/Tooltip.js b/packages/material-ui/src/Tooltip/Tooltip.js
index 160c607bd3..8d9375ab12 100644
--- a/packages/material-ui/src/Tooltip/Tooltip.js
+++ b/packages/material-ui/src/Tooltip/Tooltip.js
@@ -114,33 +114,33 @@ export const styles = (theme) => ({
   /* Styles applied to the tooltip (label wrapper) element if `placement` contains "left". */
   tooltipPlacementLeft: {
     transformOrigin: 'right center',
-    margin: '0 24px ',
+    margin: '2px 24px ',
     [theme.breakpoints.up('sm')]: {
-      margin: '0 14px',
+      margin: '2px 14px',
     },
   },
   /* Styles applied to the tooltip (label wrapper) element if `placement` contains "right". */
   tooltipPlacementRight: {
     transformOrigin: 'left center',
-    margin: '0 24px',
+    margin: '2px 24px',
     [theme.breakpoints.up('sm')]: {
-      margin: '0 14px',
+      margin: '2px 14px',
     },
   },
   /* Styles applied to the tooltip (label wrapper) element if `placement` contains "top". */
   tooltipPlacementTop: {
     transformOrigin: 'center bottom',
-    margin: '24px 0',
+    margin: '24px 2px',
     [theme.breakpoints.up('sm')]: {
-      margin: '14px 0',
+      margin: '14px 2px',
     },
   },
   /* Styles applied to the tooltip (label wrapper) element if `placement` contains "bottom". */
   tooltipPlacementBottom: {
     transformOrigin: 'center top',
-    margin: '24px 0',
+    margin: '24px 2px',
     [theme.breakpoints.up('sm')]: {
-      margin: '14px 0',
+      margin: '14px 2px',
     },
   },
 });

Do you want to work on the problem with a pull request :)?


Regarding the text selection, it seems that the only solution is to set the whole page not selectable, see https://www.reddit.com/r/webdev/comments/g1wvsb/ios_safari_how_to_disable_longpress_text_selection/.

@oliviertassinari oliviertassinari added the good first issue Great for first contributions. Enable to learn the contribution process. label Oct 24, 2020
@oliviertassinari oliviertassinari changed the title Tooltip IconButton on mobile iOS opens callout menu and selects text [Tooltip] Long press select text on iOS Oct 24, 2020
@benneq
Copy link
Contributor Author

benneq commented Oct 24, 2020

Regarding the text selection, it seems that the only solution is to set the whole page not selectable, see https://www.reddit.com/r/webdev/comments/g1wvsb/ios_safari_how_to_disable_longpress_text_selection/.

Yeah, Apple browsers.. 😞

Do you want to work on the problem with a pull request :)?

What exactly do you mean? The unfixable Long Press Text Selection? Or you think it's possible to dynamically add user-select: none to body when Tooltip is activated?


Btw: I just noticed a Tooltip regression, that works fine with material-ui 4.x:

When you hover the IconButton on desktop, there's a gap between Button and Tooltip, which is completely fine. But in version 5.x the tooltip won't disappear when you move the mouse cursor between the Button and the Tooltip.

Here's the v4 version: https://material-ui.com/components/tooltips/

And here's v5: https://next.material-ui.com/components/tooltips/

That's pretty annoying when you have a Table with a button in each row, because when you move the mouse downwards, you can't focus the next button.

Bildschirmfoto 2020-10-24 um 19 05 05

@oliviertassinari
Copy link
Member

That's pretty annoying when you have a Table with a button in each row, because when you move the mouse downwards, you can't focus the next button.

@benneq This is part of a tradeoff we took with @eps1lon in #22382. You have to set disableInteractive. We document it in the breaking change list.

@benneq
Copy link
Contributor Author

benneq commented Oct 24, 2020

@oliviertassinari Didn't notice this new prop. Thanks. That behavior annoyed me all day. Especially because it extends even below the Tooltip. If you move the mouse left and right over the Tooltip it instantly disappears. But you can move the cursor faaar below the Tooltip and it stays open. But that's a different topic.

BTT: What about the pull request and user-select: none on body when interacting with Tooltip?

@oliviertassinari
Copy link
Member

BTT: What about the pull request and user-select: none on body when interacting with Tooltip?

Does it work? I couldn't make it work.

@benneq
Copy link
Contributor Author

benneq commented Oct 24, 2020

Haven't tried yet. I'll give it a shot in the following days. But I know it works when adding user-select: none in index.css. But this of course will disable selection for the whole app.

EDIT: found this https://stackoverflow.com/questions/61855027/prevent-text-selection-on-tap-and-hold-on-ios-13-mobile-safari

@oliviertassinari
Copy link
Member

This seems to work:

diff --git a/packages/material-ui/src/Tooltip/Tooltip.js b/packages/material-ui/src/Tooltip/Tooltip.js
index 160c607bd3..1153ca4000 100644
--- a/packages/material-ui/src/Tooltip/Tooltip.js
+++ b/packages/material-ui/src/Tooltip/Tooltip.js
@@ -281,6 +281,7 @@ const Tooltip = React.forwardRef(function Tooltip(props, ref) {
       clearTimeout(closeTimer.current);
       closeTimer.current = setTimeout(() => {
         ignoreNonTouchEvents.current = false;
+        document.body.style.WebkitUserSelect = null;
       }, theme.transitions.duration.shortest);
     },
   );
@@ -355,6 +356,8 @@ const Tooltip = React.forwardRef(function Tooltip(props, ref) {

   const detectTouchStart = (event) => {
     ignoreNonTouchEvents.current = true;
+    // Prevent iOS text selection on long-tap.
+    document.body.style.WebkitUserSelect = 'none';

     const childrenProps = children.props;
     if (childrenProps.onTouchStart) {
@@ -407,6 +410,7 @@ const Tooltip = React.forwardRef(function Tooltip(props, ref) {
     document.addEventListener('keydown', handleKeyDown);

     return () => {
+      document.body.style.WebkitUserSelect = null;
       document.removeEventListener('keydown', handleKeyDown);
     };
   }, [handleClose, open]);

@oliviertassinari oliviertassinari added the bug 🐛 Something doesn't work label Oct 24, 2020
@benneq
Copy link
Contributor Author

benneq commented Oct 24, 2020

This seems to work:

Yes! That does the trick 😸

Maybe save the old value of document.body.style.WebkitUserSelect and reapply it afterwards, in case someone wants app wide disabled user-select in his index.css.

@hmaddisb
Copy link
Contributor

hmaddisb commented Nov 3, 2020

@benneq @oliviertassinari hi, can I work on this? and should i go with saving old value of WebkitUserSelect and reapply it (#23232 (comment)) or just go with null?

@oliviertassinari
Copy link
Member

@hmaddisb Yes, sure :)

@hmaddisb
Copy link
Contributor

hmaddisb commented Nov 3, 2020

@oliviertassinari nice! is it best if I go with saving and restoring the old WebkitUserSelect value or just null? maybe restoring it is the best way to go?

@oliviertassinari
Copy link
Member

@hmaddisb We would need to restore the previous value. I have seen developers disable user selection globally on applications.

@hmaddisb
Copy link
Contributor

hmaddisb commented Nov 4, 2020

@oliviertassinari seems like a good idea, do we think the previous should be stored as a state?

@oliviertassinari
Copy link
Member

@hmaddisb A ref, not a state because the value isn't used in the render.

@hmaddisb
Copy link
Contributor

hmaddisb commented Nov 4, 2020

@oliviertassinari yes of course

@eps1lon
Copy link
Member

eps1lon commented Nov 4, 2020

I'm running some experiments with preventDefault which is the diomatic way to stop some native behavior. There are just some quirks with the event timing in React though. Might better than this fairly invasive body style.

Edit: Forgot touchstart was passive by default and impacts scrolling.

@sandbox4013
Copy link

Could you explain how it works?
document.body.style.WebkitUserSelect = 'none' doesn't apply to body
Checked on tooltip' example
Ipad Pro 9.7 xcode simulator
image

@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 23, 2021

@sandbox4013 This fix is only available in v5, your link points to v4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: tooltip This is the name of the generic UI component, not the React module! good first issue Great for first contributions. Enable to learn the contribution process.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants