Problem/Motivation

IE11 is no longer supported in Drupal 10. This issue is a child of #3252084: [meta] Remove support for Internet Explorer.

I'm starting the priority of this issue as Major, as this issue is going to be the primary blocker for modernizing the CSS in both Claro and Olivero.

Proposed resolution

The process:

  • Remove Internet Explorer from the browserlist at https://git.drupalcode.org/project/drupal/-/blob/10.0.x/core/package.jso...
  • Split out Olivero's media query variables from its CSS variables file
  • Ensure that Olivero's CSS is importing the new media query file
  • Look through all of core's *.pcss.css files (probably all within the Claro and Olivero themes) and remove the @import statements where the CSS variables are being included.
  • Recompile

Testing instructions

  1. Look through the code and make sure the changes make sense. It might be easier to look through the actual commits (which are broken up pretty logically).
  2. Pull down the code and start looking browsing through and testing the install. You'll want to pay special attention to the Claro and Olivero themes.

Tugboat URL to test this (ping mherchel in slack for username/password) is https://3253148-update-browserlist-buqkhs05cxxngixj6uateaybuk7fh4ix.tugb...

CommentFileSizeAuthor
#13 d10-weird.png279.19 KBmherchel

Issue fork drupal-3253148

Command icon Show commands

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

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

About issue forks

Comments

mherchel created an issue. See original summary.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Title: Remove IE from browserlist recompile assets » Remove IE from core's browserlist and recompile assets

mherchel opened merge request !1494

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Status: Active » Needs review

MR created. This is pretty straightforward:

  1. I removed "last 1 Explorer major version", from the browserslist section of core'spackage.json
  2. Ran yarn build
  3. 🎉🎉🎉

🤞🤞🤞

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Issue summary: View changes

Opened followup #3253153: Remove unneeded @import statements from all of core's *.pcss.cssfiles

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Issue summary: View changes
mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Status: Needs review » Needs work
droplet’s picture

droplet commented

I think it should #3253153: Remove unneeded @import statements from all of core's *.pcss.cssfiles first and then go back to remove IE.

Or do both together.
Or clone the old config and keep IE for above 2 themes only and remove them from these 2 later.

Is it postcss limit? I'm surprised how it is imported in CORE now. (don't it import the base CSS from the build process?)

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

As far as I can see we can remove the ES6 transpiling step now if we wanted to. After applying this patch and picking random pairs of .es6.js and .js files shows that the only differences now seem to be that the .js files have the comments removed and other non-functional changes around whitespace or redundant brackets.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Title: Remove IE from core's browserlist and recompile assets » Remove IE from core's browserlist, remove non-essential CSS importing and recompile assets
Issue summary: View changes

Or do both together

In Drupal Slack https://drupal.slack.com/archives/C0D5GJZ8B/p1638964949036100?thread_ts=...

@lauriii agrees in Drupal Slack

I think it would be easier to handle that in one issue because otherwise to review those, I would at least want to apply patches from both issues

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented

A bit of investigation into that test error. The test is at /core/tests/Drupal/KernelTests/Core/Theme/ConfirmClassyCopiesTest.php and was introduced in #3106600: Decouple Classy libraries from Umami. Evidently the test confirms that the copy of Classy included in the Umami demo hasn't changed.

I'm guessing we just need to update the hashes for the assets that have changed, which is
profiles/demo_umami/themes/umami/js/classy/media_embed_ckeditor.theme.js

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Status: Needs work » Needs review
mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Status: Needs review » Needs work
StatusFileSize
new d10-weird.png279.19 KB

Claro looks good, but Olivero is all janky with this latest patch.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented

Ahhh... this is because Olivero makes use of https://github.com/postcss/postcss-custom-media, which is included in PostCSS Preset ENV.

The media queries still need to be imported.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Status: Needs work » Needs review

Last issue is fixed.

If the tests pass (🤞🤞🤞), this should be good to go.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Issue summary: View changes

Tests are passing! Updating the summary.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

The Gitlab interface is unusable for me on an MR of this size, so reviewing in a dreditor style.

diff --git a/core/modules/ckeditor5/js/ie11.filter.warnings.js b/core/modules/ckeditor5/js/ie11.filter.warnings.js
index 2afbf8b6a774571e19fe91423d0d6d55f35e450b..4b643a633030c2e74d5d13535d42fbdd96f6acac 100644
--- a/core/modules/ckeditor5/js/ie11.filter.warnings.js
+++ b/core/modules/ckeditor5/js/ie11.filter.warnings.js

Amusing that we removing IE11 compatibility from an IE11 compatibility warning, we need a separate issue to remove this entirely :)

--- a/core/themes/claro/css/components/form--checkbox-radio--ie.pcss.css
+++ b/core/themes/claro/css/components/form--checkbox-radio--ie.pcss.css
@@ -3,8 +3,6 @@
  * Checkbox and radio input elements styles for IE11 and below.
  */

