Deprecate Modernizr

Init patch

1. I also removed a whole date.es6.js file and its compiled version at all, because they were looks like fully created as a polyfill for browser which doesn't have a native datepicker. https://caniuse.com/input-datetime this thing shows that we are talking here mostly about IE11. I have checked mobile Safari under iOS, desktop Chrome, desktop firefox and MS Edge - and everywhere there is native datepicker. I have no time right now for ultra deep tests, but from what i saw we can fully kick that polyfill.

2. Same situation about HTML details - looks like we have used modernizr and polyfill and today it was actual only for IE11 (because again -> in other browsers from what i saw HTML details works good by default), so i decided to kick some extra code, not related to modernizr only.

3. About code a-la if (Modernizr.touchevents) { ... } we may have some custom js-hack instead of it probably, but right now i'm not sure yet how to handle all cases where such construction was removed. Maybe we can hack some of it on css level, there is some @media (hover: none) {} hacks exist.

It's anyway init patch still, but already can be tested/reviewed. I did couple of tests on my side and patch works correct looks like.

/var/www/html/core/modules/contextual/js/views/RegionView.es6.js
  16:13  error  'mapping' is never reassigned. Use 'const' instead  prefer-const

✖ 1 problem (1 error, 0 warnings)
  1 error and 0 warnings potentially fixable with the `--fix` option.

Please also don't try to remove date.js or the details polyfill in this issue, keep it scoped to Modernizr only; we have #3256549: Deprecate core/drupal.date asset library in 9.4.0 for the date polyfill and the details polyfill should handled in a separate issue too.

This is another instance where we should deprecate the library first in 9.4.

I think I was wrong about removing the date and details polyfills, as they do nothing in browsers that support them, which should be all browsers for 10.0.x

However, this patch also removes many calls to Modernizr.touchevents which then go on to handle differences between touch and non-touch devices - we can't just remove this, I think we need a replacement first, or we will be losing some functionality.

It might be worth spinning out separate issues to deal with the polyfill removals, as we can do that even if we don't get rid of Modernizr entirely just yet.

#3101922: Find replacement for Modernizr touchevent test and deprecate already exists to replace Modernizr.touchevents, so I guess we need to do that first.

Also opened #3269082: Remove HTML5 details collapse polyfill as removing the collapse polyfill seems straightforward.

I updated #3101922: Find replacement for Modernizr touchevent test and deprecate with a proposed solution. It's potentially a pretty quick solution to maintain the existing functionality without Modernizr present.

The drupal.date library was removed from 10.0 in #3256549: Deprecate core/drupal.date asset library in 9.4.0

To proceed here, we need to take care of #3101922: Find replacement for Modernizr touchevent test and deprecate and #3269082: Remove HTML5 details collapse polyfill

I think we could still make progress on planning the specific mechanics for deprecating the global Modernizr variable and/or the core/modernizr library because I believe there is some evidence that this is being used by themes and modules. We need to decide what are the recommended actions for getting rid of the deprecation warnings for anyone extending this. I guess we would want them to be able to rely on this until Drupal 10 since it might be needed for providing the necessary browser support. In Drupal 10, the recommended action would be to just stop relying on these, with the exception that we could encourage folks to stop using #3101922: Find replacement for Modernizr touchevent test and deprecate already in Drupal 9.

I'm wondering if we should postpone deprecating Modernizr library and the global variable to Drupal 10, and instead provide a hard coded global Modernizr variable in Drupal 10. This would make it easy for modules to continue supporting Drupal 9 without running into deprecation warnings.

I'm wondering if we should postpone deprecating Modernizr library and the global variable to Drupal 10

+1. This means contrib supporting both 9 & 10 can continue to do so without warnings. When 9 is EOL, Modernizr is fully unnecessary (assuming the near-completed touchevents issue lands), so we wouldn't be triggering warnings to remove something that might be necessary to work with a still-supported browser for the version being run.

I went through contrib usages of Modernizr and all of them are providing some functionality and loading a polyfill conditionally along the lines of:

if (Modernizr.details) {
  return;
}

I think based on this the approach that I proposed in #12 would make sense because it would keep the responsibility of deciding on whether to load the polyfill or not outside the module. The deciding factors could be whether the module is loaded on Drupal 9 or Drupal 10, but on top of that there could be a module that provides support for IE 11 in Drupal 10, that would override the static object provided by core with one generated by Modernizr.

I think the downside of the approach where we don't trigger deprecation warnings is that it could potentially accidentally make some modules remove support for IE 11. I think that risk exists regardless of what we do on this issue since we are not triggering deprecation warnings for other instances where IE 11 support is being removed because modules could be depending on code that is incompatible with IE 11 without getting any warning.

