Problem/Motivation

Core does not support IE8 anymore. The shiv is not required: http://caniuse.com/html5semantic

The motivation behind it was that core does not support IE8 any longer. If you're building a modern front-end, you'll drop support for IE8 - and maybe even IE9. This means there no need to include html5shiv. When creating a new Drupal 8 theme, html5shiv is always loaded; and it not documented on how to get rid of html5shiv in your theme.

Proposed resolution

The only people affected by the this patch are people who either opt-ed out of stable/classy explicitly with 'base theme: false' or extend our @Internal themes (Bartik and Seven)

Remove this library from core, leave it in Stable.

Remaining tasks

  1. Write documentation on how you can remove this library in your theme.
  2. Needs to be ported to the ie8 module contrib module,

patches welcome.

CommentFileSizeAuthor
#67 1993334-67.patch8.52 KBjoelpittet
#66 add_html5shiv_to_stable-1993334-66.patch9.07 KBjoelpittet
#66 interdiff.txt1.89 KBjoelpittet
#59 Untitled-1.png40.01 KBjoginderpc
#46 remove_html5shiv-1993334-46.patch7.93 KBjoelpittet
#46 interdiff.txt1.27 KBjoelpittet
#44 interdiff.txt870 bytesjoelpittet
#44 remove_html5shiv-1993334-44.patch8.24 KBjoelpittet
#42 remove_html5shiv-1993334-42.patch8.96 KBjoelpittet
#42 interdiff.txt931 bytesjoelpittet
#39 remove_html5shiv-1993334-39.patch8.33 KBjoelpittet
#39 interdiff.txt1.85 KBjoelpittet
#37 remove_html5shiv-1993334-37.patch7.07 KBjoelpittet
#27 core-js-remove-html5shiv-1993334-27.patch7.31 KBnod_
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 44,722 pass(es), 13,241 fail(s), and 12,393 exception(s). View
#25 IE8-WinXP-home-admin-withshiv.png66.21 KBnod_
#25 IE8-WinXP-content-admin-withshiv.png66.23 KBnod_
#25 IE8-WinXP-content-admin.png68.46 KBnod_
#25 IE8-WinXP-home-admin.png67.01 KBnod_
#25 IE8-WinXP-node1-anon.png68.05 KBnod_
#25 IE8-WinXP-home-anon.png67.03 KBnod_
#25 core-js-remove-html5shiv-1993334-25.patch7.32 KBnod_
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,745 pass(es). View
#3 core-js-remove-html5shiv-1993334-3.patch3.72 KBnod_
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch core-js-remove-html5shiv-1993334-3.patch. Unable to apply patch. See the log in the details link for more information. View
#1 core-js-remove-html5shiv-1993334-1.patch1.23 KBnod_
PASSED: [[SimpleTest]]: [MySQL] 55,977 pass(es). View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

nod_’s picture

Status: Active » Needs review
Issue tags: +JavaScript clean-up
FileSize
1.23 KB
PASSED: [[SimpleTest]]: [MySQL] 55,977 pass(es). View
Wim Leers’s picture

Status: Needs review » Needs work

Forgot to remove the JS file :)

nod_’s picture

Status: Needs work » Needs review
FileSize
3.72 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch core-js-remove-html5shiv-1993334-3.patch. Unable to apply patch. See the log in the details link for more information. View

:p

nod_’s picture

Assigned: Unassigned » Dries

Assigning to Dries since the patch removes a third party library.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
jessebeach’s picture

Removing this code provides us marginal benefit (2.3k saved?) at the expense of actually hobbling the experience of anonymous users on D8 sites using IE8. If we move this shiv to a non-required module, we pretty much make that module required for any practical purpose.

I'd rather see us keep it and make special concession to wrap it in a conditional so it only loads for IE8-.

nod_’s picture

It's already in conditional comments. For reference the shiv is only there to make IE8– recognize the new elements so that the CSS will apply to them.

Base themes usually have the shim too (zen, omega, mothership each ship with their own version). Since they don't use it anyway it doesn't make much difference if core ships with it or not.

Dries’s picture

Assigned: Dries » Unassigned
Status: Reviewed & tested by the community » Postponed
Issue tags: +revisit before beta

As far as I can tell, this library doesn't get in the way of work being done, and does make things a bit better for IE8 users. If so, let's leave it in for the time being and revisit it before the Drupal 8 release. I've added the appropriate tag.

