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

[terminal] Always open terminal links on touch events #6875

Merged
merged 1 commit into from
Jan 16, 2020

Conversation

jankeromnes
Copy link
Member

@jankeromnes jankeromnes commented Jan 13, 2020

What it does

Fixes #6320.

This PR attempts to detect whether a click event on an XtermJS link was from a touch screen (should always open) or not (should only open if Cmd was also pressed).

This is achieved by keeping track of the last touchend event, and comparing times / coordinates.

How to test

  1. yarn
  2. cd examples/browser && yarn start --hostname=0.0.0.0
  3. Open port 3000 in desktop browser: run echo http://perdu.com
    • Simple click on link (should not open)
    • Cmd + click on link (should open)
  4. Open port 3000 on touch device, e.g. iPad: run echo http://perdu.com
    • Tap on link (should open)

Review checklist

Reminder for reviewers

@jankeromnes
Copy link
Member Author

jankeromnes commented Jan 13, 2020

I still need to sign-off & add to changelog, but I'd love an initial review because I feel like my approach is a bit awkward.

@jankeromnes jankeromnes force-pushed the jx/touch-terminal-links branch 2 times, most recently from dbfef2a to 498929a Compare January 13, 2020 18:44
e.g. when tapping a link on iPad.

Signed-off-by: Jan Keromnes <jan.keromnes@typefox.io>
@akosyakov akosyakov requested review from vince-fugnitto, lmcbout and svenefftinge and removed request for vince-fugnitto January 14, 2020 01:47
@akosyakov akosyakov added ipad issues related to iPad terminal issues related to the terminal labels Jan 14, 2020
@akosyakov
Copy link
Member

@vince-fugnitto Could you try it out? i don't have ipad unfortunately

@lmcbout
Copy link
Contributor

lmcbout commented Jan 15, 2020

I cannot test this one, I don't have an IPAD and/or a touch screen to test

@vince-fugnitto
Copy link
Member

@vince-fugnitto Could you try it out? i don't have ipad unfortunately

I forgot to mention, I unfortunately do not have an iPad or any tablet for that matter to properly test out the pull request 😞

@jankeromnes
Copy link
Member Author

jankeromnes commented Jan 15, 2020

I guess there are two main risks here:

  1. Terminal links stop working properly on non-touch devices (e.g. link can't be opened anymore, or link opens without Cmd being pressed)
  2. Terminal links continue not working properly on touch devices (i.e. they still don't work, or they open inadvertently when not wanted)

I believe that 1. can be tested on non-touch devices, and that 2. is not critical (because Terminal links already don't work on touch today), but I can upload a video of me testing 2. on my iPad.

@akosyakov
Copy link
Member

One can emulate with Chrome. I'm going to test like that:
Screen Shot 2020-01-16 at 07 59 40

@akosyakov
Copy link
Member

It works as described, although I wonder how a user can figure out on iPad that a link is tappable? Can we highlight it with underscore and hover as on the desktop? Or you think it would be too much.

@jankeromnes
Copy link
Member Author

jankeromnes commented Jan 16, 2020

Here is a video where I test this PR on my iPad: https://youtu.be/TL5EaIOfUZI

However, please note that the main motivation for this fix was that on iPad (iOS 1.12), even with an external keyboard, you could not open Terminal links at all. But now that iOS 1.13 is out, if you have an external keyboard, Cmd + click works properly now. Thus, this fix is now only relevant for touch devices that don't have an external keyboard (for example smartphones).

It works as described, although I wonder how a user can figure out on iPad that a link is tappable? Can we highlight it with underscore and hover as on the desktop? Or you think it would be too much.

On iPad, the normal XtermJS styling of underlining links will happen when you focus on a link (I don't think there is an actual "hover" state on touch screens, except maybe modern/high-end screens that detect a nearby finger that's not yet in touch contact?):

  • Currently, when you tap on a Terminal link on a touch device to focus it, it will get underlined, and the Cmd + click to follow link pop-up will appear. But the link won't get visited (unless you press Cmd on an external keyboard first).

  • What changes with this PR is that when you tap on a Terminal link, it will now open directly (without the need for pressing Cmd). So I don't think there will be a way to focus the link without visiting (unless you drag your finger elsewhere to discard/cancel the touchend event maybe?)

But if you were talking about passive link styling to reveal what's clickable, I don't think XtermJS allows it, because they render everything via a <canvas> (so no custom CSS rules for links). See also: xtermjs/xterm.js#1540

@akosyakov akosyakov merged commit 8dcd431 into eclipse-theia:master Jan 16, 2020
@akosyakov akosyakov deleted the jx/touch-terminal-links branch January 16, 2020 17:20
@jankeromnes
Copy link
Member Author

Thank you for merging this!

This change is a UX improvement for touch devices without external keyboard (e.g. tablet, smartphone).

However, if it turns out that force-opening Terminal links when touched is not a desirable feature (e.g. if Theia users report issues about it), please feel free to revert my commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ipad issues related to iPad terminal issues related to the terminal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[terminal] Cmd + click doesn't work on iPad (iOS 12)
4 participants