Add HTML5shiv to Stable and Classy only

Forgot to remove the JS file :)

RTBC as per #1787012-139: [policy, no patch] Write D8 JS against ECMAScript 5. Prevent errors with feature detection (drop IE8 support) and the fact that this removes all traces of HTML5shiv.

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-.

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.

Absolutely makes sense :)

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

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...

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

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.

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.

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?

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?

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.

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?

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

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.

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

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

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

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

@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.

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

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

This should fix the failing tests.

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

@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.

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

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

@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

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

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.

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.

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.

@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.

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

Change record added, please review.

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.

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.

@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...

That is #2642122: Overriding already overridden libraries-override requires knowledge of previous libraries-overrides.

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.

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.

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

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

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.

@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

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

Re-rolled because #2806733: testMultipleLibrariesAreNotLoaded is not asserting anything has been fixed.

Think this is a good reroll, but needs eyes.

Thanks for the reroll @mpdonadio, I'd RTBC but I've touched this too much:)

Looks like this has now been resolved for D9 with #3089469: Remove html5shiv in Drupal 9, unless this is still worth backporting to D8?

Closing this as outdated given that #3089469: Remove html5shiv in Drupal 9 has been fixed.