Wim Leers’s picture

Absolutely makes sense :)

dcrocks’s picture

Wasn't there talk of including the shiv in modernizr? After it was added to core? New issue?

klonos’s picture

Base themes usually have the shim too (zen, omega, mothership each ship with their own version)...

Perhaps they wouldn't ship their own versions if we had one in core and made sure it is up to date. Or if #667058: Add a libraries folder with a README.txt in it to DRUPAL_ROOT / #1167496: Libraries API in core were implemented. Just saying...

nod_’s picture

Issue tags: +ie8

tag

catch’s picture

Assigned: Unassigned » Dries
Issue summary: View changes
Status: Postponed » Active

Eight months have passed, I think we could revisit this.

catch’s picture

Issue tags: -revisit before beta

Also we could remove this any time - we don't support IE8 so it's just a question of how much we don't support it.

jessebeach’s picture

Also we could remove this any time - we don't support IE8 so it's just a question of how much we don't support it.

We don't support IE8 on admin pages, but we have pledged to provide a good-enough experience for non-admin/content pages. Since our templates include HTML5 elements, we need to provide for their correct styling in browsers that don't recognize them and the shim does this.

  1. <section> element in comment-wrapper.html.twig.
  2. <article> element in node.html.twig, comment.html.twig, user.html.twig and book-node-export-html.html.twig.
  3. <time> element in datetime.html.twig.

In #2171071: Rename drupal_add_library() to _drupal_add_library() and remove its uses, the HTML5 shim library inclusion is pushed from theme_html to a preprocess. I just realized that this library inclusion should be moved to the html element definition so that a module could easily remove the shim by invoking hook_element_info_alter.

sun’s picture

Is there really no way to lazy-load the html5shiv for IE8 only, so that we do not have to load the code for all clients?

nod_’s picture

It's already the case. The shim is in conditional comment.

LewisNyman’s picture

Issue tags: +frontend

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 3: core-js-remove-html5shiv-1993334-3.patch, failed testing.

effulgentsia’s picture

Other than needing a reroll, any new thoughts on this related to #15? How broken is the IE8 experience already in HEAD for non-admin use? How much more broken does removing this library make it?

dcrocks’s picture

The html5shiv is pretty light weight. Why not just make it part of the modernizr load, which is part of core now, and get rid of the conditionals? It should have minimal impact on load times for modernizr and the issue can be reopened later.

nod_’s picture

"should" is not a responsible metric to use and those kind of tests can be expensive because they deal with the DOM. So #22 won't be a solution to get out of this.

@effulgentsia I don't have IE8 to test but the theory is that all JS will be broken (not a big problem) but more importantly all the styling for HTML5 elements will be lost.

effulgentsia’s picture

Re #23: yeah, my question is whether IE8 is already broken beyond repair for non-admin pages in HEAD due to no one paying attention to that. And I also don't have an IE8 environment handy to test for myself.

@jessebeach: it's been a year since you wrote #6 and 6 months since #15. What are your thoughts now?

nod_’s picture

Status: Needs work » Needs review
FileSize
7.32 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,745 pass(es). View
67.03 KB
68.05 KB
67.01 KB
68.46 KB
66.23 KB
66.21 KB

I downloaded IE VMs to test IE8 on WinXP (since that's what we're talking about, using compat mode is cheating).

Here are a few screenshots of what it looks like. Obviously no JS works but the website can still be browsed. I added the version with the Shim still here to compare. All the responsive stuff is out the window since IE8 doesn't support media queries and we decided that was fine a long time ago (adding respond.js fixes that but it's not core problem).

nod_’s picture

Also there is a new IE support policy: http://blogs.msdn.com/b/ie/archive/2014/08/07/stay-up-to-date-with-inter..., IE8 isn't even mentioned.

nod_’s picture

FileSize
7.31 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 44,722 pass(es), 13,241 fail(s), and 12,393 exception(s). View

reroll

Status: Needs review » Needs work

The last submitted patch, 27: core-js-remove-html5shiv-1993334-27.patch, failed testing.

mgifford’s picture

Issue tags: +Needs reroll

Patch fails:
Fatal error: Unsupported operand types in /home/s83a8db5196737bf/www/core/includes/common.inc on line 1853

Dries’s picture

Version: 8.0.x-dev » 8.1.x-dev
Assigned: Dries » Unassigned
Status: Needs work » Postponed

