Skip to content

Add support for touch events to jQuery UI - #7525

Closed
danxuliu wants to merge 2 commits into
masterfrom
add-support-for-touch-events-to-jquery-ui
Closed

Add support for touch events to jQuery UI#7525
danxuliu wants to merge 2 commits into
masterfrom
add-support-for-touch-events-to-jquery-ui

Conversation

@danxuliu

Copy link
Copy Markdown
Member

Although jQuery UI is expected to be removed in the future this will not happen before Nextcloud 13. jQuery UI does not support touch events out of the box, so the features that depend on jQuery UI, like dragging files in the Files app, are not available on touch screens.

jQuery UI Touch Punch adds support to touch events by mapping them to their mouse counterparts, takes just 1.3KiB minified, and requires no changes to the existing code; it is the recommended way to add support for touch events by the project lead of jQuery UI.

It is dual licensed under the MIT license and the GNU GPL v2, so we can use it under the MIT license (as GNU GPL v2 only, without the or later part, is not compatible with the GNU AGPL V3+ if I recall correctly).

Unfortunately the latest version is a few years old, and probably it will not be able to handle every situation, but it is better anyway than what we have now which is no support at all ;-)

Finally, should this be backported to Nextcloud 12?

@danxuliudanxuliu added 3. to review Waiting for reviews bug labels Dec 15, 2017
@danxuliudanxuliu added this to the Nextcloud 13 milestone Dec 15, 2017
@codecov

codecovBot commented Dec 15, 2017

Copy link
Copy Markdown

Codecov Report

Merging #7525 into master will decrease coverage by <.01%.
The diff coverage is n/a.

@@ Coverage Diff @@## master #7525 +/- ##
============================================
- Coverage 51.17% 51.16% -0.01% + Complexity 24885 24865 -20 
============================================
Files 1601 1601 Lines 94746 94683 -63 Branches 1368 1368 ============================================
- Hits 48486 48446 -40 + Misses 46260 46237 -23
Impacted FilesCoverage ΔComplexity Δ
...private/Collaboration/Collaborators/MailPlugin.php58.75% <0%> (-14.08%)19% <0%> (-4%)
lib/private/Security/CertificateManager.php91.08% <0%> (-1%)39% <0%> (ø)
core/Controller/LoginController.php77.85% <0%> (-0.87%)38% <0%> (-1%)
lib/private/Installer.php57.64% <0%> (-0.53%)74% <0%> (-3%)
lib/private/Server.php81.01% <0%> (-0.43%)134% <0%> (ø)
apps/files_trashbin/lib/Trashbin.php72.28% <0%> (-0.25%)136% <0%> (ø)
lib/private/AppFramework/Http/Request.php92.68% <0%> (-0.03%)148% <0%> (-1%)
lib/private/Console/TimestampFormatter.php0% <0%> (ø)8% <0%> (ø)⬇️
core/templates/login.php0% <0%> (ø)0% <0%> (ø)⬇️
core/Migrations/Version13000Date20170718121200.php0% <0%> (ø)30% <0%> (ø)⬇️
... and 12 more

@jancborchardt

Copy link
Copy Markdown
Member

Guess this is best reviewed by @nextcloud/javascript? :)

danxuliuand others added 2 commits January 2, 2018 12:14
jQuery UI does not support touch events out of the box, so the features
that depend on jQuery UI, like dragging files in the Files app, are not
available on touch screens.
This commit imports the latest version of jQuery UI Touch Punch, which
adds support to touch events by mapping them to their mouse counterparts
and requires no changes to the existing code.
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
@rullzer
rullzerforce-pushed the add-support-for-touch-events-to-jquery-ui branch from d9f23ef to 91578b2CompareJanuary 2, 2018 11:15

@rullzerrullzer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

sure...

@MorrisJobkeMorrisJobke mentioned this pull request Jan 2, 2018
30 tasks
@MorrisJobke

Copy link
Copy Markdown
Member

@danxuliu What problem does it solve? I don't have a particular problem as of now. 🤔

@danxuliu

Copy link
Copy Markdown
MemberAuthor

@MorrisJobke It makes possible, for example, to drag and drop files on touch screens in the Files app, but it may affect other elements either in the server or in apps using jQuery UI features.

If it works fine in your mobile device maybe your browser translates touch events to mouse events or something like that. In that case you can see the problem toggling the touch events in the responsive design mode of Firefox or toggling the device toolbar in the Developer tools of Chromium (although this pull request was created before #7527 was merged, so better rebase onto master before checking it ;-) ).

@MorrisJobke

Copy link
Copy Markdown
Member

But now the scrolling is not possible anymore (except ontop of the checkboxes) :/

@danxuliu

Copy link
Copy Markdown
MemberAuthor

But now the scrolling is not possible anymore (except ontop of the checkboxes) :/

Ugh, you are totally right.

The drag and drop on touch screens could be changed to be initiated only after a long press on a file, but that would interfere with the common behaviour of showing a context menu with that gesture. Probably it would be better to disable drag and drop of files on touch screens (if support for touch events is added to jQuery UI; if it is not added then nothing needs to be done, as in current master it is implicitly disabled due to touch events not being handled).

Regardless of that adding support for touch events to jQuery UI would be needed at least for Nextcloud Talk to be able to drag the right sidebar to show it on touch screens. I do not know if it could benefit other apps, or if it could cause other problems like the scrolling one :-S So... maybe it would be better to add it directly on each app that needs it?

@MorrisJobke

Copy link
Copy Markdown
Member

So... maybe it would be better to add it directly on each app that needs it?

I would say so. At least I would not add it right now to 13 and blindly backport it.

@rullzer

Copy link
Copy Markdown
Member

Ok lets move this out of 13 then.

@MorrisJobke

Copy link
Copy Markdown
Member

@danxuliu@rullzer What are the next steps here? I would close this PR and fix it in the apps.

@danxuliu

Copy link
Copy Markdown
MemberAuthor

@MorrisJobke I agree.

@danxuliu
danxuliu deleted the add-support-for-touch-events-to-jquery-ui branch February 15, 2018 12:59
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to reviewWaiting for reviewsenhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@danxuliu@jancborchardt@MorrisJobke@rullzer