Based on that, I think keeping the responsibility of the decision to support IE 11 outside of majority of the modules would make sense, and in this instance it would be simple to achieve. I think if we implemented the proposal from #12, we should explicitly document this behavior as part of broader documentation about dropping support for IE 11.

#12 makes sense to me if I understand correctly. No deprecation in 9.x. Provide a fake modernizr in 10.x that always returns false (or true? whichever one it is) to make it a no-op with a change record. Deprecate modernizr in 10.x for 11.x as a whole (from 10.1.x onwards).

Here's a proof of concept of how this could look like – just to make it a bit easier to grasp what is being proposed here. I didn't remove any of the usages of Modernizr from core in case anyone wants to set breakpoints and confirm that this works as expected.

I understand that this is just a PoC and may be thrown away. I'm happy to implement different approach if there's another approach that is preferred over this. 😊 Marking NR just to get a CI run for the patch – this is not actually a reviewable final patch.

cspell 😇

+1 for #14 and #15 as well.

+++ b/core/core.libraries.yml
@@ -817,16 +817,9 @@ drupal.jquery.position:
 modernizr:
-  # Block the page from being loaded until Modernizr is initialized.
-  header: true
-  remote: https://github.com/Modernizr/Modernizr
-  license:
-    name: MIT
-    url: https://modernizr.com/license/
-    gpl-compatible: true
-  version: "3.11.7"
+  version: VERSION

Can we also add a comment on the library definition briefly explaining it and mentioning that it will be deprecated in Drupal 10? Since it's a little weird and different from what it was in 9.3.

Even though contrib uses of Modernizr were evaluated in #12, I'm still concerned that the (admittedly appealing) mock Modernizr demoed in #17 could lead to problems if a site is using other aspects of the Modernizr API. The presence of a Modernizr object in the global namespace could prove confusing to sites that are using core's Modernizr to add functionality (such as adding tests for newer features, not just the IE-specific ones core tests for).

While a library_info_alter makes it possible for a site to bring in "real" Modernizr, I'm still concerned about providing a window.Modernizr that doesn't deliver the full Modernizr. Although it doesn't appear to be used in contrib, it seems more like something a specific site might do. It seems possible that a high traffic Drupal might extend core's Modernizr either to support additional browsers or to detecting newer features and gracefully integrate them.

Within core, all Modernizr D10 use could easily be replaced with a few lines of JS similar to misc/touchevents-test.js to add the classes in <html> that Modernizr previously added. The library itself could then be deprecated, but since there may be sites using it for detection beyond what Core uses (via a not-internal API), I'd prefer it continue to be regular old full featured Modernizr. It means keeping a dependency around for another Major release, but the alternative seems like it could be rough on sites.

Can we just leave real Modernizr in core for Drupal 10, but remove all our uses of it and deprecate it either immediately or in 10.1? Then anyone who depends on the library still gets the real thing, but nothing in core will load it by default. I am not sure what the mock Modernizr gains us, if we don't use it in core anyway.

Can we just leave real Modernizr in core for Drupal 10, but remove all our uses of it and deprecate it either immediately or in 10.1?

That seems like less initial work. I guess the downside is what if the library goes out of support before the EOL of Drupal 10 and then gets an unpatched vulnerability or similar? But it still gets a couple of releases a year, so maybe that's unlikely. I don't know if there's still a big use case for it for other projects that aren't us or if everyone's going to stop using it over the next five years as IE11 support gets dropped across the board.

Went ahead and opened #3281559: Remove modernizr usages from core which seems like it can happen now regardless of what else we do here.

I am not sure what the mock Modernizr gains us, if we don't use it in core anyway.

It optimizes making it easy for contrib modules to be compatible with both, Drupal 10 and Drupal 9 without introducing changes, and introducing changes to browser support in the lifecycle of Drupal 9. Although it only does that for a the use case of providing polyfills conditionally based on whether certain browser features are available, and doesn't support advanced use cases such as adding custom tests or listening to events.

While a library_info_alter makes it possible for a site to bring in "real" Modernizr, I'm still concerned about providing a window.Modernizr that doesn't deliver the full Modernizr.

That raises a good point that there's some risk in having Modernizr object in these advanced use cases. We could have a Drupal 9.4.0 patch that deprecates all of the other APIs but I believe it doesn't fully address this concern since that still leaves room for confusion.

Can we just leave real Modernizr in core for Drupal 10, but remove all our uses of it and deprecate it either immediately or in 10.1?

