WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 4 months ago

Last modified 3 months ago

#16024 closed enhancement (fixed)

Conditional Comments for JS

Reported by: filosofo Owned by: azaozz
Milestone: 4.2 Priority: normal
Severity: normal Version: 3.1
Component: Script Loader Keywords: has-patch needs-testing dev-feedback
Focuses: template Cc:

Description (last modified by filosofo)

Currently, we can add IE conditional comments for CSS but not JS. That seems unnecessarily inconsistent.

Patch would allow us to do something like the following:

wp_enqueue_script( 'pngfix-handle' );
$wp_scripts->add_data( 'pngfix-handle', 'conditional', 'IE 6' );

Attachments (13)

conditional-comments-for-js.16024.diff (1.2 KB) - added by filosofo 5 years ago.
16024.diff (982 bytes) - added by aaroncampbell 4 years ago.
Don't allow conditionals with concat
16024.2.diff (1.5 KB) - added by aaroncampbell 4 years ago.
Conditionals that play nice with concat
16024.3.patch (2.5 KB) - added by ethitter 3 years ago.
Expand conditional comments to permit targeting browsers other than IE
16024.sans-not-ie.with-wrapper-functions.diff (3.5 KB) - added by georgestephanis 3 years ago.
16024.5.diff (5.3 KB) - added by georgestephanis 3 years ago.
16024.6.diff (4.6 KB) - added by azaozz 6 months ago.
16024.unit-test.diff (1002 bytes) - added by valendesigns 6 months ago.
16024.unit-test.2.diff (974 bytes) - added by valendesigns 6 months ago.
16024.7.diff (3.8 KB) - added by valendesigns 6 months ago.
16024.8.diff (5.4 KB) - added by valendesigns 6 months ago.
16024.9.diff (8.5 KB) - added by azaozz 6 months ago.
16024.10.diff (3.4 KB) - added by valendesigns 5 months ago.

Download all attachments as: .zip

Change History (119)

comment:1 @filosofo5 years ago

  • Description modified (diff)

comment:2 @aaroncampbell4 years ago

  • Keywords 3.3-early added; 3.2-early removed

I ran against this same thing wanting to use an html5.js file much like Twenty Eleven does. The only drawback is that the conditional option is ignored is do_concat is true. We have two options here. One is to not concatenate scripts that have a condition, the second is to just say that concatenation doesn't support conditionals. If we do that latter I'd move the logic into the else so it's only run if do_concat is false.

I'll try to add a couple patches here...one for each way so we can test them out an see what works best.

@aaroncampbell4 years ago

Don't allow conditionals with concat

@aaroncampbell4 years ago

Conditionals that play nice with concat

comment:3 @demetris4 years ago

Seems like a duplicate of the older #10891. Closing the older one, leaving this open.

comment:4 @chipbennett4 years ago

  • Cc chip@… added

comment:5 @Seags4 years ago

  • Cc Seags added

comment:6 @F J Kaiser3 years ago

  • Cc 24-7@… added

comment:7 @Mamaduka3 years ago

  • Cc georgemamadashvili@… added

comment:8 @jeremyfelt3 years ago

  • Cc jeremy.felt@… added

comment:9 @ethitter3 years ago

  • Cc ehitter@… added

comment:10 @danielbachhuber3 years ago

  • Cc wordpress@… added

comment:11 @pizzli3 years ago

  • Cc pizzli added

comment:12 @kobenland3 years ago

  • Cc obenland@… added

comment:13 @kobenland3 years ago

Yes, please!

To fully leverage the functionality, how about a wrapper function for $wp_scripts->add_data() like I suggested for styles in #18753?

comment:14 @mintindeed3 years ago

  • Cc gabriel.koen@… added

comment:15 @l3rady3 years ago

  • Cc scott@… added

comment:16 @CharlesKakiDCM3 years ago

  • Cc CharlesKakiDCM added

comment:17 @lancewillett3 years ago

  • Cc lancewillett added

comment:18 @WraithKenny3 years ago

  • Cc Ken@… added

comment:19 follow-up: @mattwiebe3 years ago

This should also detect for the use-case of !IE (and other conditions which will show if 1) the condition is true or 2) the browser doesn't pay attention to CCs), which requires a slightly different syntax:

