Opened 6 months ago
Closed 3 months ago
#63821 closed enhancement (fixed)
Proposal: Drop support for IE conditional scripts and styles
| Reported by: |
|
Owned by: |
|
|---|---|---|---|
| Milestone: | 6.9 | Priority: | normal |
| Severity: | normal | Version: | 4.8 |
| Component: | General | Keywords: | has-patch has-unit-tests needs-testing has-test-info reviewing has-dev-note |
| Focuses: | javascript, css | Cc: |
Description
WordPress supports for HTML "conditional" comments in scripts and styles. These were typically used to target specific Internet Explorer versions as documented on this codex page. For example:
<!--[if lte IE 7]>
<link rel="stylesheet" href="/ie7.css" media="screen" type="text/css" />
<![endif]-->
Microsoft dropped support for conditional comments in IE10 when it was released 12 years ago, making IE9 the final version to support conditional comments.
As far as I can tell, global usage of IE9 is basically 0.
I propose support for conditionals scripts and styles be deprecated and conditional script and styles not printed to the page at all.
Any modern browser treats these script tags as a single HTML comment, so these conditional comments should never used by any post-IE9 browser. This HTML constructs the following tree:
<!DOCTYPE html> <body> <!--[if]> <script>alert('script')</script> <![endif]-->
├─DOCTYPE: html
└─HTML
├─HEAD
└─BODY
├─#text
└─#comment [if]> <script>alert('script')</script> <![endif]
There is no script tag present in that page and it seems that there's no place for these conditional script and style tags in today's web.
Change History (48)
This ticket was mentioned in PR #9497 on WordPress/wordpress-develop by @joedolson.
6 months ago
#4
- Keywords has-patch has-unit-tests added
Returns false if conditional scripts or styles are identified; updates comments; removes files that were only loaded conditionally.
Trac ticket: https://core.trac.wordpress.org/ticket/63821
#5
@
6 months ago
- Version set to 4.8
I investigated a bit to ensure these browsers should not be supported.
According to this make post, IE8, IE9 and IE10 support was dropped in WordPress 4.8.
IE10 removed support for conditional comments. No browsers supported by WordPress use conditional comments.
@jonsurrell commented on PR #9497:
6 months ago
#6
I scanned this and didn't t see any _doing_it_wrong_ or similar notices in this change. Did you add any? Should some notice be included?
What if each class handed some notice and ignored conditionals in ::add_data()? They could avoiding adding conditional scripts/styles at all. It would save handling conditional scripts and styles when printing.
We could consider doing that in WP_Dependencies::add_data() as well, although there's a small risk of affecting other subclasses. I scanned a quick wpdirectory search and it didn't seem likely to be a problem, but it would be safer to do it in the script and style classes.
#7
follow-up:
↓ 10
@
6 months ago
Isn't this feature still used in many themes, though? Shouldn't it be removed from themes first?
#8
@
6 months ago
@jonsurrell I considered a doing it wrong or deprecation notice, but was undecided whether it was necessary - with this implementation, we're just ignoring the information. I think that's a good point for discussion.
@siliconforks there's actually no need to remove this from themes first; the implementation doesn't require it. Any scripts/styles with conditional flags are ignored; themes can remove them on their own schedule.
Note: I'm aware the tests are failing, in the initial commit I left them partially modified to ensure that old tests would fail as expected.
Adding an early exit in ::add_data() makes sense to me.
@joedolson commented on PR #9497:
6 months ago
#9
I was thinking about handling this in ::add_data(), but if we exit before adding the data, then the related script will still be handled, and will be printed to the page, just without conditionals. Handling it in do_item() allows us to also ignore the script itself and its dependencies.
I guess we could de-register the handle in add_data(), but that feels dirty, somehow.
Can still handle a deprecation notice in add_data(), though.
#10
in reply to:
↑ 7
@
6 months ago
Replying to siliconforks:
Isn't this feature still used in many themes, though? Shouldn't it be removed from themes first?
It certainly should be removed from themes, but it shouldn't be necessary to do it first like @joedolson said. That work can happen at any time and in parallel with the Core changes.
@joedolson commented on PR #9497:
6 months ago
#11
The suggested change to test_wp_script_add_data_with_conditional_key() didn't work, because the return value of wp_script_add_data( 'test-only-conditional', 'conditional', 'gt IE 7' ) isn't falsey; need to actually test the outcome.
@jonsurrell commented on PR #9497:
6 months ago
#12
I guess we could de-register the handle in
add_data(), but that feels dirty, somehow.
Setting a script as conditional is akin to de-registering it, nothing will happen with it.
Removing the conditional scripts dependencies is similar, and perhaps riskier. A script could be set to conditional and later have its conditional removed, in which case it has lost its associated dependencies and is likely broken in some way. In that sense, de-registering the script may be preferable and the formerly conditional script can be re-registered without the conditional.
Can still handle a deprecation notice in `add_data(), though.
I do think a deprecation notice there would be good.
@joedolson commented on PR #9497:
6 months ago
#15
The cases surrounding removing dependencies could definitely get complicated. Would like to get some additional opinions here, I think.
#16
@
6 months ago
- Keywords 2nd-opinion added
There's a question of how to handle conditional scripts.
The proposed implementation keeps the script and adds the data to flag the script as conditional. Conditional scripts are prevented from being printed. The conditional script's dependency array must also be emptied so that its dependencies are not printed.
An alternative would be to de-register a script when it is marked as "conditional".
Setting a script as conditional is analogous to de-registering it, nothing should happen with it.
Removing the conditional scripts dependencies is similar, and perhaps riskier. A script could be set to conditional and later have its conditional removed, in which case it has lost its associated dependencies and is likely broken in some way. In that sense, de-registering the script may be preferable and the formerly conditional script can be re-registered without the conditional.
More feedback would be appreciated.
@jonsurrell commented on PR #9497:
5 months ago
#17
I pushed a change to add @expectedDeprecation annotations to the tests for fix the failures.
This ticket was mentioned in PR #9765 on WordPress/wordpress-develop by @jonsurrell.
5 months ago
#18
Remove conditional scripts and styles from themes. They target browsers that are not supported by WordPress and should never be used. The conditional scripts and styles are rendered as plain HTML comments by any supported browser.
Microsoft dropped support for conditional comments in IE10?redirectedfrom=MSDN) when it was released 12 years ago, making IE9 the final version to support conditional comments.
Any modern browser treats these script tags as a single HTML comment, so these conditional comments should never used by any post-IE9 browser.
- Remove twentyfifteen conditional styles
- Remove twentytwelve conditional styles
- Remove twentysixteen conditional scripts and styles
- Remove twentyfourteen conditional styles
- Remove twentythirteen conditional styles
- Remove twentyseventeen conditional scripts and styles
Companion to https://github.com/WordPress/wordpress-develop/pull/9497 that removes support for conditional scripts and styles from Core.
Trac ticket: https://core.trac.wordpress.org/ticket/63821
@sabernhardt commented on PR #9765:
5 months ago
#19
@jonsurrell commented on PR #9765:
5 months ago
#20
Thanks for linking those @sabernhardt 👍 I'll close this, it's duplicated a lot of your work.
#21
@
5 months ago
- Keywords needs-testing has-test-info added; 2nd-opinion removed
- Owner set to joedolson
- Status changed from new to accepted
Testing Info:
Install the plugin at https://gist.github.com/joedolson/fc5a5da9c7cb23e44257128e6a6564f8
The plugin comments include the test expectations.
This ticket was mentioned in Slack in #core by welcher. View the logs.
5 months ago
#23
@
5 months ago
Test Report
Description
This report validates whether the indicated patch works as expected.
Patch tested: https://github.com/WordPress/wordpress-develop/pull/9497
Environment
- WordPress: 6.9-alpha-60093-src
- PHP: 8.2.29
- Server: nginx/1.29.1
- Database: mysqli (Server: 8.4.6 / Client: mysqlnd 8.2.29)
- Browser: Chrome 139.0.0.0
- OS: Linux
- Theme: Twenty Twenty-Five 1.3
- MU Plugins: None activated
- Plugins:
- Test 63821 1.0.0
- Test Reports 1.2.0
Actual Results
- ✅ Issue resolved with patch.
Based on https://gist.github.com/joedolson/fc5a5da9c7cb23e44257128e6a6564f8, it shows a depreciated notices when the debugging set to true on the environment. As evident, I have this on the header when activating the test data:
Additional Notes
- Also run the following test individually and all of them passed:
test_wp_script_add_data_with_conditional_key test_wp_add_inline_script_after_and_before_with_concat_and_conditional test_conditional_inline_styles_are_also_conditional test_wp_add_inline_script_after_with_concat_and_conditional_and_core_dependency
Supplemental Artifacts
Test suite: 
Test plugin from: https://gist.github.com/joedolson/fc5a5da9c7cb23e44257128e6a6564f8
#24
@
5 months ago
My only question in this is about the deprecation notice. On the one hand, I think it's good to tell developers that there is a deprecation, and they can remove calls to conditional scripts.
On the other hand, this warning is a visible error flag for any site that has debugging enabled in the environment. It's most likely to be relevant on sites that were built long enough ago that conditional scripts were relevant, and those are more likely to be sites that don't have active development - e.g., themes that are no longer maintained, sites who don't have developers on hand, etc.
So I'm wondering whether we should actually deprecate this silently. The existence of these additional scripts doesn't harm sites; but the deprecation notice might.
Opinion, @jonsurrell?
#25
follow-up:
↓ 26
@
5 months ago
Isn't it safe to use notices liberally?
I like that it surfaces conditional scripts and styles and makes it clear that they should be removed.
I really don't know whether this can be harmful or to what degree. My inclination is to keep the notices. If there's precedent for silent deprecation and/or you feel strongly that would be better for users, I won't oppose removing the notices.
#26
in reply to:
↑ 25
@
5 months ago
Replying to jonsurrell:
Isn't it safe to use notices liberally?
I like that it surfaces conditional scripts and styles and makes it clear that they should be removed.
I really don't know whether this can be harmful or to what degree. My inclination is to keep the notices. If there's precedent for silent deprecation and/or you feel strongly that would be better for users, I won't oppose removing the notices.
In my opinion, I think deprecation notices are the right way to go. They do a good job of flagging scripts and styles that really shouldn’t be hanging around anymore.
On the other hand, this warning is a visible error flag for any site that has debugging enabled in the environment.
I don't think we should worried about them showing up as error flags when debugging is turned on, because well that’s exactly what debug mode is for. Besides, debugging mode isn’t meant for production, so it’s on the maintainer or site owner to handle those notices in their environment.
@jonsurrell commented on PR #9497:
5 months ago
#27
What’s the role of
json2in this? I am guessing it was necessary in some IE polyfill? But it’s removal, while very welcome, is confusing.
That's a good point about json2. If I enqueue a script that depends on json2, with this change it depends on an unregistered script and therefore does not print. The same may be true with wp-embed-template-ie.css, could styles depend on that and stop being printed unexpectedly?
I'm not sure what's best here.
@joedolson commented on PR #9497:
5 months ago
#28
json2 was used by core as a polyfill for a few scripts. It was loaded conditionally for those core scripts. It's removed in this because core no longer needs it.
But I think you're right that we probably can't actually remove it - it's enqueued by some widely installed plugins without conditional scripting, so we probably need to leave it in and only remove the core calls to it.
#29
@
5 months ago
I think my concerns are probably more historic than anything else regarding the deprecation notices; it was once a lot more common for production sites to run with debugging enabled and PHP errors exposed. I do think that's a lot less common now.
We can always commit with deprecation notices, and if they raise too many flags during beta/RC we can remove it.
@joedolson commented on PR #9497:
5 months ago
#30
Conflicts resolved, json2 restored, deprecation notice text updated.
@jonsurrell commented on PR #9497:
5 months ago
#31
I tested this out for backwards compatibility regarding json2 script and wp-embed-template-ie style.
I'm seeing two things that don't seem correct:
- Any style that depended on wp-embed-template-ie is not printed. This may be OK, I did some searching on https://www.wpdirectory.net/ and didn't see results in plugins or themes that directly depended on this style. I want to be aware of that and make an informed decision. I think the handling should be the same as
json2. json2is printed as a normal, unconditional script.
json2 seems like a clear regression. It should not be printed at all. In this PR it changes from a conditional script (never evaluated in any supported browser) to a regular script tag (always evaluated).
I also searched for json2 and the very first result was a script registration that included a dependency on json2 in Elementor. It seems clear that json2 handling is important to get right.
---
I added some enqueues like this:
wp_enqueue_script(
'a',
plugins_url( 's-a', __FILE__ ),
array( 'json2' ),
);
wp_enqueue_style(
'a-css',
plugins_url( 's-a-css', __FILE__ ),
array( 'wp-embed-template-ie' ),
);
Trunk:
<link rel='stylesheet' id='a-css-css' href='…' media='all' /> <script src="…" id="a-js"></script>
This PR:
<script src="/wp-includes/js/json2.js?ver=2015-05-03" id="json2-js"></script> <script src="…" id="a-js"></script>
Expected:
<link rel='stylesheet' id='a-css-css' href='…' media='all' /> <script src="…" id="a-js"></script>
@joedolson commented on PR #9497:
5 months ago
#32
Do you have a suggestion for how we would register a script, but not print it? I'm not sure what the best option is there...
Since it was conditional by default, if we left it as it was, it would simply never print. So, in that case, maybe it *is* something that we can just remove? I had been thinking we needed to register it, and let developers decide to remove it, but if it's presence could be harmful when it's not actually required, then we actually would be doing better by removing it.
If developers are enqueuing the script and it no longer exists, that just doesn't enqueue the script. So, it seems to me that removing the script entirely will have the same effect as leaving it in place but marked as conditional.
Regarding the wp-embed-ie-template-css, I think there's no risk there. It's always possible, but it would be a strange style to have a dependency on. I'm comfortable with this change.
@jonsurrell commented on PR #9497:
5 months ago
#33
Do you have a suggestion for how we would register a script, but not print it? I'm not sure what the best option is there...
I pushed an idea for this:
json2needs to continue to be set asconditional. That seems clear to me, it prevents it from being printed.- We don't want to add _doing it wrong_ messages for this Core script that can't safely be removed.
- I changed the value for the conditional to a specific value that bypasses the _doing it wrong_ call.
This works but I'm not attached to it if other folks have ideas.
One thing that would be nice is some messaging to tell scripts they should stop depending on json2.
@jonsurrell commented on PR #9497:
5 months ago
#34
- if a script has been registered as conditional, then no script depending on it _actually_ depends on it, because it will have never been read by a browser.
Yes, that's my understanding.
- therefore, all conditional scripts can safely be removed without worrying about introducing breakage; any breakage has been present for years already.
Exactly.
- when printing scripts, we can go through the motions for conditional scripts but simply ignore actually printing them out.
Yes. The script tags were not in the DOM before (they were in comment text) and they're not in the DOM.
would there be value in first removing dependencies on the conditional scripts/styles from Core and _then_ updating the registration system to handle conditional includes? make the changes more focused?
We could split this up, but removing the conditional dependencies from Core doesn't add much complication. The tricky part has been figuring out how to keep the Core conditional scripts (json2) "available" so that external scripts that depend on it continue to be enqueued, while not actually printing the script.
@joedolson commented on PR #9497:
5 months ago
#35
I think that @dmsnell's point that no script should actually be depending on json2 is valid. If it's enqueued but is conditional, then no supported browser is actually using it. Not enqueuing it should result in exactly the same results.
And because the way json2 was registered forced it to be conditional, there shouldn't be *any* instances of it being enqueued without getting wrapped in the conditionals.
Any way, I guess my feeling is that the real result is probably identical regardless of whether json2 is included and rendered conditionally or removed; there may be some rare edge cases that are handled by keeping it.
@joedolson commented on PR #9497:
5 months ago
#36
Re: messaging. I think that can just go in the dev note. It could probably also be added to the plugin check plugin as a warning.
@joedolson commented on PR #9497:
5 months ago
#37
I changed the value for the conditional to a specific value that bypasses the doing it wrong call.
I don't really have a problem with throwing _doing_it_wrong for this. Even if the script can't be removed, the call to the script can be, so this still benefits the developer.
5 months ago
#38
The tricky part has been figuring out how to keep the Core conditional scripts (json2) "available" so that external scripts that depend on it continue to be enqueued, while not actually printing the script.
How much harm is there in replacing the contents of the file with a simple comment explaining that it’s deprecated and not used? That leaves the scripts and styles in place, but removes their weight and impact on the page.
Someone looking at the output could see the note and remove the dependencies.
Then we make no systematic changes; just blank out those files. Leave the deprecation notice in place and let developers find that out?
@jonsurrell commented on PR #9497:
5 months ago
#39
Even if the script can't be removed, the call to the script can be, so this still benefits the developer.
The problem is that the message is shown when a script is set as conditional, so it would always fire as Core prepares scripts.
I agree it could be beneficial to show messaging when a script depends on a conditional script like this.
@joedolson commented on PR #9497:
5 months ago
#40
The problem is that the message is shown when a script is set as conditional, so it would always fire as Core prepares scripts.
Good point. So we should probably go ahead and exempt this from the doing it wrong message.
How do we feel about replacing both of these with empty files, as @dmsnell suggests? I think that's probably fine. If they do show up, they won't have any real impact.
If we agree, I'll make a last change that restores the IE css file as a blank file. restores that file & adds the conditional data there, and adds an exemption for the IE css following the same model as for json2, empty the json2 file, and then I think we're ready.
(That sentence felt like it was going to be a lot shorter when I started it.)
@jonsurrell commented on PR #9497:
5 months ago
#41
I like the plan, it seems like a safe course of action.
#42
@
4 months ago
- Keywords reviewing added
Based on the lengthy conversations in the PR, I think this is ready. Doing a final review and test.
#44
@
3 months ago
Just wanted to chime in here about the deprecation notice.
As we saw with the _load_textdomain_just_in_time change in 6.8, a lot of users have WP_DEBUG enabled without realizing it. That update caused a wave of confusion and support requests from everyday users who suddenly saw warnings in their logs or even on the frontend:
https://wordpress.org/search/_load_textdomain_just_in_time/
In my opinion, _load_textdomain_just_in_time was a necessary notice - it revealed a real problem that could lead to incorrect translations.
In this case, though, the IE conditional change doesn't seem to expose the same kind of breakage risk. The notice could end up creating noise and unnecessary confusion for end-users without providing meaningful value to developers.
Maybe it's better handled silently (or logged only when SCRIPT_DEBUG is enabled), instead of surfacing as a _doing_it_wrong() notice.
#45
@
3 months ago
Thanks for sharing @edge22! That's also my concern about the deprecation notice. This is more an issue where it's an opportunity for knowledgeable devs to remove some code that won't do anything, but does no harm to stay in place.
This patch was specifically committed with the intent to remove the deprecation notice if it appeared to be a problem, so any feedback to help indicate that is useful.
See also #64174.
#47
@
3 months ago
- Resolution fixed deleted
- Status changed from closed to reopened
Bug Report
Description
With default theme Twenty Sixteen multiple deprecation notices fire.
Environment
- WordPress: 6.9-RC2
- PHP: 8.3.23
- Server: nginx/1.26.1
- Database: mysqli (Server: 8.0.35 / Client: mysqlnd 8.3.23)
- Browser: Safari 26.1
- OS: macOS
- Theme: Twenty Sixteen 3.6
- MU Plugins: None activated
- Plugins:
- Query Monitor 3.20.0
- Test Reports 1.2.1
- WordPress Beta Tester 4.0.0
- WP Debugging 2.12.2
Steps to Reproduce
- Install WP 6.9-RC2
- Install/activate Twenty Sixteen
- Set
WP_DEBUGto true - Visit home page.
- 🐞 Bug occurs.
Expected Results
- ✅ What should happen.
No errors
Actual Results
- ❌ What actually happened.
Errors from TwentySixteen functions.php in twentysixteen_scripts() where IE conditionals still exist.
Additional Notes
I suspect removing these commands will solve the issue.
// Load the Internet Explorer specific stylesheet. wp_enqueue_style( 'twentysixteen-ie', get_template_directory_uri() . '/css/ie.css', array( 'twentysixteen-style' ), '20170530' ); wp_style_add_data( 'twentysixteen-ie', 'conditional', 'lt IE 10' ); // Load the Internet Explorer 8 specific stylesheet. wp_enqueue_style( 'twentysixteen-ie8', get_template_directory_uri() . '/css/ie8.css', array( 'twentysixteen-style' ), '20170530' ); wp_style_add_data( 'twentysixteen-ie8', 'conditional', 'lt IE 9' ); // Load the Internet Explorer 7 specific stylesheet. wp_enqueue_style( 'twentysixteen-ie7', get_template_directory_uri() . '/css/ie7.css', array( 'twentysixteen-style' ), '20170530' ); wp_style_add_data( 'twentysixteen-ie7', 'conditional', 'lt IE 8' ); and wp_script_add_data( 'twentysixteen-html5', 'conditional', 'lt IE 9' );
Do we even need the IE specific stylesheets?
Related: [60285] removed some conditionals for IE < 9 support.
#63471 is the related ticket.