Given that it is conditionally loaded, it really doesn't really hurt anyone. I'd wait to remove this as too many people are still using IE8. Let's revisit this when we're working on 8.1.

joelpittet’s picture

Status: Postponed » Needs work

Should be able to start working on this as the 8.1.x branch is in sync.

joelpittet’s picture

Version: 8.1.x-dev » 9.x-dev

Actually maybe the total removal of this can be pushed to 9.x since we launched 8.0.x with it there (incase someone in contrib or custom depends on it) And we can remove the library from being used in core in 8.1.x?

There was an issue open for that #2409083: Remove html5shiv from being loaded

catch’s picture

Version: 9.x-dev » 8.2.x-dev

I still wouldn't rule out removing this in a minor release.

hass’s picture

Don't do this in 8.x, please!

joelpittet’s picture

@catch I still think this should be left in as it doesn't hurt much and could be painful to remove if people are using and at least @hass has indicated he may be using it, even though core is officially not supporting ie8 and below.

Is there a way we could deprecate it as a library? And leave it till 9.

joelpittet’s picture

I think we can remove it from bartik and seven though...

joelpittet’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
7.07 KB

Here's a reroll. Left the library in for classy but removed it for stable, seven, and bartik (crossing fingers on stable).

Status: Needs review » Needs work

The last submitted patch, 37: remove_html5shiv-1993334-37.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
1.85 KB
8.33 KB

This should fix the failing tests.

hass’s picture

Version: 8.2.x-dev » 9.x-dev

This would break thousands of themes. Don't do it in minor versions, please!!!

joelpittet’s picture

Version: 9.x-dev » 8.2.x-dev

@hass that is a bit hyperbolic.

This would break thousands of themes.

This is only a support for older browsers (<= IE8)

<!--[if lte IE 8]>

and drupal core explicitly/officially doesn't support < IE9. I've added backwards compatibility to classy so it won't be removed in the patch I posted yesterday. I'll add it to Stable theme as well because that is API theme. Seven and Bartik are not API and we can change them as we see fit.

IE8 and have 0.3%-1.3% market share globally:
http://www.w3schools.com/browsers/browsers_explorer.asp
http://gs.statcounter.com/#browser_version_partially_combined-ww-monthly...
Likely less depending on your market.

joelpittet’s picture

This won't break any site extending classy or stable (you are in the "wild west" if you opt-out of stable base theme).

lauriii’s picture

  1. +++ b/core/COPYRIGHT.txt
    @@ -24,9 +24,6 @@ Javascript
    -  HTML5 Shiv - Copyright (c) 2012  Alexander Farkas, Jonathan Neal, Paul Irish,
    -    and John-David Dalton
    -
    

    I think this is still needed somewhere since we ship HTML5 shiv as part of core

  2. +++ b/core/themes/classy/classy.info.yml
    @@ -6,7 +6,10 @@ version: VERSION
    +# @deprecated html5shiv library is scheduled to be removed in 9.0.x. It was
    +# added for backwards compatibility with the 8.0 release.
    
    +++ b/core/themes/stable/stable.info.yml
    @@ -7,6 +7,11 @@ core: 8.x
    +# @deprecated html5shiv library is scheduled to be removed in 9.0.x. It was
    +# added for backwards compatibility with the 8.0 release.
    

    These docs are not needed since stable & classy itself are acting as a BC layer

joelpittet’s picture

@lauriii I agree with item 1) I think that's correct too. I'd like to some how describe the intention behind those library additions(moves) to stable/classy. Is there another way we can describe this or maybe just having an open D9 issue would suffice?

Thanks for the review

The last submitted patch, 42: remove_html5shiv-1993334-42.patch, failed testing.

joelpittet’s picture

Fixed the tests and don't need to explicitly add it to classy because classy extends stable.

The last submitted patch, 44: remove_html5shiv-1993334-44.patch, failed testing.

lauriii’s picture

Status: Needs review » Reviewed & tested by the community

I don't think we need to include this info in the theme at all. At least it is not in the scope of this issue.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record

We definitely need a CR here. Given that stable is still loading the library if people have followed instructions when creating their theme if they extend that or classy there is no change. And seven and bartik are considered to be internal - so this is exactly the type of change we wanted to be able to implement in 8.x.x.

hass’s picture

The percentage is not my point. People build their themes and rely on core. It was released at 8.0 and works fine. Now you break features intentionally. This is not acceptable and need to wait until 9.x for removal. It was said in past that core is stable and does not change it's api's within a major version. Theme code may start failing with this change. Something not allowed to happen. The browsers I need to support is not your or my decission.

