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.

Sign up for GitHub

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

Jump to bottom

Basic Touch Events #123

Merged
merged 9 commits into from
Sep 30, 2021
Merged

Basic Touch Events #123

merged 9 commits into from
Sep 30, 2021

Conversation

bign8
Copy link
Contributor

@bign8 bign8 commented Oct 2, 2020

👋 Hey Jupyterlab,

I recently started developing in Jupyter Notebooks on a tablet and was running into issues getting panels to move around consistently. After digging through the Jupyter source I found it used this project for UI interactions as such. I’m not an expert on TypeScript or anything, but I figured I could give it a shot. I spent a few evenings trying to get everything working and eventually stumbled across this solution. I know its not the best, but by converting touch events to mouse events reduces the amount of logic that needed to be re-written on the base event handlers. Additionally, I know making it public isn’t great, but given the current pakage structure and layout, it seemed sensible. I verified the tests on my machine with Firefox and everything passes. I know this doesn’t resolve all the issues with #77, but I figured its a start.

Anyway, hope this patch finds the maintainers well, and look forward to hearing back.

Cheers 🍻

telamonian
telamonian reviewed Dec 16, 2020
View reviewed changes
Copy link
Member

@telamonian telamonian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bign8 This is neat! Thanks for the hard work. Overall, the code looks good. There's probably a couple of places where it could be cleaned up, but before that I think we need to consider a few big-picture questions:

  1. Is manually converting touch events to mouse events in our application code the right approach? According to MDN, browsers are supposed to automatically handle this sort of thing by emitting a corresponding mouse event for every touch event.

  2. Is this the right approach from an accessibility perspective? We'll have to consult the experts.

  3. Given that this is the right approach overall, instead of implementing the touch -> mouse conversion in a bunch of separate widgets, should we instead implement the conversion "globally" in Application, similar to how the context menu is implemented?

    lumino/packages/application/src/index.ts

    Lines 469 to 473 in b268386

    protected addEventListeners(): void {
    document.addEventListener('contextmenu', this);
    document.addEventListener('keydown', this, true);
    window.addEventListener('resize', this);
    }

@bign8 From your perspective, what exactly isn't working currently with our mobile UX, and what exactly does the code in this PR fix? Can you please add some specific before/after examples?

@jptio I know you've been a bunch of work to improve mobile UX. What's your take on the approach to handling touch events in this PR?

@manfromjupyter In general, is converting touch -> mouse events reasonable w.r.t. accessibility?

@manfromjupyter
Copy link

@telamonian,

From an accessibility perspective I don't see a problem with it. I'd say as long as we test that gestures work and we can check that focusing on an element with a single click and hold causes the focus styling of a given element to appear, it'd say it's safe for accessibility users.

Gestures to test (for general users + Low Vision users):

  • Try double tapping - make sure it zooms.
  • Try dragging with one finger while zoomed in - should allow you to move along surface without breaking contact
  • Try pinch/spreading by touching surface with two fingers to move in (pinch) or out (spread). Should allow you to control how much you zoom in or out.
  • Press - touch surface and hold to make sure you can gain focus on an element. Not sure what element to test to see if CSS styling for when focus appears but click around and find out something.
  • Flick quickly to the left to see if it sends you back to the previous thing in your browsing history.

Side note for the rest: Once we get low vision support (ability to magnify at will without the app breaking) it will also support mobile and tablet users because the viewport scaling will be the same and a lot of this moving of panels around won't be as big of a problem because nothing will be cut off or falling off the side of the page. I can still see moving notebook tabs around though and until we get low vision support might be quite necessary. Just throwing it out there.

@isabela-pf isabela-pf mentioned this pull request Dec 18, 2020
Closed
@bign8
Copy link
Contributor Author

bign8 commented Dec 24, 2020 •
edited
Loading

Hey @telamonian 👋,

Apologies for the delay, but I wanted a chance to pull latest master to see if other changes fixed the issues I was seeing. After verifying with a build of the examples from latest master, it appears my issue has since been fixed, so feel free to close this PR.