I'm feeling more favorable towards this after concerns raised in #20. I still think it would be unfortunate to not be able to remove this dependency from Drupal 10 in particular because there's always the risk that it might not be supported for the lifetime of Drupal 10. However, I don't think we have been able to come up with a solution that doesn't come with downsides.

Rescoping to the new planned scope and moving to 10.1.x.

Opened #3309341: Remove toolbar/toolbar dependency on Modernizr to remove the Toolbar usage since that's a change we would ideally do only in a major release.

All child issues are in and only the library definition and some comments remain.

In this patch the library is deprecated and most comments have been updated, except one in the replacement library that states where the library came from, and one in vendor-update.js that I think can stay until we actually remove the library.

+++ b/core/themes/claro/templates/details.html.twig
@@ -19,11 +19,6 @@
 #}
-{#
-  Prefix 'details' class to avoid collision with Modernizr.
-
-  @todo Remove prefix after https://www.drupal.org/node/2981732 has been solved.
-#}
 {%
   set classes = [
     'claro-details',
diff --git a/core/themes/claro/templates/navigation/details--vertical-tabs.html.twig b/core/themes/claro/templates/navigation/details--vertical-tabs.html.twig

I don't think it should happen here, but should we open a new issue for this @todo?

Are we keeping the prefix? If we need to remove the prefix then we need to act fast. If we are ignoring the prefix from now on, the comment could be removed, but i agree with @catch, split it from the deprecation.

Over in #3269082-19: Remove HTML5 details collapse polyfill @bnjmnm gives three reasons for just removing the @todo and keeping the prefix.

Thanks, that makes total sense.

Moving to the D11 roadmap.

Added a change record and release note.

This issue is being reviewed by the kind folks in Slack, #need-reveiw-queue. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge require as a guide.

Reviewing the patch and everything looks good.
Seems #30 was 31-32 address why removing the prefix is fine now.

Reading the CR everything looks good. Going to mark this but only comment on the CR would it be useful for an example snippet for how contrib can add modernizer to their module?

Going to mark this but only comment on the CR would it be useful for an example snippet for how contrib can add modernizer to their module?

There isn't any one standard or best practice for doing so. #2873160: Implement core management of 3rd-party FE libraries is an existing issue to address this problem space for contrib.

Updated the link in #29 to point to the real change record, leaving as RTBC.

We have the following remaining references to "modernizr" once this patch is applied:

[ayrton:maintainer | Mon 07:00:39] $ grep -ri "modernizr" * | grep -v "node_modules" | grep -v "core/assets/vendor/modernizr"
core/misc/cspell/dictionary.txt:modernizr
core/misc/touchevents-test.js: * A replacement for Modernizr touch events detection.
core/.eslintrc.legacy.json:    "Modernizr": true
core/COPYRIGHT.txt:  Modernizr - Copyright (c) 2021 The Modernizr Team
core/core.libraries.yml:    # Set weight to -21 so it loads alongside Modernizr, the library previously
core/core.libraries.yml:modernizr:
core/core.libraries.yml:  # Block the page from being loaded until Modernizr is initialized.
core/core.libraries.yml:  remote: https://github.com/Modernizr/Modernizr
core/core.libraries.yml:    url: https://modernizr.com/license/
core/core.libraries.yml:    assets/vendor/modernizr/modernizr.min.js: { preprocess: 0, weight: -21, minified: true }
core/scripts/js/vendor-update.js: * This script handles all dependencies except CKEditor and Modernizr, which
core/.eslintrc.json:    "Modernizr": true,

The dictionary, libraries, cspell, .eslintrc.json, and vendor-update.js mentions should be removed in the D11 followup. (Maybe it would make sense to file it as postponed now, so we don't miss those parts?) However, I think that the mention in touchevents-test.js should be removed.

The touch-events.js mention is the only documentation for that class:

/**
 * @file
 * A replacement for Modernizr touch events detection.
 */

document.documentElement.classList.add(
  'ontouchstart' in window ||
    (window.DocumentTouch && document instanceof window.DocumentTouch)
    ? 'touchevents'
    : 'no-touchevents',
);

If we want to remove it, we'd need to replace it with something else, but I kind of think it's fine - it's a port of a single feature from modernizr.

I came to the same conclusion as @catch in #29, I think it's OK to retain the source of the feature.

Opened up #3334920: Remove references to modernizer for #40

Committed aa373a8 and pushed to 10.1.x. Thanks!

Closed #2848401: [upstream] Modernizr prevents strict CSP as a duplicate