Update modernizr to 3.8.0

There's logic in Drupal.tableDrag.prototype.findDropTargetRow that uses coordinates and row height to determine if a dragged row is in a new target region
if (y > rowY - rowHeight && y < rowY + rowHeight)

Debugging \Drupal\Tests\field_ui\FunctionalJavascript\ManageDisplayTest::testFormatterUI, I found that the values of the variables in this conditional are different with Modernizr 3.8.0. They are close enough that it's still possible to manually drag a row into a new position. However, these different values are not enough to trigger a new position for the pointer distance covered by dragTo()

Values in testFormatterUI with Modernizr 3.3.1

Y: 165 || rowY: 51 || rowHeight: 14.5
Y: 165 || rowY: 82 || rowHeight: 14.5
Y: 165 || rowY: 113 || rowHeight: 17
Y: 165 || rowY: 149 || rowHeight: 10
Y: 165 || rowY: 171 || rowHeight: 10

Values in testFormatterUI with Modernizr 3.8.0

Y: 199 || rowY: 51 || rowHeight: 22
Y: 199 || rowY: 97 || rowHeight: 22
Y: 199 || rowY: 143 || rowHeight: 22
Y: 199 || rowY: 189 || rowHeight: 10
Y: 199 || rowY: 211 || rowHeight: 10

Looking into what may be causing this difference.

The test failure in LayoutBuilderDisableInteractionsTest::testFormsLinksDisabled can be reproduced manually.

• Enable layout builder and go to "Manage Layout" for any content type
• Add a search block
• Save the layout
• Return to "Manage Layout" and it will no longer be possible to reveal contextual links on a block by hovering over it

Not yet sure why this is the case, but the steps to reproduce are specific enough to suggest there may be a fairly specific answer awaiting us.

Looks like this is due to Modernizr.touchevents returning different results than 3.3.1 than 3.8. Will investigate solutions.

In 3.7.1, Modernizr.touchevents added window.TouchEvent as one of the ways of determining if a browser supports touchevents. This results in far fewer instances of .no-touchevents (perhaps none?) and is the reason the tests are failing.

The touchevents test is deprecated in Modernizr 3.8 and will be removed in 4.0, so it may be worth refactoring out of this particular test anyway

This addresses the test failures, which were due to changes in Modernizr's touchevents test. This is a problem others have experienced: https://github.com/Modernizr/Modernizr/issues/2431 , and it will not be fixed as the touchevents test is already deprecated and will be removed in 4.0.

To fix this, I created a Modernizr build that did not include touchevents, but added two options (mq and prefixes) that would make it possible to recreate the touchevents test as a custom addition. This otherwise matches the configuration of the 3.3.1 build previously used in core.

This is the build command I used:
./bin/modernizr -f details,inputtypes -o addTest,prefixed,setClasses,testStyles,mq,prefixes -d ./modernizr.min.js -u

No interdiff since this is not based on the previous patch.

Patch #11 did not include the compiled modernizr-additional-tests.js this fixes that.

Re #14
⓵

This URL is in the JS so someone could rebuild this like when we need to upgrade.

Is this the same as the build comment above? If not how will someone know to do the specific build in the future?

That's something should have mentioned. That URL will download Modernizr with the correct config... BUT, it will be a custom build of version 3.6.0, not the most recent. In order to create a build with Modernizr 3.8.0, I had to do it locally.

⓶

I don't think it is clear that the Modernizr we have included in core doesn't include touchevents. When I read this before before reading explanation in the last comment on this issue I didn't get that. We should make this very clear.

Yea I had a little trouble explaining this well. Lets see if this patch improves it and I'll keep iterating until it makes sense.

⓷

is this something we should also deprecate?

Yea probably, I tagged with Needs Followup and will take care of that. It's been deprecated from Modernizr because of how frequently developers misunderstand what it detects, but there's currently no issue with the actual functionality, so it's not horribly urgent at the moment https://github.com/Modernizr/Modernizr/pull/2432
But since it's no longer supported, there is the risk of this being out of sync with newer browsers that may have different indicators of touch support.

Created followup #3101922: Find replacement for Modernizr touchevent test and deprecate