This can be seen as a general discussion. We can only extend api's, but cannot change them. Libs are the same like an api. If developers cannot rely on D8 core and must fear a break with every MINOR version, than drupal is no longer a good choice. It is already hard enough for most that every major version changes everything. Not every owner has the money to build it's website from scratch every 3-6 months or 3years and nobody expect things to break with a minor version upgrade!

You are destroying drupals reputation.

joelpittet’s picture

@hass please consider this patch barely changes anything. The only people affected by the this patch are people who either opt-ed out of stable explicitly with 'base theme: false' and the users of themes we've decided to manage ourselves and are not "API" (aka non BC breaking) themes.
Stable and Classy we have agreed we'd keep them from changes that would affect users. Core, Seven and Bartik are free to innovate and improve. Like alexpott mentioned in #49 this type of change is exactly why we separated bartik/seven from classy/stable.

joelpittet’s picture

Status: Needs work » Needs review
Issue tags: -Needs change record

https://www.drupal.org/node/2721335

Change record added, please review.

hass’s picture

Can look tomorrow into this first, but I think this breaks library overrides. I have overridden core html5shiv as it is outdated. Now the override does no longer override the stable library. Not tested yet, but I think this happens.

Cottser’s picture

The way this patch was done I'm 99% sure it won't affect your library overrides. It's not changing core/html5shiv at all, just not loading it in Bartik and Seven. However I think it also means the library is still loaded by core, for example when you use base theme: false in a theme .info.yml file.

joelpittet’s picture

@hass If library overrides is broken lets fix that in a different issue. I think you already have an issue and I've spent time on the weekend working on that issue...

joelpittet’s picture

Ah I was referring to #2642046: libraries-override does not update drupalSettings libraries array that I was working on that was created as a major issue of libraries-override.

hass’s picture

Ah, well that too, but potentially more troubles is causing the other. I never understood yet why the entries exist in the drupalSettings and I'm not aware what issues the bug #2642046: libraries-override does not update drupalSettings libraries array may cause. On #2642122: Overriding already overridden libraries-override requires knowledge of previous libraries-overrides I expierenced heavy malfunctions.

joginderpc’s picture

Status: Needs review » Needs work
FileSize
40.01 KB

Given patch in comment #46 is not resolving the issue. Attached screenshot please check.

html5shiv showing in source.

joelpittet’s picture

Status: Needs work » Needs review

@joginderpc that's an unrelated issue, see my fix for that in #2642046-4: libraries-override does not update drupalSettings libraries array

Wim Leers’s picture

Status: Needs review » Needs work

and drupal core explicitly/officially doesn't support < IE9.

… for its admin themes/functionality.

There's no reason to intentionally break things in IE8.

However:

The only people affected by the this patch are people who either opt-ed out of stable explicitly with 'base theme: false'

this makes it acceptable.


+++ b/core/modules/system/src/Tests/Render/AjaxPageStateTest.php
@@ -84,18 +68,7 @@ public function testHtml5ShivIsNotLoaded() {
   public function testMultipleLibrariesAreNotLoaded() {
     $this->drupalGet('node',
-      array(
-        "query" =>
-          array(
-            'ajax_page_state' => array(
-              'libraries' => 'core/html5shiv,core/drupalSettings'
-            )
-          )
-      )
-    );
-    $this->assertNoRaw(
-      '/core/assets/vendor/html5shiv/html5shiv.min.js',
-      'The html5shiv library from core should be excluded from loading.'
+      ['query' => ['ajax_page_state' => ['libraries' => 'core/drupalSettings']]]
     );

This change is wrong. Now this is no longer testing the excluding of multiple libraries.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

joelpittet’s picture

Title: Remove HTML5shiv » Add HTML5shiv to Stable and Classy only
joelpittet’s picture

Issue summary: View changes
joelpittet’s picture

@Wim Leers re #61thanks for checking that, while trying to fix that I found that test isn't currently testing anything because the result of drupalGet('node') is returning a 404 and it's a negative assertion so it's always true. #2806733: testMultipleLibrariesAreNotLoaded is not asserting anything

joelpittet’s picture

Status: Needs work » Needs review
FileSize
1.89 KB
9.07 KB

This may need to be re-rolled after #65 but should cover that too.

joelpittet’s picture

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

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.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.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.