Problem/Motivation

When there are more than 31 stylesheets on a page, in order to support IE9 CssCollectionRenderer outputs them within an @import statement instead of as <link> elements.

This affects Content-Security-Policy rules, requiring 'unsafe-inline' in order for the CSS to not be blocked by the browser.

Enabling CSS aggregation should reduce the number of stylesheets on the page below the threshold of 31 items such that they can be output as <link> elements, so production sites should not be affected if this particular support for IE9 was removed.

Proposed resolution

Remove the @import workaround so that CssCollectionRenderer always outputs stylesheets in <link> elements.

Create a contrib module (ie9 / legacyie) that provides the current CssCollectionRenderer from 8.3

Release note snippet

In Internet Explorer 9 and earlier, there is a limit of 31 style sheets per page. Drupal dropped support for Internet Explorer 9 and 10 in 8.4.0, but Drupal 8.5 and 8.6 retained a workaround to allow 32 or more stylesheets to be included. This workaround has been removed in 8.7. Sites requiring Internet Explorer 9 support should enable CSS aggregation (preferred) or install the IE9 Compatibility contributed module.

CommentFileSizeAuthor
#22 drupal-2897408-22-interdiff.patch3.46 KBgapple
#22 drupal-2897408-22-remove-css-ie-hack.patch32.77 KBgapple
#20 drupal-2897408-19-interdiff.txt20.4 KBgapple
#20 drupal-2897408-19-remove-css-ie-hack.patch29.17 KBgapple
#5 drupal-2897408-05-remove-css-ie-hack.patch9.14 KBcburschka
#4 drupal-2897408-04-remove-css-ie-hack.interdiff.patch2.67 KBcburschka
#4 drupal-2897408-04-remove-css-ie-hack.patch9.14 KBcburschka
#3 drupal-2897408-03-remove-css-ie-hack.patch7.05 KBcburschka
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

gapple created an issue. See original summary.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented

I noticed we were still doing this when working on #1014086: Stampedes and cold cache performance issues with css/js aggregation, it'd be great to remove that workaround, and production sites should still be using aggregation so it doesn't actually break anything visitor-facing anyway.

cburschka’s picture

cburschka
they
CreditAttribution: cburschka at PwC's Experience Center commented
Status: Active » Needs review
FileSize
drupal-2897408-03-remove-css-ie-hack.patch7.05 KB

This removes the hack, but doesn't update the corresponding unit test yet.

cburschka’s picture

cburschka
they
CreditAttribution: cburschka at PwC's Experience Center commented
FileSize
drupal-2897408-04-remove-css-ie-hack.patch9.14 KB
drupal-2897408-04-remove-css-ie-hack.interdiff.patch2.67 KB

Also removed the explanation of the hack from the phpdoc.

cburschka’s picture

cburschka
they
CreditAttribution: cburschka at PwC's Experience Center commented
FileSize
drupal-2897408-05-remove-css-ie-hack.patch9.14 KB

The last submitted patch, 3: drupal-2897408-03-remove-css-ie-hack.patch, failed testing. View results

The last submitted patch, 4: drupal-2897408-04-remove-css-ie-hack.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 5: drupal-2897408-05-remove-css-ie-hack.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia at Acquia commented

+1 for doing this in 8.5. For 8.4, https://www.drupal.org/node/2897971 currently says:

Drupal 8.4 is following suit and is also no longer officially supporting IE9 or IE10 but has retained existing workarounds that will be removed in future releases of Drupal 8.

Should we honor that for this issue? In which case, should we do something like rename the existing CssCollectionRenderer to CssCollectionRendererLegacy or DeprecatedCssCollectionRenderer and have some mechanism for sites to opt into it (or out of it)?

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia at Acquia commented

production sites should still be using aggregation so it doesn't actually break anything visitor-facing anyway

Alternatively, should we update that change record to narrow "has retained existing workarounds" to something like "has retained existing workarounds for production configurations"?

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented

I think #10 is reasonable for this issue.

gapple’s picture

gapple
he/they
Primary language English
CreditAttribution: gapple as a volunteer commented

I've created https://www.drupal.org/project/ie9, which decorates core's CssCollectionRenderer to handle output when there are 32 or more stylesheets.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Issue tags: +Needs change record

I agree on doing this for 8.5.x. I'm not sure it's worth backporting anything to 8.4.x for it.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented

Also, thanks @gapple for #12. This would be the first actually hard break for IE9 (vs. just passively not adding workarounds or testing against IE9 anymore). I know @droplet has been concerned about the impact of dropping IE9 support for users in China, so that project is a great compromise.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Title: Remove IE9 support from CssCollectionRenderer » Remove IE9 support from CssCollectionRenderer and provide it in contrib instead

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

gapple’s picture

gapple
he/they
Primary language English
CreditAttribution: gapple as a volunteer commented
Status: Needs work » Needs review

Updated patch

- Removes additional comments on the class regarding LINK versus STYLE tags that is no longer relevant
- Updated tests:
-- Change expected order of #attributes keys
-- Test that 1, 31, and 32 stylesheets are always rendered with LINK elements; remove IE-specific tests for STYLE grouping.

gapple’s picture

gapple
he/they
Primary language English
CreditAttribution: gapple as a volunteer commented
FileSize
drupal-2897408-19-remove-css-ie-hack.patch29.17 KB
drupal-2897408-19-interdiff.txt20.4 KB

Status: Needs review » Needs work

The last submitted patch, 20: drupal-2897408-19-remove-css-ie-hack.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

gapple’s picture

gapple
he/they
Primary language English
CreditAttribution: gapple as a volunteer commented
Status: Needs work » Needs review
FileSize
drupal-2897408-22-remove-css-ie-hack.patch32.77 KB
drupal-2897408-22-interdiff.patch3.46 KB

Another test was dependent on the order of attributes

Status: Needs review » Needs work

The last submitted patch, 22: drupal-2897408-22-interdiff.patch, failed testing. View results

gapple’s picture

gapple
he/they
Primary language English
CreditAttribution: gapple as a volunteer commented
Status: Needs work » Needs review
Issue tags: +Content Security Policy

I forgot to not name the interdiff file .patch. Updating status since the actual patch was successful.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet at The University of British Columbia commented
Status: Needs review » Reviewed & tested by the community

So much cruft removal, I love it and thanks for the ie9 decorator! Let's ensure that is mentioned in the CR

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Issue tags: +8.7.0 release notes

This is something to mention in 8.7.0 release notes. Still needs a change record.

Patch looks great so +1 to the RTBC.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Status: Reviewed & tested by the community » Needs work

Again, +1 RTBC, but we need a change record here

gapple’s picture

gapple
he/they
Primary language English
CreditAttribution: gapple as a volunteer commented
Status: Needs work » Needs review
Issue tags: -Needs change record

Created change record: CSS Rendering no longer includes workaround for Internet Explorer 9

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet at The University of British Columbia commented
Status: Needs review » Reviewed & tested by the community

Back to RTBC, thanks for adding a CR @gapple

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed c724390 and pushed to 8.7.x. Thanks!

  • catch committed c724390 on 8.7.x 
    Issue #2897408 by cburschka, gapple, xjm, effulgentsia: Remove IE9...

Status: Fixed » Closed (fixed)

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

kattekrab’s picture

kattekrab CreditAttribution: kattekrab as a volunteer commented
Issue tags: +Needs release note

Adding "needs release note" tag.

As this is listed in the "important update info" section of the alpha release notes for Drupal 8.7, we need to let people know what has changed.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Issue summary: View changes
Issue tags: -Needs release note

Thanks @kattekrab! Adding a snippet based on the CR.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Issue summary: View changes
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Issue summary: View changes
gapple’s picture

gapple
he/they
Primary language English
CreditAttribution: gapple as a volunteer commented
Issue tags: +IE9