Problem/Motivation

IE9 was dropped in 8.5.0 (see #2842298: [policy, no patch] Drop IE9 and IE10 support from Drupal 8.4.x).

responsive-image.html.twig still contains a workaround for IE9.

Proposed resolution

Remove unnecessary workaround and save a few bytes of HTML sent to everyone using this module.

CommentFileSizeAuthor
#7 3108071-7.patch2.73 KBtstoeckler
#7 3108071-4-7--interdiff.txt1.01 KBtstoeckler
#4 3108071-4.patch1.72 KBtstoeckler
#4 3108071-2-4--interdiff.txt870 byteststoeckler
#2 3108071-2.patch890 byteststoeckler
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tstoeckler created an issue. See original summary.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Status: Active » Needs review
FileSize
3108071-2.patch890 bytes

Here's a patch that changes the code in the responsive_image module. It applies in 9.0.x, 8.9.x and 8.8.x. I did not change the equivalent code in stable, as I am not sure what the current policy is on whether it is allowed to be modified and if so, in which branches.

idebr’s picture

idebr CreditAttribution: idebr at iO commented
Status: Needs review » Needs work

The template in the stable theme should be updated as well: core/themes/stable/templates/field/responsive-image.html.twig

Edit: per the backwards compatibility policy, themes markup is allowed to change https://www.drupal.org/core/d8-bc-policy#themes

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Status: Needs work » Needs review
FileSize
3108071-2-4--interdiff.txt870 bytes
3108071-4.patch1.72 KB

OK, fair enough. Thanks!

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented

ResponsiveImageFieldDisplayTest contains a test that will need updating:

      // Make sure the IE9 workaround is present.
      $this->assertRaw('<!--[if IE 9]><video style="display: none;"><![endif]-->');
      $this->assertRaw('<!--[if IE 9]></video><![endif]-->');

The last submitted patch, 2: 3108071-2.patch, failed testing. View results

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
FileSize
3108071-4-7--interdiff.txt1.01 KB
3108071-7.patch2.73 KB

Good catch, thanks!

The last submitted patch, 4: 3108071-4.patch, failed testing. View results

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs review » Reviewed & tested by the community

Looks good. I did a search and found some other references to old IE versions that we can probably clean up now, but nothing else related to this particular one.

  • lauriii committed 7640205 on 9.0.x 
    Issue #3108071 by tstoeckler, longwave, idebr: Remove IE9 workaround...

  • lauriii committed b90dfea on 8.9.x 
    Issue #3108071 by tstoeckler, longwave, idebr: Remove IE9 workaround...
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Version: 8.8.x-dev » 8.9.x-dev
Status: Reviewed & tested by the community » Fixed

Technically templates in the Stable theme are not supposed to be changed according to https://www.drupal.org/core/d8-frontend-bc-policy#themes. However, since this is just removing code related to supporting IE9 which isn't supported anymore, it seems reasonable to make this change there. This only impacts users of IE9 and some overly strict test assertions.

Committed 7640205 and pushed to 9.0.x and 8.9.x. Thanks!

Status: Fixed » Closed (fixed)

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