Remove support for IE conditional scripts and styles

[joedolson]

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

---

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

[github-actions]

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props joedolson, jonsurrell, shailu25, dmsnell, sabernhardt.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[github-actions]

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

• The Plugin and Theme Directories cannot be accessed within Playground.
• All changes will be lost when closing a tab with a Playground instance.
• All changes will be lost when refreshing the page.
• A fresh instance is created each time the link below is clicked.
• Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
  it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

[sirreal]

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.

[joedolson]

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.

[joedolson]

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.

[sirreal]

I guess we could de-register the handle in add_data(), but that feels dirty, somehow.

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.

Can still handle a deprecation notice in `add_data(), though.

I do think a deprecation notice there would be good.

[joedolson]

The cases surrounding removing dependencies could definitely get complicated. Would like to get some additional opinions here, I think.

[sirreal]

A few final notes from me, this is mostly ready. I'll bring the remaining question over to the trac ticket for more feedback.

[sirreal]

Thanks, I think this is ready!

I left one point of non-blocking feedback you may handle as you see fit. If you disagree, you may disregard without further discussion, I don't feel strongly.

[dmsnell]

What’s the role of json2 in this? I am guessing it was necessary in some IE polyfill? But it’s removal, while very welcome, is confusing.

[sirreal]

What’s the role of json2 in 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]

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.

[joedolson]

Conflicts resolved, json2 restored, deprecation notice text updated.

[sirreal]

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.
• json2 is 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:

<!--[if lte IE 8]>
<link rel='stylesheet' id='wp-embed-template-ie-css' href='/wp-includes/css/wp-embed-template-ie.css?ver=6.9-alpha-60093-src' media='all' />
<![endif]-->
<link rel='stylesheet' id='a-css-css' href='…' media='all' />

<!-- …snip… -->

<!--[if lt IE 8]>
<script src="/wp-includes/js/json2.js?ver=2015-05-03" id="json2-js"></script>
<![endif]-->
<script src="…" id="a-js"></script>

This PR:

<!-- no a-css stylesheet at all -->

<script src="/wp-includes/js/json2.js?ver=2015-05-03" id="json2-js"></script>
<script src="…" id="a-js"></script>

Expected:

<!-- no conditional wp-embed-template-ie style -->
<link rel='stylesheet' id='a-css-css' href='…' media='all' />
<!-- no conditional json2 script -->
<script src="…" id="a-js"></script>

[joedolson]

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.

[sirreal]

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:

• json2 needs to continue to be set as conditional. 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.

[dmsnell]

this is complicated. I don‘t think I understand the implications of removing the conditional scripts well, but to clarify my own understanding:

• 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.
• therefore, all conditional scripts can safely be removed without worrying about introducing breakage; any breakage has been present for years already.
• when printing scripts, we can go through the motions for conditional scripts but simply ignore actually printing them out.

is this right?

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?

[dmsnell]

this should only be the case if the conditional script is the only dependency pulling it in, right?

or at this point in the code will that be handled because the deps here are only for the condition script itself and not all scripts?

[sirreal]

There are details here: #9497 (comment)

[joedolson]

Each script that has a dependency enqueues its dependencies separately; so any dependency will still be enqueued by other scripts.

[dmsnell]

could we not leave this in as an empty file? or with a CSS comment that it’s deprecated?

[sabernhardt]

I like the idea of keeping the file with only a CSS comment inside. To maintain the handle too, you could define false for the source and move the line(s) to the "Deprecated CSS" section of script-loader.php.

	$styles->add( 'wp-embed-template-ie', false );
	$styles->add_data( 'wp-embed-template-ie', 'conditional', 'lte IE 8' );

The chance of someone labeling the embed IE stylesheet as a dependency is very low, but possible.

[sirreal]

• 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]

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]

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]

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.

[dmsnell]

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?

[sirreal]

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]

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

[sirreal]

I like the plan, it seems like a safe course of action.