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
- Write documentation on how you can remove this library in your theme.
Add this to your theme’sinfo.yml
filelibraries-override: core/html5shiv: false
- Needs to be ported to the ie8 module contrib module,
patches welcome.
Comment | File | Size | Author |
---|---|---|---|
#71 | 1993334-71.patch | 8.65 KB | mpdonadio |
#67 | 1993334-67.patch | 8.52 KB | joelpittet |
#66 | add_html5shiv_to_stable-1993334-66.patch | 9.07 KB | joelpittet |
#66 | interdiff.txt | 1.89 KB | joelpittet |
#59 | Untitled-1.png | 40.01 KB | joginderpc |
Comments
Comment #1
nod_Comment #2
Wim LeersForgot to remove the JS file :)
Comment #3
nod_:p
Comment #4
nod_Assigning to Dries since the patch removes a third party library.
Comment #5
Wim LeersRTBC 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.
Comment #6
jessebeach CreditAttribution: jessebeach commentedRemoving 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-.
Comment #7
nod_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.
Comment #8
Dries CreditAttribution: Dries commentedAs 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.
Comment #9
Wim LeersAbsolutely makes sense :)
Comment #10
dcrocks CreditAttribution: dcrocks commentedWasn't there talk of including the shiv in modernizr? After it was added to core? New issue?
Comment #11
klonosPerhaps 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...
Comment #12
nod_tag
Comment #13
catchEight months have passed, I think we could revisit this.
Comment #14
catchAlso 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.
Comment #15
jessebeach CreditAttribution: jessebeach commentedWe 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.
<section>
element incomment-wrapper.html.twig
.<article>
element innode.html.twig
,comment.html.twig
,user.html.twig
andbook-node-export-html.html.twig
.<time>
element indatetime.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 invokinghook_element_info_alter
.Comment #16
sunIs 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?
Comment #17
nod_It's already the case. The shim is in conditional comment.
Comment #18
LewisNymanComment #21
effulgentsia CreditAttribution: effulgentsia commentedOther 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?
Comment #22
dcrocks CreditAttribution: dcrocks commentedThe 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.
Comment #23
nod_"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.
Comment #24
effulgentsia CreditAttribution: effulgentsia commentedRe #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?
Comment #25
nod_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).
Comment #26
nod_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.
Comment #27
nod_reroll
Comment #29
mgiffordPatch fails:
Fatal error: Unsupported operand types in /home/s83a8db5196737bf/www/core/includes/common.inc on line 1853
Comment #30
Dries CreditAttribution: Dries commentedGiven 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.
Comment #31
joelpittetShould be able to start working on this as the 8.1.x branch is in sync.
Comment #32
joelpittetActually 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
Comment #33
catchI still wouldn't rule out removing this in a minor release.
Comment #34
hass CreditAttribution: hass commentedDon't do this in 8.x, please!
Comment #35
joelpittet@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.
Comment #36
joelpittetI think we can remove it from bartik and seven though...
Comment #37
joelpittetHere's a reroll. Left the library in for classy but removed it for stable, seven, and bartik (crossing fingers on stable).
Comment #39
joelpittetThis should fix the failing tests.
Comment #40
hass CreditAttribution: hass commentedThis would break thousands of themes. Don't do it in minor versions, please!!!
Comment #41
joelpittet@hass that is a bit hyperbolic.
This is only a support for older browsers (<= IE8)
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.
Comment #42
joelpittetThis won't break any site extending classy or stable (you are in the "wild west" if you opt-out of stable base theme).
Comment #43
lauriiiI think this is still needed somewhere since we ship HTML5 shiv as part of core
These docs are not needed since stable & classy itself are acting as a BC layer
Comment #44
joelpittet@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
Comment #46
joelpittetFixed the tests and don't need to explicitly add it to classy because classy extends stable.
Comment #48
lauriiiI 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.
Comment #49
alexpottWe 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.
Comment #50
hass CreditAttribution: hass commentedThe 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.
Comment #51
joelpittet@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.
Comment #52
joelpittethttps://www.drupal.org/node/2721335
Change record added, please review.
Comment #53
hass CreditAttribution: hass commentedCan 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.
Comment #54
star-szrThe 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.Comment #55
joelpittet@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...
Comment #56
hass CreditAttribution: hass commentedThat is #2642122: Overriding already overridden libraries-override requires knowledge of previous libraries-overrides.
Comment #57
joelpittetAh 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.
Comment #58
hass CreditAttribution: hass commentedAh, 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.
Comment #59
joginderpc CreditAttribution: joginderpc at Srijan | A Material+ Company commentedGiven patch in comment #46 is not resolving the issue. Attached screenshot please check.
Comment #60
joelpittet@joginderpc that's an unrelated issue, see my fix for that in #2642046-4: libraries-override does not update drupalSettings libraries array
Comment #61
Wim Leers… for its admin themes/functionality.
There's no reason to intentionally break things in IE8.
However:
this makes it acceptable.
This change is wrong. Now this is no longer testing the excluding of multiple libraries.
Comment #63
joelpittetComment #64
joelpittetComment #65
joelpittet@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
Comment #66
joelpittetThis may need to be re-rolled after #65 but should cover that too.
Comment #67
joelpittetRe-rolled because #2806733: testMultipleLibrariesAreNotLoaded is not asserting anything has been fixed.
Comment #71
mpdonadioThink this is a good reroll, but needs eyes.
Comment #72
joelpittetThanks for the reroll @mpdonadio, I'd RTBC but I've touched this too much:)
Comment #73
joelpittetComment #78
gappleLooks like this has now been resolved for D9 with #3089469: Remove html5shiv in Drupal 9, unless this is still worth backporting to D8?
Comment #79
lauriiiClosing this as outdated given that #3089469: Remove html5shiv in Drupal 9 has been fixed.