Problem/Motivation

The polyfill added in #1815602: Introduce a polyfill for matchMedia is no longer needed by any of the browsers supported by Drupal core.
See: https://caniuse.com/#feat=matchmedia

Proposed resolution

Mark the asset library deprecated in core.libraries.yml.

Remaining tasks

  1. Write patch
  2. Test against supported browsers to ensure no change in behavior

User interface changes

API changes

Data model changes

Release notes snippet

The core/matchmedia and core/matchmedia.addListener asset libraries are marked deprecated, to be removed in Drupal 9.
A contrib module was created to provide these libraries for sites which still need to support older browsers.

CommentFileSizeAuthor
#17 3086369-17.patch1.95 KBlauriii
#8 interdiff.txt1.68 KBzrpnr
#5 interdiff_2-5.txt2.64 KBzrpnr
#5 3086369-5.patch1.93 KBzrpnr
#2 3086369-2.patch2.79 KBzrpnr
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

zrpnr created an issue. See original summary.

zrpnr’s picture

zrpnr
he/him
Primary language English
Location UTC-7
CreditAttribution: zrpnr at Acquia commented
Status: Active » Needs review
FileSize
3086369-2.patch2.79 KB

This can be removed from libraries files and leave all the JavaScript unchanged, since the polyfill adds matchMedia to the window object.
Removed dependency on core/matchmedia and core/matchmedia.addEventListener from:

  • drupal.vertical-tabs
  • picturefill
  • ckeditor
  • toolbar
  • seven

Checked IE using a responsive image so that picturefill would be loaded, and with resizing the toolbar to make sure the events fired.
Added the draft change record for the deprecation message.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

zrpnr’s picture

zrpnr
he/him
Primary language English
Location UTC-7
CreditAttribution: zrpnr at Acquia commented

I talked with @xjm and @lauriii and they suggested adding a contrib module to replace this library, I added https://www.drupal.org/project/matchmedia

This is identical to the core library except I added header: true just to ensure it would load at the top of the page so any other files would come back to it. It attaches the libraries while the module is active vs current core which has selective dependencies. I thought the simpler approach worked in the contrib since if it's necessary it's better to have it on all the time, but I'm happy to make changes if that doesn't seem right to any reviewers.

zrpnr’s picture

zrpnr
he/him
Primary language English
Location UTC-7
CreditAttribution: zrpnr at Acquia commented
Version: 8.9.x-dev » 8.8.x-dev
FileSize
3086369-5.patch1.93 KB
interdiff_2-5.txt2.64 KB

Just like in #3086379: Deprecate html5shiv and remove usages in core this patch is more conservative and just adds the deprecation mark to matchmedia and matchmedia.addListener.
These asset libraries are not used directly in tests however they are attached by vertical-tabs, toolbar and others which do have tests, so I suppressed the deprecation warnings.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs review » Needs work

This patch looks great :)

But I am concerned about https://git.drupalcode.org/project/matchmedia/blob/8.x-1.x/matchmedia.mo.... It should not be attaching the matchmedia/matchmedia on all pages.

(Because we try to be thoughtful about front-end performance and load things only when they’re necessary. That was one of the big perf improvements in Drupal 8 core compared to 7 actually 🙂Zero JS for anon users instead of lots of unused and hence useless JS.)

I have two remarks about https://git.drupalcode.org/project/matchmedia/blob/8.x-1.x/matchmedia.mo...

  1. its hook_library_info_alter() is currently deleting other "copies" of matchmedia. I think that should specifically only delete core/matchmedia + core/matchmedia.addListener, not do it for any extension.
  2. its hook_library_info_alter() could scan all library definitions and make it swap core/matchmedia for matchmedia/matchmedia 🙂 I'm not sure we truly want that, but it's definitely possible. Because today in Drupal 8, matchMedia.js is also only loaded if an asset library declares a dependency on core/matchmedia, and this would then make it automatically switch to matchmedia/matchmedia. So that'd automatically also address the "load on all pages" concern.