Some info to help with testing of the modifications of Modernizr's touchevents test.
When Modernizr is loaded on a this test will either add .touchevents or .no-touchevents to <html>. The best way to confirm there are no regressions in the patch is manually test as many scenarios/devices as possible and confirm that .touchevents/.no-touchevents is added the same pre-patch and post-patch. Note that Chrome's mobile device simulator is recognized by Modernizr as supporting touchevents, while in normal desktop mode it adds .no-touchevents.

If testing with a device that doesn't offer easy access to html source, the behavior can be tested by using a feature impacted by CSS rules using .touchevents or .no-touchevents, but it may be easiest to temporarily add easily-identifiable CSS rules based on those classes.

1. +++ b/core/misc/modernizr-additional-tests.es6.js
   @@ -0,0 +1,36 @@
   +  // - `window.TouchEvent` is removed from the criteria that determines if the
   +  //   browser supports touchevents. This criteria results in false positives.
   +  //   This is a known issue in Modernizr, but will not be fixed as the
   +  //   touchevent is deprecated.
   +  //   @see https://github.com/Modernizr/Modernizr/pull/2432
   ...
   +    // This is the conditional that `window.TouchEvent` is removed from.
   ...
   +      (window.DocumentTouch && document instanceof window.DocumentTouch)
   

   Should we asses whether we should do this in a follow-up?

2. +++ b/core/misc/modernizr-additional-tests.es6.js
   @@ -0,0 +1,36 @@
   +    const query = [
   +      '(',
   +      Modernizr._prefixes.join('touch-enabled),('),
   +      'heartz',
   +      ')',
   +    ].join('');
   +    return Modernizr.mq(query);
   

   I spent a while figuring out what this code does. What it essentially does is it generates a bunch of media queries that could theoretically result in detecting whether the users' device has touch-enabled or not, but AFAIK this isn't supported by any browsers. The 'heartz' is added in the end just for the sake of having something to end the list because otherwise, this would end with '( which would make this an invalid media query. Let's add an inline comment explaining this.

Patch addresses the two items in #20

+++ b/core/misc/modernizr-additional-tests.js
@@ -0,0 +1,17 @@
\ No newline at end of file

No newline at end of file

Tests passed with php7.3

Re #23 - no newline at the end of the file is expected, that is how the file is provided by Modernizr.

Re #24 - this should be still set back to "Needs review" since the test failure is an unrelated test that is known to intermittently fail. It shouldn't have been set back to Needs Review because it passed on PHP 7.3 and not 7.2. Tests need to pass on all supported PHP versions. The intermittently-failing Media Library tests may have made that confusing.

Sorry, my comment on #20.1 was confusing. I tried to suggest there to remove all changes to the logic from this patch and move them to separate follow-ups so that they can be reviewed appropriately.

#27 makes sense. Updated the test so it's a copy of 3.3.1 with no logic changes.Since this test will need to ultimately be replaced, I think any following up can happen when the test is replaced in #3101922: Find replacement for Modernizr touchevent test and deprecate as opposed to updating the logic within it.

This looks good. Marking this as RTBC. I've tested manually on Chrome that touchevents are recognized correctly, but given that this has lots of cross browser compatibility concerns, this isn't enough to validate that the code is correct. Therefore, I simply compared the code with https://github.com/Modernizr/Modernizr/blob/v3.3.1/feature-detects/touch... and it seems to be exactly the same.

1. +++ b/core/misc/modernizr-additional-tests.es6.js
   @@ -0,0 +1,41 @@
   +  // This is a copy of Modernizr's touchevents test from version 3.3.1. Drupal
   

   Any thoughts on adding a @see comment referring to https://github.com/Modernizr/Modernizr/blob/v3.3.1/feature-detects/touch...?

2. +++ b/core/core.libraries.yml
   @@ -574,9 +574,11 @@ modernizr:
   +
   

   Nit: This new line is unnecessary (can be fixed on commit).

Let's address both points in #29. Thanks!

Addresses #29

#31 is small enough that I don't think we need another peer review. :)

Updating issue credit...

Given the manual testing documented in #8, #18, #19, and #29 I think this is ready!

...except that I get the following errors when trying to commit this:

yarn run v1.17.3
$ cross-env BABEL_ENV=legacy node ./scripts/js/babel-es6-build.js --check --file /Users/xjm/git/maintainer/core/misc/modernizr-additional-tests.es6.js
[12:59:48] '/Users/xjm/git/maintainer/core/misc/modernizr-additional-tests.es6.js' is being checked.
✨  Done in 1.00s.