As for the original issue, I was unable to "click-and-drag" tabs in Chrome on my iPad (touch based input). An explicit example would be loading up the examples/example-docpanel on a touch based tablet device, clicking any of the tab pane titles, and dragging them to a new position, causing a tab re-order, move to other window-pane or docking to another set of tabs. Before (if I remember correctly), the touch events seemed not to properly trigger the "drag", so the highlighted panel would activate, but none of the drag events would be handled and the panel would stay in its original location.

Thank you for your responses, and looking forward to seeing these updates consumed in jupyterlab soon (still unable to drag tabs in a new notebook 😢)!

EDIT: Looking further, it appears that jupyterlab is consuming the latest dragdrop and widgets packages. So maybe there is still a bug in this interaction; I'll keep poking around to see what I can find.

EDIT2: Looks like most of my issues are documented here: jupyterlab/jupyterlab#3275

EDIT3: Was able to re-produce on my desktop using Chrome dev tools, below you'll find the video

basic-touch-events.mov

@afshin
Copy link
Member

afshin commented Sep 21, 2021

I don't have the bandwidth to review this. In previous conversations whenever this has come up, we've shied away from using TouchEvent in favor of using PointerEvent. This was not a specification that was supported widely enough when the initial implementation was written, but it is a level of abstraction that is likelier to survive longer. Given the position in a UI's stack where Lumino is used, opting for the more expansive abstraction layer is probably a healthier long-term strategy.
(cf. Using Pointer Events — MDN and Pointer Events — javascript.info)

I apologize this PR has been open for so long without adequate attention.

@bign8, thank you so much for putting time and careful consideration into this. I'd love to hear your thoughts about switching over to PointerEvent support.

Additionally, if you have the time and inclination, please consider coming to tomorrow's (Wednesday, 22 September 2021) JupyterLab weekly call to pair up with someone who is able to dedicate some time to reviewing this and working with you so that we can move forward. The calls take place every Wednesday (and we post the meeting minutes each week), if you can't make this next one:

Weekly Developer Meetings take place on zoom, on Wednesdays at 9AM Pacific Time (your time).

@afshin afshin removed their request for review September 22, 2021 20:29
@bign8
Merge branch 'master' of github.com:jupyterlab/lumino into touch-events
df3b286 
Conflicts:
	packages/dragdrop/src/index.ts
	packages/dragdrop/tests/src/index.spec.ts
	packages/widgets/src/dockpanel.ts
	packages/widgets/src/tabbar.ts
@bign8
remove eslint exceptions
2432f56
@bign8
Use PointerEvents instead of TouchEvents
5135519 
Recommended by: @afshin on #123

Found native browser drag/drop inadvertently firing the pointercancel event.
Disabled with css `pointer-events: none` on tabs.

Details: https://javascript.info/pointer-events#event-pointercancel
@bign8
Restoring MouseEvents (marked as deprecated)
3754c7b 
The simulate-event package does not support pointer events.

Looks to be reported here: blakeembrey/simulate-event#3
@blink1073
Copy link
Contributor

Manually creating a binder link for testing:

To try out this branch on binder, follow this link: Binder

@blink1073 blink1073 added the enhancement New feature or request label Sep 29, 2021
@blink1073
Copy link
Contributor

Kicking CI to pick up #241

@blink1073 blink1073 closed this Sep 30, 2021
@blink1073 blink1073 reopened this Sep 30, 2021
@blink1073
Copy link
Contributor

Kicking again

@blink1073 blink1073 closed this Sep 30, 2021
@blink1073 blink1073 reopened this Sep 30, 2021
blink1073
blink1073 approved these changes Sep 30, 2021
View reviewed changes
Copy link
Contributor

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @bign8! I confirmed the fix on my tablet.

@blink1073 blink1073 merged commit e6612f6 into jupyterlab:master Sep 30, 2021
@welcome
Copy link

welcome bot commented Sep 30, 2021

Congrats on your first merged pull request in this project! 🎉
congrats
Thank you for contributing, we are very proud of you! ❤️

@bign8 bign8 deleted the touch-events branch October 1, 2021 00:04
@martaszmit martaszmit mentioned this pull request Oct 21, 2021
Open
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@telamonian telamonian telamonian left review comments

@blink1073 blink1073 blink1073 approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@bign8 @manfromjupyter @afshin @blink1073 @telamonian