-
-
Notifications
You must be signed in to change notification settings - Fork 127
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
Basic Touch Events #123
Basic Touch Events #123
Conversation
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.
@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:
-
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.
-
Is this the right approach from an accessibility perspective? We'll have to consult the experts.
-
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?
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):
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. |
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 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 |
I don't have the bandwidth to review this. In previous conversations whenever this has come up, we've shied away from using 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 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:
|
Conflicts: packages/dragdrop/src/index.ts packages/dragdrop/tests/src/index.spec.ts packages/widgets/src/dockpanel.ts packages/widgets/src/tabbar.ts
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
The simulate-event package does not support pointer events. Looks to be reported here: blakeembrey/simulate-event#3
Manually creating a binder link for testing: To try out this branch on binder, follow this link: |
Kicking CI to pick up #241 |
Kicking again |
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.
Thanks @bign8! I confirmed the fix on my tablet.
👋 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 🍻