<!--[if !IE]><!-->
<p>You are not using Internet Explorer.</p>
<!--<![endif]-->

This will become especially important once we hit jQuery 2.0, which will not support IE <= 8, using 1.9 as a fallback for oldIE. The recommended usage at that point will be:

<!--[if lt IE 9]>
    <script src="jquery-1.9.0.js"></script>
<![endif]-->
<!--[if gte IE 9]><!-->
    <script src="jquery-2.0.0.js"><</script>
<!--<![endif]-->

^ From http://blog.jquery.com/2012/06/28/jquery-core-version-1-9-and-beyond/

comment:20 @wpsmith3 years ago

  • Cc travis@… added

comment:21 @cgrymala3 years ago

  • Cc curtiss@… added

comment:22 @sepster3 years ago

  • Cc sepster added

comment:23 @mercime3 years ago

  • Cc mercijavier@… added

@ethitter3 years ago

Expand conditional comments to permit targeting browsers other than IE

comment:24 in reply to: ↑ 19 @ethitter3 years ago

  • Keywords needs-testing added; 3.3-early removed
  • Version changed from 3.1 to 3.4.1

Replying to mattwiebe:

This should also detect for the use-case of !IE (and other conditions which will show if 1) the condition is true or 2) the browser doesn't pay attention to CCs), which requires a slightly different syntax:

16024.3.patch expands on what aaroncampbell worked on by adding the ability to target browsers other than IE.

To do so, rather than passing a string for the conditional, an array is used. I took this approach rather than parsing the conditional's content to determine if non-IE browsers are targeted. The array syntax is only needed to flag that the syntax should be tweaked as mattwiebe noted, so a string can still be passed for all IE-specific conditionals.

Array structure:
array( 'condition' => '!IE', 'target_ie' => false )

comment:25 @SergeyBiryukov3 years ago

  • Version changed from 3.4.1 to 3.1

Version field indicates when the enhancement was initially suggested.

comment:26 @jazbek3 years ago

  • Cc j.yzbek@… added

comment:27 @maorb3 years ago

  • Cc maor@… added

comment:28 @georgestephanis3 years ago

Added a new patch, without ethitter's non-IE changes (as I think that's out of the scope of the ticket, and should be addressed separately for both scripts AND styles in a subsequent ticket ... bite-sized chunks)

I also added wrapper functions for both $wp_scripts->add_data() and $wp_styles->add_data() as per kobenland in #18753

@georgestephanis3 years ago

comment:29 @georgestephanis3 years ago

Added one more revision, this time with twentytwelve integrating these changes.

comment:30 @ryanve3 years ago

I agree there a need for the capability, but I think it'd be simpler to implement it via a applying filter on the <script> tag markup. See #22245 and #22249

comment:31 @georgestephanis3 years ago

Eh, this is just pulling across the infastructure that's already in place for styles. Hurrah consistency and stuff!

comment:32 @flix3 years ago

  • Cc flix added

comment:33 @helen2 years ago

#23106 was marked as a duplicate.

comment:34 @desrosj2 years ago

  • Cc desrosj@… added

comment:35 @mfields2 years ago

  • Cc michael@… added

comment:36 @cais2 years ago

  • Cc edward.caissie@… added

comment:37 @sabreuse2 years ago

  • Cc sabreuse added

comment:38 @pauldewouters2 years ago

  • Cc pauldewouters@… added

comment:39 @ocean902 years ago

  • Cc ocean90 added

comment:40 follow-ups: @georgestephanis2 years ago

In hindsight, I'm not 100% sold on this, primarily because IE10 has dropped support for conditional comments entirely. So this would be adding a new feature for old tech, which I'm not sure is the best idea.

I mean, I like having a better system to deal with it, but it feels like we'd be moving backwards. Yay consistency and all, but ... ?

Thoughts?

comment:41 @ryanve2 years ago

@georgestephanis I agree. This is not needed as a separate feature.

#13592 'script_loader_tag' filter would be sufficient and more generally useful.

comment:42 @ethitter2 years ago

  • Cc erick@… added

I think this is necessary, for two reasons.

