Find replacement for Modernizr touchevent test and deprecate [#3101922]

Problem/Motivation

In #3100937: Update modernizr to 3.8.0, it was discovered that Modernizr's touchevent test is deprecated. https://github.com/Modernizr/Modernizr/pull/2432
There are also problems with that test from Modernizr 3.7.1 onward, which requires an override of that test in Drupal core (added in the aformentioned issue).

Core's custom touchevent, added in #3100937 currently works fine, but may become outdated as newer browser versions are released and different ways of detecting touchevent become available.

What touchevents currently does for core

Modernizr adds a .touchevents or .no-touchevents class to <html> based on if the touchevents test is true or false.

The following CSS files have styles that expect one of these two classes.

./core/misc/dialog/off-canvas.tabledrag.css
./core/misc/dialog/off-canvas.button.css
./core/modules/contextual/css/contextual.theme.css
./core/modules/system/css/components/tabledrag.module.css
./core/themes/stable/css/core/dialog/off-canvas.tabledrag.css
./core/themes/stable/css/core/dialog/off-canvas.button.css
./core/themes/stable/css/contextual/contextual.theme.css
./core/themes/stable/css/system/components/tabledrag.module.css
./core/themes/stable9/css/core/dialog/off-canvas.tabledrag.css
./core/themes/stable9/css/core/dialog/off-canvas.button.css
./core/themes/stable9/css/contextual/contextual.theme.css
./core/themes/stable9/css/system/components/tabledrag.module.css
./core/themes/claro/css/components/dropbutton.css
./core/themes/claro/css/components/form--text.css
./core/themes/claro/css/components/table--file-multiple-widget.css
./core/themes/claro/css/components/tabledrag.css
./core/themes/claro/css/components/button.css
./core/themes/claro/css/components/form--select.css
./core/themes/claro/css/components/action-link.css
./core/themes/claro/css/theme/views_ui.admin.theme.css
./core/themes/seven/css/components/buttons.css
./core/themes/olivero/css/components/tabledrag.css

In many (most?) cases this is not trivial styling, but fairly significant changes.

Contextual Links also use the value of Modernizr.touchevents in two places:
./core/modules/contextual/js/views/VisualView.es6.js

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

./core/modules/contextual/js/views/RegionView.es6.j

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

Proposed resolution

Determine if there is another library capable of detecting touchevent in a way that is useful to core. If so, add that library and deprecate Modernizr touchevent.
If there isn't another library that fits that exact need, determine if it's better to support Drupal core's custom touchevent test, or using a different library that may require refactoring how touchevent/no-touchevent classes are applied and what CSS rules are based on them.

I propose building on the existing logic in core/misc/modernizr-additional-tests.es6.js. This is in core because Modernizr itself has deprecated touchevents , and for good reason: it's not an accurate test. However, the criteria used by the former Modernizr.touchevents is very established in core, with many styles (and a little bit of JS) relying on its verdict. Switching to logic that is different from this (admittedly flawed) touchevents check would be very disruptive. The fact that it was disruptive is why core/misc/modernizr-additional-tests.es6.js is in core to begin with.

So I suggest keeeping the logic in core/misc/modernizr-additional-tests.es6.js, but making it a standalone asset, not a modernizr feature. This would be expanded to add the .touchevents/.no-touchevents as needed, and the contextual links JavaScript could either look for

'html.touchevents'

or we can move the touchevents result into drupalSettings.

TLDR: although better touchevents detection might exist, changing this would be prohibitively disruptive. 50% of the logic is already part of core, so adding the missing 50% will get us closer to being able to remove Modernizr altogether.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

The Modernizr.touchevents test has been deprecated. In addition, Modernizr is no longer responsible within Drupal for detecting touch devices and adding the touchevents/no-touchevents classes to the html element based on the detection result. The specific functionality of adding classes based on touch device detection is now provided via a core library: core/drupal.touchevents-test. The detection method used is identical to Modernizr's.

Comment	File	Size	Author
#30	3101922-30-10x.patch	13.27 KB	bnjmnm
#29	interdiff-26-29.txt	1.46 KB	nod_
#29	core-touch-3101922-29.patch	23.68 KB	nod_
#26	interdiff_23-26.txt	913 bytes	bnjmnm
#26	3101922-26.patch	23.57 KB	bnjmnm
#23	3101922-23-94x.patch	23.45 KB	bnjmnm
#22	3101922-22-94x.patch	23.49 KB	bnjmnm
#20	3101922-20-94x.patch	17.77 KB	bnjmnm
#18	interdiff_15-18.txt	9.25 KB	bnjmnm
#18	3101922-18-94x.patch	15.94 KB	bnjmnm
#15	3101922-15-94x.patch	9.11 KB	bnjmnm

Issue fork drupal-3101922

Show commands

Start within a Git clone of the project using the version control instructions.

Add & fetch this issue fork’s repository

Or, if you do not have SSH keys set up on git.drupalcode.org:

Add & fetch this issue fork’s repository

3101922-find-replacement-for compare
Check out this branch for the first time

Check out existing branch, if you already have it locally

About issue forks

Comments

Comment #1

18 December 2019 at 19:39

bnjmnm created an issue. See original summary.

Comment #2

lauriii

he/him

Finnish

Finland

commented 8 January 2020 at 10:15

We can't create a test that does what people think this test does so we should end the confusion and remove the test. If people want the test in their projects it's an easy one to implement.

Additionally, I would argue that testing for what people think this test does is harmful to users and contributing to that pattern (even via a misunderstanding of what this test does) is bad for the web.

This quote is from the PR. I'm wondering if we use the test correctly? Could we update the issue summary with info on what depends on the touchevent test so we could try to evaluate that?

Comment #3

8 January 2020 at 10:15

Version:

9.0.x-dev

» 9.1.x-dev

Drupal 9.0.10 was released on December 3, 2020 and is the final full bugfix release for the Drupal 9.0.x series. Drupal 9.0.x will not receive any further development aside from security fixes. Sites should update to Drupal 9.1.0 to continue receiving regular bugfixes.

Drupal-9-only bug reports should be targeted for the 9.1.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.2.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Comment #4

8 January 2020 at 10:15

Version:

9.1.x-dev

» 9.3.x-dev

Drupal 9.1.10 (June 4, 2021) and Drupal 9.2.10 (November 24, 2021) were the last bugfix releases of those minor version series. Drupal 9 bug reports should be targeted for the 9.3.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Comment #5

xjm

she/her

English

commented 14 March 2022 at 19:20

Parent issue:

» #3238507: Drupal 10 JavaScript dependency plan

Comment #6

xjm

she/her

English

commented 31 March 2022 at 01:42

Version:	9.3.x-dev	» 9.4.x-dev
Issue tags:		+Drupal 10, +Drupal 9.4 target

Comment #7

bnjmnm

he/him

English

Ann Arbor, MI

commented 4 April 2022 at 16:28

Version:

9.4.x-dev

» 10.0.x-dev

Comment #8

bnjmnm

he/him

English

Ann Arbor, MI

commented 4 April 2022 at 16:49

Issue summary:

View changes

Comment #9

bnjmnm

he/him

English

Ann Arbor, MI

commented 4 April 2022 at 17:44

The touchdevices check can potentially be simplified/

Part of Modernizr's check looks for what is essentially the following:
@media (touch-enabled),(-webkit-touch-enabled),(-moz-touch-enabled),(-o-touch-enabled),(-ms-touch-enabled)

-moz-touch-enabled was removed in Firefox 73 a few years ago, it's well past the "last two major relesases" supported browsers range. 👉 https://bugzilla.mozilla.org/show_bug.cgi?id=1486964. I see no evidence of any browser other than Firefox supporting a touch-enabled media query. Another set of eyes could be useful, but I think the media-query-checking logic isn't needed since no supported browsers recognize a touch-enabled media query.

That could make replacing the modernizr touchevents pretty easy, just run this on load.

const toucheventsClass =  'ontouchstart' in window ||
      (window.DocumentTouch && document instanceof window.DocumentTouch) ? 'touchevents' : 'no-touchevents';

document.documentElement.classList.add(toucheventsClass)

Comment #10

nod_

French

Lille

commented 5 April 2022 at 11:19

Sounds good to me

Comment #11

lauriii

he/him

Finnish

Finland

commented 20 April 2022 at 11:48

Before we go deeper into figuring out the approach for implementing replacement for this, we should evaluate if we this is something we should be doing at all as mentioned in #2. The reason this was removed from Modernizr was that they thought it was harmful for the web to do checks like this, because having touch enabled on a device doesn't indicate that the user is using touch, since the device could have touch enabled while still having a mouse.

Comment #12

bnjmnm

he/him

English

Ann Arbor, MI

commented 20 April 2022 at 13:36

Re #11: I updated the issue summary in #8 and I hoped the proposed solution had a good justification of the approach, but I'll try a differently worded version here and see what people think.

This approach requires very little code, and gets us off Modernizr without any disruptive changes
While the Modernizr detection approach was not ideal, it's been the logic used by Drupal for years. It's what people are used to, and changing this could be disruptive. I couldn't find any issues where people were having problems with the current detection, but it's possible they don't contain the keywords I was searching for
Ultimately, it would be better to remove anything replying on the touchevents/no-touchevents classes and remove this detection entirely. There doesn't seem to be a reliable way to do this in general. That doesn't need to be in the scope of eliminating Modernizr as a dependency, though. This could take a bit of time, though. Claro in particular has a good amount of styling based on these classes. I think it's best to quickly eliminate the Modernizr dependency with my approach, then phase out touch detection carefully.

Comment #13

lauriii

he/him

Finnish

Finland

commented 21 April 2022 at 08:31

Thank you for explaining the thought process behind #9! That is really helpful. 💯

I was curious and reviewed most of the CSS uses. Those seem fine since they are enhancements making the UI more touch friendly.

The JavaScript uses are a bit concerning and those could be something we should look into deeper and potentially try to deprecate (either this issue or follow-up). I'm wondering if #2168711: Modernizr.touchevents use can lead to scenarios that break contextual links is potentially related to that.

I like the idea of limiting the API to the CSS class. However, documenting the concerns on using those CSS classes is harder than if it was a JavaScript API.

I'm wondering if the steps are then:

Deprecate core/misc/modernizr-additional-tests.es6.js
Add a standalone version of core/misc/modernizr-additional-tests.es6.js which adds a CSS class to the <html> element
As part of the CR that documents deprecating core/misc/modernizr-additional-tests.es6.js, document the downsides/concerns of using the touchevents detection
Figure out what to do about the contextual links usage which is potentially creating problems

Comment #14

bnjmnm

he/him

English

Ann Arbor, MI

commented 21 April 2022 at 15:07

I added a patch to #2168711: Modernizr.touchevents use can lead to scenarios that break contextual links that eliminates the Modernizr dependency. If that landed we'd only have to be concerned with steps 1-3 of #13!

Comment #15

bnjmnm

he/him

English

Ann Arbor, MI

commented 27 April 2022 at 15:11

Status:

Active

» Needs review

Status	File	Size
new	3101922-15-94x.patch	9.11 KB

Here's a first try of deprecating & adding the replacement. Lets see what the tests do.

Comment #16

nod_

French

Lille

commented 27 April 2022 at 16:07

+++ b/core/misc/modernizr-additional-tests.es6.js
@@ -2,7 +2,20 @@
+  Drupal.deprecatedProperty({

Might need to do a Modernizr = Drupal.deprecateProperty…

Should probably be below the Modernizr.addTest() too.

Comment #17

27 April 2022 at 16:11

Status:

Needs review

» Needs work

The last submitted patch, 15: 3101922-15-94x.patch, failed testing. View results

Comment #18

bnjmnm

he/him

English

Ann Arbor, MI

commented 27 April 2022 at 20:32

Status:

Needs work

» Needs review

Status	File	Size
new	3101922-18-94x.patch	15.94 KB
new	interdiff_15-18.txt	9.25 KB

re #16

Yep that was the wrong use of deprecatedProperty

I have deprecatedProperty happening first as the Modernizr.addTest() will only be called if drupal.touchevents-test isn't loaded. Deprecating the property is always necessary, but the test isn't. An early return seems like the most elegant way to do it, but if the order seems important I could wrap Modernizr.addTest() in a conditional instead.

This patch adds tests, and I'm running into a problem where Nightwatch isn't seeing the deprecations, but when I pause the Chromedriver window I can see the deprecations in the console. Seems like a "I forgot {x} setting" kind of thing, but I'll put what I have as is for the time being.

Comment #19

nod_

French

Lille

commented 28 April 2022 at 09:39

+++ b/core/modules/system/tests/modules/load_a_library_test/src/Controller/LoadLibraryController.php
@@ -0,0 +1,25 @@
+    $build['#attached']['library'][] = "$module/$library";

Seems dangerous to pass the input as-is. It looks safe but I would restrict the possibilities just in case.

+++ b/core/tests/Drupal/Nightwatch/Tests/touchDetectionTest.js
@@ -0,0 +1,68 @@
+    browser.drupalInstall().drupalLoginAsAdmin(() => {
+      browser
+        .drupalRelativeURL('/admin/modules')
+        .setValue('input[type="search"]', 'Load a library')
+        .waitForElementVisible(
+          'input[name="modules[load_a_library_test][enable]"]',
+          1000,
+        )
+        .click('input[name="modules[load_a_library_test][enable]"]')
+        .click('input[type="submit"]'); // Submit module form.

We have the drupalInstallModule command to simplify a bit:

browser
  .drupalInstall()
  .drupalInstallModule('load_a_library_test');

Comment #20

bnjmnm

he/him

English

Ann Arbor, MI

commented 28 April 2022 at 13:02

Status	File	Size
new	3101922-20-94x.patch	17.77 KB

Addresses #19.

Still not sure how to get the deprecations detected in the tests. As mentioned earlier, I can see those deprecations if I pause the tests locally, but deprecationErrorExists isn't seeing them and I bet it's some simple little thing I'm overlooking 🙂

Comment #21

nod_

French

Lille

commented 28 April 2022 at 13:26

There is a little load order issue in the last patch.

Drupal is used in the modernizr additional tests file, but core/drupal is not a dependency and drupal.js is not loaded in the head. So the various deprecation methods are not available at this point.

Comment #22

bnjmnm

he/him

English

Ann Arbor, MI

commented 4 May 2022 at 16:28

Status	File	Size
new	3101922-22-94x.patch	23.49 KB

Some acrobatics were required to make the deprecation detectable in tests due to Modernizr's library definition being set to header: true and the weights set to load before js_testing_log.js and its related dependencies. The test module I added (modernizr_deprecation_test) uses library_info_alter() to get around this, but I'm unsure if the deprecation is visible to tooling without that module running.

Comment #23

bnjmnm

he/him

English

Ann Arbor, MI

commented 4 May 2022 at 17:15

Status	File	Size
new	3101922-23-94x.patch	23.45 KB

Comment #24

nod_

French

Lille

commented 4 May 2022 at 21:10

haven't looked in detail yet but copy/paste solution for the deprecation method is +1 from me

Comment #25

nod_

French

Lille

commented 5 May 2022 at 09:16

Status:

Needs review

» Needs work

+++ b/core/core.libraries.yml
@@ -758,6 +759,17 @@ drupal.timezone:
+  dependencies:
+    - core/drupal
+    - core/once

This makes drupal and once load in the header and they are not required as far as I can see.

+++ b/core/misc/touchevents-test.es6.js
@@ -0,0 +1,11 @@
+document.documentElement.classList.add(
+  'ontouchstart' in window ||
+    (window.DocumentTouch && document instanceof window.DocumentTouch)
+    ? 'touchevents'
+    : 'no-touchevents',
+);

The condition is a bit hard to read but it prevents a temporary variable to be created.

So shortest code achieve that +1

```
+++ b/core/core.libraries.yml
@@ -758,6 +759,17 @@ drupal.timezone:
+    misc/touchevents-test.js: { preprocess: 0, weight: -15 }
```
so modernizr test is at -21 and -20. so this should be -22.

Not sure we need the preprocess = 0 either. That might simplify the tests as well?

Comment #26

bnjmnm

he/him

English

Ann Arbor, MI

commented 11 May 2022 at 19:43

Status:

Needs work

» Needs review

Status	File	Size
new	3101922-26.patch	23.57 KB
new	interdiff_23-26.txt	913 bytes

Addresses #25

Comment #27

nod_

French

Lille

commented 12 May 2022 at 07:22

Status:

Needs review

» Reviewed & tested by the community

Perfect, thanks. Test looks good too.

Comment #28

lauriii

he/him

Finnish

Finland

commented 12 May 2022 at 08:55

Status:

Reviewed & tested by the community

» Needs work

Did some research related to #9 and I agree that it seems fine to remove the media query based check since it was highly unlikely to do anything on the supported browsers.

+++ b/core/misc/modernizr-additional-tests.es6.js
@@ -3,6 +3,77 @@
+      console.log('will return', target);

Should we remove the console log?

```
+++ b/core/misc/modernizr-additional-tests.es6.js
@@ -3,6 +3,77 @@
+  const _deprecatedPropertyModernizrCopy = ({
```
Can we add a short comment explaining why we are not using this from the core/drupal? I saw that was discussed here and agree that there's a good reason to do so but would be nice to have that documented in the code too.

Comment #29

nod_

French

Lille

commented 12 May 2022 at 09:16

Status:

Needs work

» Reviewed & tested by the community

Status	File	Size
new	core-touch-3101922-29.patch	23.68 KB
new	interdiff-26-29.txt	1.46 KB

I really struggle with comments so I did my best there even if it doesn't look like it :/

Comment #30

bnjmnm

he/him

English

Ann Arbor, MI

commented 12 May 2022 at 12:39

Status	File	Size
new	3101922-30-10x.patch	13.27 KB

Comment #31

nod_

French

Lille

commented 12 May 2022 at 12:49

+++ b/core/core.libraries.yml
@@ -723,6 +724,14 @@ drupal.timezone:
+    misc/touchevents-test.js: { weight: -21 }

should that be -20 to match the weight of the old modernizr test?

The weight thing is a detail so either way RTBC for me.

Comment #32

bnjmnm

he/him

English

Ann Arbor, MI

commented 12 May 2022 at 13:11

Re #31

should that be -20 to match the weight of the old Modernizr test?

Intentionally set to -21. The old test was -20 because it required Modernizr to be present. Now that this is a dependency-free test I thought it best to put it at the same weight as the Modernizr so -21 is the home to all JS that adds classes to <html> based on feature detection results.

Comment #33

nod_

French

Lille

commented 12 May 2022 at 13:39

works for me

Comment #34

12 May 2022 at 13:51

lauriii committed 199f2b4 on 10.0.x

Issue #3101922 by bnjmnm, nod_, lauriii: Find replacement for Modernizr...

Comment #35

12 May 2022 at 13:53

lauriii committed 3070c83 on 9.5.x

Issue #3101922 by bnjmnm, nod_, lauriii: Find replacement for Modernizr...

Comment #36

12 May 2022 at 13:53

lauriii committed a52e937 on 9.4.x

Issue #3101922 by bnjmnm, nod_, lauriii: Find replacement for Modernizr...

Comment #37

lauriii

he/him

Finnish

Finland

commented 12 May 2022 at 13:54

Version:	10.0.x-dev	» 9.4.x-dev
Status:	Reviewed & tested by the community	» Fixed

Committed 199f2b4 and pushed to 10.0.x. Committed #29 to 9.5.x. and cherry-picked to 9.4.x. Thanks!

Comment #38

bnjmnm

he/him

English

Ann Arbor, MI

commented 12 May 2022 at 17:53

Issue summary:	View changes
Issue tags:		+9.4.0 release notes

Comment #39

26 May 2022 at 17:54

Status:

Fixed

» Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

Find replacement for Modernizr touchevent test and deprecate

Problem/Motivation

What touchevents currently does for core

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3101922

Comments

Change records for this issue

Parent issue

Related issues

Referenced by