Seems like we can drop this file entirely?

--- a/core/themes/claro/css/layout/card-list.pcss.css
+++ b/core/themes/claro/css/layout/card-list.pcss.css

   /* Using 100% as base causes issues in IE11. */

Do we need to deal with this here or in a followup? There are a few more comments in the CSS that refer to IE11 workarounds.

--- /dev/null
+++ b/core/themes/olivero/css/base/media-queries.css
@@ -0,0 +1,19 @@
+/*
+ * DO NOT EDIT THIS FILE.
+ * See the following change record for more information,
+ * https://www.drupal.org/node/3084859
+ * @preserve
+ */
+
+/*
+ * Media query breakpoints.
+ * Processed by postcss/postcss-custom-media.
+ */
+
+/* Navigation related breakpoints */
+
+/* Grid related breakpoints */
+
+/* Grid shifts from 6 to 14 columns. */
+
+/* Width of the entire grid maxes out. */

This compiled CSS file only contains comments?

--- a/core/themes/olivero/css/base/utility.css
+++ b/core/themes/olivero/css/base/utility.css
@@ -10,6 +10,19 @@
  * Utility classes.
  */
 
+/*
+ * Media query breakpoints.
+ * Processed by postcss/postcss-custom-media.
+ */
+
+/* Navigation related breakpoints */
+
+/* Grid related breakpoints */
+
+/* Grid shifts from 6 to 14 columns. */
+
+/* Width of the entire grid maxes out. */

These comments are being added anywhere media-queries.pcss.css is being imported, but they aren't very helpful.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented

These are all good points. Responses below:

Amusing that we removing IE11 compatibility from an IE11 compatibility warning, we need a separate issue to remove this entirely :)

I have a placeholder in #3252084: [meta] Remove support for Internet Explorer for Claro specific issues to remove IE support.

Do we need to deal with this here or in a followup? There are a few more comments in the CSS that refer to IE11 workarounds.

I have an issue to do Olivero specific IE removal at #3253156: [meta] Remove IE11 Support from Olivero that will cover this.

This compiled CSS file only contains comments?

Yeah, that's the nature of our build system. It looks for any *.pcss.css file and will compile it. This has been the case since the build system was introduced. You can see similar situation on 9.4.x head at https://git.drupalcode.org/project/drupal/-/blob/9.4.x/core/themes/olive...

These comments are being added anywhere media-queries.pcss.css is being imported, but they aren't very helpful.

I totally agree. But once again, this is part of the build system and short of modifying that (which is out of scope IMO), I don't know what else we can do short of removing the source comments.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Maybe we could use the PostCSS Custom Media "importFrom" option to avoid needing the import at all, and then this would sidestep the comments and empty file issues as well? https://github.com/postcss/postcss-custom-media#importfrom - if we don't do this here perhaps worth a followup to investigate?

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented

That's a fantastic idea.

Followup created at #3253380: Use PostCSS Custom Media's importFrom option to import breakpoints into core's CSS. The build step is global for all of core's themes (and CSS), so we'd need to namespace the media variables, but it is doable!

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Issue summary: View changes

Updating testing instructions with Tugboat URL: https://3253148-update-browserlist-buqkhs05cxxngixj6uateaybuk7fh4ix.tugb...

ping me mherchel in Drupal Slack for the admin credentials so you can QA Claro.

ckrina’s picture

ckrina
she/her
Primary language Catalan
Location Barcelona
commented

I'm not able to see any more issues than the ones you've already opened a follow-up for and I haven't find any issue on my manual tests locally. So +1 to this! Bye IE11 👋

ckrina’s picture

ckrina
she/her
Primary language Catalan
Location Barcelona
commented
Status: Needs review » Reviewed & tested by the community

Moving to RTBC.

saschaeggi’s picture

saschaeggi
he/him
commented

Awesomesauce. RIP IE11 – we won't miss you at all.

xjm’s picture

xjm
she/her
Primary language English
commented
Issue tags: +Drupal 10, +pre-alpha target

Discussed with @catch. Since this issue affects Drupal 10's platform requirements, it can be committed to 10.0.x prior to the release of 10.0.0-alpha1.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

I was pondering whether this needs a change record - but I think the IE dropping bit of the release note should be linked to the meta where all the related issues are collected so not tagging this issue yet.

Also given out transpilation step is now doing nothing should we discuss removing it - or should we keep it so we can make similar progress in the future? I guess we should open an issue to discuss.

Committed f1e5720 and pushed to 10.0.x. Thanks!

  • alexpott committed f1e5720 on 10.0.x 
    Issue #3253148 by mherchel, longwave, ckrina, droplet: Remove IE from...
mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented

Woot!

Our CSS transpilation is still doing a number of things (CSS custom media, indentation, etc), but we may be able to remove the JavaScript build step.

Status: Fixed » Closed (fixed)

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