First, as @mattwiebe noted in comment #19, jQuery 2.0 will drop a lot of backcompat for versions of IE that are still quite common. Unless Core plans on dropping support for those versions of IE when 2.0 is added to Core, we'll need some mechanism for loading the appropriate version needed for IE.

Second, we already have this ability for stylesheets, so why not introduce parity in the script functions as well?

comment:43 @sabreuse2 years ago

jQuery 2.0 is a non-issue: their plan is to continue to develop 1.9 and 2.0 in parallel; for as long as we're supporting older IE, the 1.9 branch will be there with the same features as the 2.0 branch. See their clarification post: http://blog.jquery.com/2012/07/01/jquery-1-9-and-2-0-tldr-edition/

comment:44 @azaozz2 years ago

Agree with @georgestephanis too. More reasons:

  • Using IE conditional tags will make the scripts impossible to concatenate.
  • In practice it could be useful for IE7 however it nears EOL.
  • For small tweaks, there are other ways to detect a browser in JS, feature detection being best. Also the body tag in the admin gets ie7 class.

comment:45 @mcbenton2 years ago

  • Cc morgan.benton@… added

comment:46 @lkraav2 years ago

  • Cc leho@… added

comment:47 @wedi2 years ago

  • Cc wedi added

comment:48 @philiparthurmoore2 years ago

  • Cc philip@… added

comment:49 @SergeyBiryukov2 years ago

#24570 was marked as a duplicate.

comment:50 @Otto422 years ago

  • Cc Otto42 added

comment:51 @BandonRandon2 years ago

  • Cc BandonRandon added

comment:52 @patrickhopman2 years ago

  • Cc patrickhopman added

comment:53 in reply to: ↑ 40 @WraithKenny23 months ago

Replying to georgestephanis:

In hindsight, I'm not 100% sold on this, primarily because IE10 has dropped support for conditional comments entirely. So this would be adding a new feature for old tech, which I'm not sure is the best idea.

I mean, I like having a better system to deal with it, but it feels like we'd be moving backwards. Yay consistency and all, but ... ?

Thoughts?

Since it's IE10 that drops support, I don't think it's "adding a new feature for old tech" until WordPress completely drops IE9 support. This Feature will be relevant and useful until long into the foreseeable future, for as long as WordPress decides to support IE < 10.

comment:54 @MikeHansenMe22 months ago

  • Cc mdhansen@… added

comment:55 @mindctrl22 months ago

  • Cc dailyrants@… added

comment:56 @buffler22 months ago

  • Cc jeremy.buller@… added

comment:57 in reply to: ↑ 40 @retlehs19 months ago

Replying to georgestephanis:

In hindsight, I'm not 100% sold on this, primarily because IE10 has dropped support for conditional comments entirely. So this would be adding a new feature for old tech, which I'm not sure is the best idea.

I mean, I like having a better system to deal with it, but it feels like we'd be moving backwards. Yay consistency and all, but ... ?

Thoughts?

There's no need for conditionals for newer IE's, but there are a lot of instances where old IE's need it. Older IE's are still going to be around for a while, and whether or not WordPress drops support for them shouldn't have an effect on being able to add conditionals for JS.

I'm all for dropping support for anything but modern browsers in the admin, but lots of visitors to our [corporate] sites are still on IE8 (thanks, IT departments). Right now it's not possible to properly enqueue a polyfill such as Respond.js to just older IE's: https://github.com/scottjehl/Respond

I can't think of a reason why it'd be a bad idea to add support for adding conditionals to enqueued JS & it makes no sense that it's not consistent with enqueuing CSS.

Also, reference #13592

comment:58 @nacin17 months ago

  • Component changed from Template to Script Loader
  • Focuses template added

I don't feel strongly about this ticket. I'm not against doing this. That said, given the current browser market, where IE6 is long dead, do we find this to be a particularly compelling need? The only reason why I like it is to be consistent with styles, but it's far less useful for scripts than it is for styles.

comment:59 follow-up: @retlehs17 months ago

IE6/IE7 aren't really an issue at this point, but IE8 will be sticking around for a while. It's more important to have this ability for JS than it is to have it for CSS now. There's a lot of shims/shivs/polyfills these days — whenever a browser lags behind there's almost always going to be better to fix with JS rather than CSS.

twentyeleven through twentyfourteen use the following code in the header template:

<!--[if lt IE 9]>
<script src="<?php echo get_template_directory_uri(); ?>/js/html5.js"></script>
<![endif]-->

Seems like it would be better to have that handled where all the other theme JS is enqueued.

comment:60 @georgestephanis17 months ago

Also, it would make it simpler for someone to serve jQuery 2 for more modern browsers, and jQuery 1 for the ones that need it.

comment:61 @pauldewouters17 months ago

It's useful for scripts too. For example, some libraries don't support old browsers so it would be better to not load the script in this case. In this particular case, I'm using select2 for dropdowns, which doesn't work with IE7.

comment:62 in reply to: ↑ 59 @jdgrimes16 months ago

Replying to retlehs:

IE6/IE7 aren't really an issue at this point, but IE8 will be sticking around for a while. It's more important to have this ability for JS than it is to have it for CSS now. There's a lot of shims/shivs/polyfills these days — whenever a browser lags behind there's almost always going to be better to fix with JS rather than CSS.

Yep, IE8 is exactly the reason I was searching for this and landed on this ticket.

comment:63 @kevdotbadger15 months ago

It's kind of frustrating that there's major inconsistancies between $wp_scripts->add_data and $wp_styles->add_data. It's also a pain that you can use wp_register/enqueue_styles and wp_register/enqueue_scripts to includes almost everything, then use wp_head() to print them out, THEN you're forced to manually add the conditional comments for scripts because of the inconsistency. georgestephanis's patch works great, and it was posted 18 months ago.

comment:64 @QuarkSEO14 months ago

i agree ! :-)

comment:65 @fgirardey14 months ago

I agree too, actually i can't use a conditional script as repond.js for IE without printing it with wp_head which is bad :/

comment:66 @timersys14 months ago

+1 on this. I also landed here with same problem

comment:67 @ircbot14 months ago

This ticket was mentioned in IRC in #wordpress-dev by georgestephanis. View the logs.

comment:68 @samuelaguilera11 months ago

+1 from here too.

If we have for styles we should have for scripts too.

In fact, most of the times, when you need to output conditional tags for CSS it's very probably you need some JS for doing the trick too.

comment:69 @GregLone9 months ago

4 years!
Unbelievable :|

comment:70 @patrickhopman9 months ago

+1, please implement this in 4.1.

comment:71 @sarukuku8 months ago

  • Keywords dev-feedback added

Agreed. This is an annoying inconsistency.

comment:72 @nikonratm8 months ago

Yes, please! Sadly the day is not yet here when we can truly forget about supporting IE...

comment:73 @Howdy_McGee8 months ago

+1 We have this for styles but not scripts? Though, at this point with IE10+ being bearable and for the most part able to use HTML5 it may be too late, this ticket has been open for too long and the issue is slowly being non-existent.

Would still like to have this feature though.

comment:74 @GregLone8 months ago

@Howdy_McGee
Unfortunately IE8 and 9 won't disappear any time soon...

comment:75 @grapplerulrich6 months ago

Could this be closed as the filter script_loader_tag has been added? #13592

comment:76 follow-up: @sarukuku6 months ago

@grapplerulrich I think we could. With the script_loader_tag it's now fairly simple to write a plugin to do it or just add a functio to you theme that takes care of it. Maybe post an example here to this ticket and then close it?

comment:77 in reply to: ↑ 76 @GregLone6 months ago

Replying to sarukuku:

@grapplerulrich I think we could. With the script_loader_tag it's now fairly simple to write a plugin to do it or just add a functio to you theme that takes care of it. Maybe post an example here to this ticket and then close it?

We were able to achieve the exact same thing before 4.1, with the filter script_loader_src (unless do_concat is true).

comment:78 @kevdotbadger6 months ago

I think the major problem was the fact that the style loader had this built in (with add_data), but this is another WP inconsistency.

Saying we could write a plugin, or a function in our theme to take care of this is a little silly, as most bugs could be fixed by writing a function. We want this in the core just like the style loader has.

comment:79 @sarukuku6 months ago

@kevdotbadger fair enough, agreed

comment:80 @fgirardey6 months ago

If the WordPress API is providing conditional comments for styles, it should be the same for scripts for sure...

@azaozz6 months ago