And one remark about https://git.drupalcode.org/project/matchmedia/blob/8.x-1.x/matchmedia.li...

  # Load before any dependents
  header: true

Looks like #4 introduced this. That is only necessary when you don't use the asset library system as intended: the asset library system in Drupal 8 specifically gained a dependency system, to ensure JS files are loaded in the right order. By stopping the loading on all pages and only loading it when it's a dependency, we can also remove that 🥳

zrpnr’s picture

zrpnr
he/him
Primary language English
Location UTC-7
CreditAttribution: zrpnr at Acquia commented
Status: Needs work » Needs review

Thanks for looking this over @Wim Leers!
#6.1 I kept the part that unsets the library but changed the check for the extension core only.
#6.2 This makes sense, I think the changes I made to the module address this - scanning over a library dependencies to replace core/matchmedia or core/matchmedia.addListener.

Updated link to the previous commit matchmedia.module
Current commit matchmedia.module

Also removed the header: true which I added to brute-force put this ahead of other libraries.

zrpnr’s picture

zrpnr
he/him
Primary language English
Location UTC-7
CreditAttribution: zrpnr at Acquia commented
FileSize
interdiff.txt1.68 KB

I realized I could post an interdiff instead of links to gitlab.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Needs review » Needs work
Issue tags: +Needs release manager review

This library should be always loaded as a dependency because it doesn't do anything by itself. For that reason I think the solution in #7 is correct 👍.

It seems like we're referencing to a non-existing library in the libraries.yml file: https://git.drupalcode.org/project/matchmedia/blob/8.x-1.x/matchmedia.li.... The library on the dependency should be matchmedia/matchmedia.

Also adding a tag for a release manager review since @xjm mentioned that this should be reviewed by a release manager.

zrpnr’s picture

zrpnr
he/him
Primary language English
Location UTC-7
CreditAttribution: zrpnr at Acquia commented

The library on the dependency should be matchmedia/matchmedia.

Fixed the incorrect camelCasing.
https://git.drupalcode.org/project/matchmedia/commit/000c28a784e98967f07...

zrpnr’s picture

zrpnr
he/him
Primary language English
Location UTC-7
CreditAttribution: zrpnr at Acquia commented
Status: Needs work » Needs review
zrpnr’s picture

zrpnr
he/him
Primary language English
Location UTC-7
CreditAttribution: zrpnr at Acquia commented
Title: Deprecate matchMedia and remove usages in core » Deprecate matchMedia
Issue summary: View changes
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Needs review » Needs work

The folder name is camelCased so that change should be reverted from https://git.drupalcode.org/project/matchmedia/commit/000c28a784e98967f07....

zrpnr’s picture

zrpnr
he/him
Primary language English
Location UTC-7
CreditAttribution: zrpnr at Acquia commented
Status: Needs work » Needs review

I changed case in the wrong place! Thanks for catching that, I reverted and fixed the dependency.
https://git.drupalcode.org/project/matchmedia/commit/e42cb046d49bf656c66...

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Needs review » Reviewed & tested by the community

The patch and the contrib project both look good! Confirmed that the deprecation warning message points to the right change record and tested manually that when I visit page with vertical tabs, the core matchmedia library has been replaced with one from the contrib project. 👏

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

I went to commit this but it does not apply, probably because it's modifying the same lines at the end of the trait that html5shiv did. Feel free to set it directly back to RTBC after rerolling.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll
FileSize
3086369-17.patch1.95 KB

👨‍🎨

  • xjm committed e61b786 on 9.0.x 
    Issue #3086369 by zrpnr, lauriii, Wim Leers: Deprecate matchMedia

  • xjm committed dad226e on 8.9.x 
    Issue #3086369 by zrpnr, lauriii, Wim Leers: Deprecate matchMedia

(...

  • xjm committed bd2b745 on 8.8.x 
    Issue #3086369 by zrpnr, lauriii, Wim Leers: Deprecate matchMedia

(...
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs release manager review

Committed and pushed to 9.0.x, 8.9.x, and 8.8.x, and published the change record. Thanks!

Status: Fixed » Closed (fixed)

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