/Users/xjm/git/maintainer/core/misc/modernizr-additional-tests.es6.js
  23:52  error  'DocumentTouch' is not defined  no-undef

✖ 1 problem (1 error, 0 warnings)

yarn run v1.17.3
$ cross-env BABEL_ENV=legacy node ./scripts/js/babel-es6-build.js --check --file /Users/xjm/git/maintainer/core/misc/modernizr-additional-tests.es6.js
[12:59:50] '/Users/xjm/git/maintainer/core/misc/modernizr-additional-tests.es6.js' is being checked.
✨  Done in 0.45s.

/Users/xjm/git/maintainer/core/misc/modernizr-additional-tests.es6.js
  23:52  error  'DocumentTouch' is not defined  no-undef

✖ 1 problem (1 error, 0 warnings)

Since the test is copied from an upstream test, I wonder if we need to simply mark it as a file to skip linting?

Addresses #33 by bypassing a single eslint rule in the test's conditional statement.

More nitpicky feedback on the patch 🧐

1. +++ b/core/core.libraries.yml
   @@ -574,9 +574,10 @@ modernizr:
   +    misc/modernizr-additional-tests.js: {preprocess: 0, weight: -20}
   

   I don't think we have actual coding standard on this but we've definitely standardized on adding space before and after the opening and the closing objects.

2. +++ b/core/misc/modernizr-additional-tests.es6.js
   @@ -0,0 +1,47 @@
   +    // Disable the eslint no-undef rule so the conditional is not functionally
   +    // different from the Modernizr 3.3.1 version this is copied from.
   +    /* eslint-disable no-undef */
   +    if (
   +      'ontouchstart' in window ||
   +      (window.DocumentTouch && document instanceof DocumentTouch)
   +    ) {
   +      /* eslint-enable no-undef */
   

   I would consider adding a parameter to the closure for passing these since it shouldn't interfere with functionality in any way.

3. +++ b/core/misc/modernizr-additional-tests.es6.js
   @@ -0,0 +1,47 @@
   +      // include the 'heartz' as a way to have a non matching MQ to help terminate the join
   +      // https://git.io/vznFH
   

   Nit: We might want to consider making this comply with the 80 characters per line coding standard.

4. +++ b/core/modules/system/src/Controller/SystemController.php
   @@ -16,7 +16,7 @@
   -class SystemController extends ControllerBase {
   +class  SystemController extends ControllerBase {
   

   We should revert this change. Can be done on commit.

Should we actually be making coding standards fixes on the test? Since it's copied from upstream.

Addresses #35

Item 2 was addressed by changing document instanceof DocumentTouch to document instanceof window.DocumentTouch after visiting the instanceof spec to be sure it was fine. Simply adding it to the closure wasn't passing linting and although it could certainly be made to work, this could be done with fewer changes and it's more readable.

Should we actually be making coding standards fixes on the test? Since it's copied from upstream.

It's actually not possible to work with an exact copy of Modernizr's test due to some variables not being in the same scope when a test is added outside of Modernizr. If it needs a few changes anyway, I see the benefit of fully adopting Drupal's JS code standards as well. Fortunately, it's a simple enough test that a reviewer can confirm that it is still functionally identical.

Oops, here is the patch.

The downside as I see it is that if Modernizr further updates the test, we can't diff easily to apply their changes to our test.

The downside as I see it is that if Modernizr further updates the test, we can't diff easily to apply their changes to our test.

There's a few reasons why this doesn't strike me as a concern:

• The touchevents test is deprecated, no changes will be made unless they're security related.
• This is the version from 3.3.1, the last version known to work, and there are already a few unavoidable changes due to scope differences with a few variables. Modernizr's maintainers specifically mentioned that the bug forcing Drupal to stay on this version will not be addressed. If there was a security-related change, it would already need to be applied surgically to Drupal's custom version of the test since it would be built on top of 3.8, not 3.3.1
• I suspect this code will be referenced the most in the process of working on #3101922: Find replacement for Modernizr touchevent test and deprecate , where we try to find a maintained solution for this deprecated test. For this scenario, it's easier to reference code that is consistent with core's JS standards.

(this isn't a hill I need to die on, btw 🙂)

OK, sounds like fair reasoning to me.

Looks like #38 addresses all of my feedback and #40 addresses concerns raised by @xjm on #39. Thank you @bnjmnm! 👏

Committed and pushed to 9.0.x. Thanks!