Problem

Tabledrag and contextual links don't work on Firefox (nothing happens when clicking on a row handle, or hovering the mouse on a contextual links region), and probably on other browsers/devices where Modernizr.touch gives a false positive or that have a hybrid (touch + pointer) input.

The Modernizr.touch documentation recommends to "set both touch *and* mouse events together, to cater for hybrid devices". However, in the two locations in core javascript (tabledrag.js and contextual.js) where the feature is used right now, Modernizr.touch is used to exclude mouse events:

tabledrag.js, row 125:

if (Modernizr.touch) {
  $(document).on('touchmove', function (event) { return self.dragRow(event.originalEvent.touches[0], self); });
  $(document).on('touchend', function (event) { return self.dropRow(event.originalEvent.touches[0], self); });
}
else {
  $(document).on('mousemove', function (event) { return self.dragRow(event, self); });
  $(document).on('mouseup', function (event) { return self.dropRow(event, self); });
}

tabledrag.js, row 327:

if (Modernizr.touch) {
  handle.on('touchstart', function (event) {
    event.preventDefault();
    event = event.originalEvent.touches[0];
    self.dragStart(event, self, item);
  });
}
else {
  handle.on('mousedown', function (event) {
    event.preventDefault();
    self.dragStart(event, self, item);
  });
}

contextual.js, row 261:

// We only want mouse hover events on non-touch.
if (!Modernizr.touch) {
  mapping.mouseenter =  function () { this.model.focus(); };
}

contextual.js, row 393:

// We don't want mouse hover events on touch.
if (Modernizr.touch) {
  mapping = {};
}
return mapping;

Proposed resolution

Change the above code to always add mouse events, and use Modernizr.touch only to detect if touch events should be added.

Files: 
CommentFileSizeAuthor
#14 touch-mouse-14.patch1.56 KBdroplet
#12 touch-mouse-12.patch1.49 KBdroplet
#5 2168711-5.patch4.96 KBmariancalinro
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,805 pass(es). View
#1 modernizrtouch-breaks-firefox-2168711-1.patch2.52 KBpeterpoe
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch modernizrtouch-breaks-firefox-2168711-1.patch. Unable to apply patch. See the log in the details link for more information. View

Comments

peterpoe’s picture

Status: Active » Needs review
FileSize
2.52 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch modernizrtouch-breaks-firefox-2168711-1.patch. Unable to apply patch. See the log in the details link for more information. View
LewisNyman’s picture

Status: Needs review » Needs work

The last submitted patch, 1: modernizrtouch-breaks-firefox-2168711-1.patch, failed testing.

mariancalinro’s picture

Assigned: Unassigned » mariancalinro
mariancalinro’s picture

Assigned: mariancalinro » Unassigned
Status: Needs work » Needs review
FileSize
4.96 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,805 pass(es). View

I have a possible solution for this issue. It's not extremely pretty, but it's smart:

If we attach a touchstart event listener to the window object very early, this is the very first event handler that will be executed when (if) a user touches the screen. We initialize a variable that represents the presence of a touch capable screen(device) to false, and set it to true inside the event listener. Also, we can attach the event using one(), so that after it's triggered it unattaches itself. Code from the patch:

// Detect if screen is touch capable. This only happens at the first user
// interaction, so you can't depend on it when te page is renderd.
Modernizr.touchDisplay = false;
if (Modernizr.touch) {
  // touchstart is the first event triggered when a user touches the
  // screen, so this will be executed before any other listener that
  // needs to know if the device is touch-capable or not.
  jQuery(window).one('touchstart', function (){
    Modernizr.touchDisplay = true;
  });
}

The only problem with this solution is that at the time the page is rendering, the user most definitely did not interact yet with the page, so we can't know yet if the device has a touch capable screen or not. This limits our use of this detection mechanism to the event handlers, because they are invoked after the user started to interact with the page.

I have created a patch that implements this ideea, I just haven't been able to test this on a touch device, I will probably test this on simplytest.me sometimes later in the day.

P.S. I am not sure if adding a new attribute to the Modernizr object is the best approach, but it has the added benefit of being the first place where someone would look for this.

mariancalinro’s picture

I managed to test this patch on my Nexus 10 in Chrome, and table drag is working. I don't know where the code from contextual.js is used, so that one is not tested on a mobile device. Also we need to test this in a desktop browser that reports it has support for touch events.

LewisNyman’s picture

Issue tags: +JavaScript, +frontend
Wim Leers’s picture

Then please also make sure to not create a UX regression in contextual links; we specifically excluded hover events there on touch-enabled devices because when you have both "hover" and "click" events on a certain DOM element, then "hover" equates to a single tap and "click" equates to a double-tap, which is an atrocious UX.

jhedstrom’s picture

Status: Needs review » Needs work

Needs work based on #8 I think.

droplet’s picture

swentel’s picture

Ok, I was taken here after a couple of duplicates :)

Thing is, contextual links don't even show up when I hover on my machine, on chrome. Firefox is fine.
Note that it's a touch enabled laptop.

droplet’s picture

Title: Wrong use of Modernizr.touch, breaks tabledrag and contextual links on Firefox » Wrong use of Modernizr.touch, breaks contextual links
Status: Needs work » Needs review
Issue tags: +Needs manual testing
FileSize
1.49 KB

Try it.

tableDrag, I think I've fixed it some times ago..

droplet’s picture

Let me explain it in a little further. Above patch works because `touchstart` event is fired before mouse event.

droplet’s picture

Ahh. I think I should add `mousemove` event to re-enable it again after any touch event

(this patch may cause little performance impacts)

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

ChristianAdamski’s picture

Bump.

Issue still present on touch-enabled laptop. This problem won't go away.

drpal’s picture

Version: 8.3.x-dev » 8.4.x-dev

@droplet

+++ b/core/modules/contextual/js/views/RegionView.js
@@ -16,16 +16,30 @@
+          istouchStart = true;

Is the expectation to completely disable the 'hover' behavior when interacting with the region?

@ChristianAdamski

Could you give the patch in #14 a shot?