Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Since 8.4 release core drop support for IE 9-10 CR https://www.drupal.org/node/2897971
IE11 quirks are not used in "active-link" library https://caniuse.com/#search=classList
Proposed resolution
Deprecate classList asset library to allow for the removal in Drupal 9.
Remaining tasks
- review patch and commit
User interface changes
No
API changes
No
Data model changes
No
Release notes snippet
Deprecated classList polyfill.
Comment | File | Size | Author |
---|---|---|---|
#20 | interdiff_11-20.txt | 1.14 KB | zrpnr |
#20 | 2955842-20.patch | 1.63 KB | zrpnr |
Comments
Comment #2
andypostPatch removes usage, not sure it needs CR
The only usage in core - that proofs that core libraries does not affected by IE11 quirks
Comment #3
nod_looks good to me
Comment #4
alexpottI think this should be added to the IE 9-10 CR https://www.drupal.org/node/2897971 and also this needs it's own CR for 8.6.x because it is possible that someone has added a library that depends on drupal.active-link and is depending on its classlist dependency for ie 9/10 support. In that instance they need to add the classlist dependency to their library.
Comment #7
gappleI've created an issue on the IE9 compatibility module, which could alter the library to restore this dependency for sites that need it #3043990: classList JS library, and could provide its own classList library in the future when it's removed from core.
Comment #8
andypostFiled patch to test deprecation in #2951857-11: Update classList.js to 1.2.20180112
But I think it should be marked as duplicate of this issue
Comment #9
andypostPatch deprecates library and second one removes usage, needs deprecation test
Comment #11
zrpnrThe patch in #9 looks perfect, I just re-rolled it against 8.9.x and removed the single quotes.
The only other change is the link in the deprecation notice should point to the CR for this issue, I created a quick draft change record optimistically hoping this can still make it into 8.8, and updated that url in the patch.
Comment #12
bnjmnm\Drupal\Tests\Listeners\DeprecationListenerTrait::getSkippedDeprecations
?(better to have the contrib available that never gets used than not having a solution available)
Comment #13
zrpnr#12.1 I don't think it does- unless the library is a dependency you don't get that warning so there's no need to suppress it.
#12.2 Looked again for these specific IE11 limitations, now that #3074267: Replace jQuery UI position() with PopperJS is landed there's a use of classList.toggle with a 2nd parameter in core/modules/quickedit/js/views/EntityToolbarView.es6.js
A contrib module would be a good idea but we also don't want to break an existing feature for IE11.
#10.3 Made a quick note in the CR but I'll update again once the contrib module is released.
#10.4 Added a release note to the IS
Comment #14
bnjmnmLooks like some of my feedback from #12 is irrelevant as I just noticed that this polyfill is loaded conditionally
browsers: { IE: 'lte IE 9', '!IE': false }
, so this never polyfilled IE11 to begin with.With that in mind, it's also worth noting that Quickedit uses a second param with
classList.toggle()
. This was a recent addition to core as part of adding the PopperJS library. This may mean this polyfill can't be deprecated, and actually needs to be loaded on more browsers.Comment #15
bnjmnmA followup should be created to refactor the use of
classList.toggle()
with a second argument in Quickedit'sEntityToolBarView
to instead use a conditional that determines ifadd()
orremove()
is called, as opposed to using the param intoggle()
to determine if the class is added or removed.The IE11 specific stuff in the CR and RN I requested can be removed since we've since realized the polyfill is conditionally loaded.
Comment #16
zrpnrI agree, the polyfill is not currently loaded for IE11 so removing the polyfill won't make any difference.
The info about IE11 in the CR still seems useful, but I changed the wording to explain that the polyfill was only applied to IE9 and below.
Better to address that existing browser bug separately than to block this issue. Created #3089752: Follow-up to #3074267 Refactor use of classList in quickedit for browser compatibility
Setting this back to NR
Comment #17
bnjmnmEverything looks good, I also provided a patch for #3089752: Follow-up to #3074267 Refactor use of classList in quickedit for browser compatibility
Comment #18
alexpottDiscussed with @catch and we both feel this patch is good for 8.8.x BUT on close reading of #3086379: Deprecate html5shiv and remove usages in core that patch deprecated Html5Shiv which is only loaded in IE8 and less but did not remove core usages. Which is a different approach than this patch takes. Going to get @lauriii's opinion as frontend framework manager.
Comment #19
alexpottDiscussed further with @catch - let's add the deprecation but also add it to getSkippedDeprecations() ala #3086379: Deprecate html5shiv and remove usages in core and then file a follow-up issue to actually do the removal in Drupal 9.0.0
As @catch said
Comment #20
zrpnrThat makes sense- just like html5shiv and matchmedia and domready there can be a Drupal 9 issue to actually remove the usage while this patch just gets the deprecation in.
Restored dependency to active-link
Added the warning suppression since it would cause a warning in
LanguageSwitchingTest
.Comment #21
zrpnrUpdated IS and created #3090010: Remove classList in Drupal 9
Comment #22
andypostLet's get it in!
Comment #23
alexpottCommitted 592b6ae and pushed to 9.0.x. Thanks!
Committed and pushed d5fc5da747 to 8.9.x and 1bfa7c6dfc to 8.8.x. Thanks!
Now we can remove classlist completely from Drupal 9.
Comment #27
alexpott