comment:81 @azaozz6 months ago

  • Keywords dev-feedback removed
  • Milestone changed from Future Release to 4.2

In 16024.6.diff: refreshed and cleaned up a bit @georgestephanis' .5.diff.

Thinking we should add this for consistency with WP_Styles.

comment:82 @kevdotbadger6 months ago

It's a Christmas miracle! 4 years in the waiting.

comment:83 @pento6 months ago

  • Keywords needs-unit-tests added

comment:84 follow-up: @morganestes6 months ago

  • Keywords dev-feedback added

The conditional comments in .6.diff are for downlevel-hidden content. I believe the intent is to add the script if the condition is true, which means (if I'm reading http://msdn.microsoft.com/en-us/library/ms537512%28v=vs.85%29.aspx#syntax correctly) we'll want to use downlevel-revealed comments like this instead.

Am I missing anything?

downlevel-hidden

$cond_before = "<!--[if {$conditional}]>\n";
$cond_after  = "\n<![endif]-->";

downlevel-revealed

$cond_before = "<![if {$conditional}]>\n"; 
$cond_after  = "\n<![endif]>";

comment:85 @valendesigns6 months ago

  • Keywords needs-unit-tests removed

I've added a unit test for this ticket. The reason there are two attachments is the first one uses the global $wp_scripts and the other uses the new wp_script_add_data() function. The first will allow you to test the assertion without the patch being applied, which would PHP fatal since wp_script_add_data() doesn't exist yet. The second adds coverage when the patch is applied.

Cheers,
Derek

@valendesigns6 months ago

comment:86 @valendesigns6 months ago

I've added a patch that includes both the fix and unit test. As well, I've removed the changes to the Twenty Twelve theme, as they should be in their own ticket. Which I have created and added the patch to #30863.

comment:87 in reply to: ↑ 84 @valendesigns6 months ago

@morganestes That is not how I interpreted the docs you linked to. This is taken from that page.

The downlevel-revealed conditional comment enables you to include content in browsers that don't recognize conditional comments.

The Twenty Twelve theme uses downlevel-hidden to add their conditional comment. So I believe we're using the correct syntax in this case.

@valendesigns6 months ago

comment:88 @valendesigns6 months ago

The latest patch 16024.8.diff adds a few more unit tests plus the ability to wrap conditionals around data.

comment:89 @nikonratm6 months ago

A Christmas (or Hanukah or Kwanzaa or...) miracle indeed—working great for me!

Last edited 6 months ago by nikonratm (previous) (diff)

@azaozz6 months ago

comment:90 follow-up: @azaozz6 months ago

In 16024.9.diff: cleaned up the latest patch and added support for the (newly committed) data-after. Still unsure if we really need to be able to output some script data in an arbitrary place after the script tag (seems not that useful), so holding up on this for few days.

comment:91 in reply to: ↑ 90 ; follow-up: @valendesigns6 months ago

Replying to azaozz:

In 16024.9.diff: cleaned up the latest patch and added support for the (newly committed) data-after. Still unsure if we really need to be able to output some script data in an arbitrary place after the script tag (seems not that useful), so holding up on this for few days.

Thank you for cleaning up the patch. However, I removed the code specific to Twenty Twelve because it's not what this ticket is solving. I created ticket #30863 to specifically address that code, and it was shot down as a backwards compatibility issue, which I agree with. I think a new patch should be created without the theme dependent code in it. Thoughts?

comment:92 @slackbot6 months ago

This ticket was mentioned in Slack in #themereview by otto42. View the logs.

comment:93 @azaozz5 months ago

  • Owner set to azaozz
  • Resolution set to fixed
  • Status changed from new to closed

In 31223:

Add support for IE conditional comments for WP_Scripts to match the functionality of WP_Styles, including unit tests. Props filosofo, aaroncampbell, ethitter, georgestephanis, valendesigns. Fixes #16024.

comment:94 in reply to: ↑ 91 @azaozz5 months ago

Replying to valendesigns:

I removed the code specific to Twenty Twelve because it's not what this ticket is solving. I created ticket #30863 to specifically address that code...

Yep, makes sense. Removed that from the commit, thanks.

comment:95 follow-ups: @nacin5 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Two minor things based on reviewing [31223]:

  • That single test should be broken up into multiple tests. A good rule of thumb is one assertion per test. Obviously, multiple assertions testing the same thing are OK. A general example would be asserting that the array length is 3 and that it contains X key. An example specific to this would be to assert the printed scripts then confirm there is nothing left to print.
  • wp_script_add_data() has the potential to replace wp_localize_script(). Is there some kind of method signature that would be ideal to replace wp_localize_script() that the current signature would prevent? Thinking forwards compatibility here.

comment:96 @saas5 months ago

Glad it finally arrived in 4.2. CHEERS :)

comment:97 @DrewAPicture5 months ago

  • Priority changed from normal to high

Moving to high because there are already commits on the ticket and @nacin's feedback in comment:95 should be addressed.

@valendesigns5 months ago

comment:98 in reply to: ↑ 95 @valendesigns5 months ago

Replying to nacin:

  • That single test should be broken up into multiple tests. A good rule of thumb is one assertion per test. Obviously, multiple assertions testing the same thing are OK. A general example would be asserting that the array length is 3 and that it contains X key. An example specific to this would be to assert the printed scripts then confirm there is nothing left to print.

I've broken up the single test into multiple tests in 16024.10.diff.

  • wp_script_add_data() has the potential to replace wp_localize_script(). Is there some kind of method signature that would be ideal to replace wp_localize_script() that the current signature would prevent? Thinking forwards compatibility here.

In order to replace wp_localize_script we would likely need to add another parameter for name and add support for the localize key, as data and conditional are the only two keys that print to the screen AFAIK.

Alternatively, we could make it so any keys other than data and conditional are treated as name, but only if an array is passed in as the value. Otherwise, it is treated as an invalid key.

comment:99 in reply to: ↑ 95 @azaozz5 months ago

Replying to nacin:

...wp_script_add_data() has the potential to replace wp_localize_script(). Is there some kind of method signature that would be ideal to replace wp_localize_script() that the current signature would prevent? Thinking forwards compatibility here.

This is just a convenience, any plugin can use $wp_scripts->add_data() directly. I left it in there to match wp_style_add_data(). As this will be used mostly from themes, it makes sense to have these "convenience functions" with similar names. Don't mind changing the name though :)

comment:100 @azaozz5 months ago

In [31317]:

Separate the tests for IE conditional comments support in WP_Scripts. Props valendesigns, see #16024.

comment:101 @mikkelbreum5 months ago

This will be a great improvement in 4.2

comment:102 follow-up: @DrewAPicture4 months ago

  • Status changed from reopened to reviewing

@azaozz: Can you summarize what's left and what our next steps would be here?

comment:103 in reply to: ↑ 102 @azaozz4 months ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

Replying to DrewAPicture:

If we are not going to rename that convenience function, it's all done.

comment:104 follow-up: @reypm4 months ago

I get lost at this point, are conditional scripts added to the CORE in 4.1? I'm discussing some theme optimization [here][1] at SO and this ticket comes out but still don't get how to load scripts conditionally and I can't find any docs around it, can any give me some advice? Also don't know if this is the right place but I should ask also for the right way to pass a variable from PHP side to jQuery as this [line][2] does, any advice on this doubts?

[1]: http://wordpress.stackexchange.com/questions/178376/move-all-the-js-files-to-the-bottomfooter-the-right-way
[2]: https://gist.github.com/paquitodev/ff96fe0cce53334a6e2f#file-header-php-L68

comment:105 in reply to: ↑ 104 @jdgrimes4 months ago

Replying to reypm:

I get lost at this point, are conditional scripts added to the CORE in 4.1?

No. If you look at the top of the ticket, you will see that the Milestone is 4.2. So this will be a part of 4.2.

I'm discussing some theme optimization [here][1] at SO and this ticket comes out but still don't get how to load scripts conditionally and I can't find any docs around it, can any give me some advice?

I don't know if there are any docs for it, but one thing you can do is look at the tests committed in [31317]. They demonstrate how it works.

Also don't know if this is the right place but I should ask also for the right way to pass a variable from PHP side to jQuery as this [line][2] does, any advice on this doubts?

You can use wp_localize_script() for this.

comment:106 @DrewAPicture3 months ago

  • Priority changed from high to normal
Note: See TracTickets for help on using tickets.