Make WordPress Core

Opened 13 years ago

Closed 9 years ago

Last modified 9 years ago

#16024 closed enhancement (fixed)

Conditional Comments for JS

Reported by: filosofo's profile filosofo Owned by: azaozz's profile 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 13 years ago.
16024.diff (982 bytes) - added by aaroncampbell 13 years ago.
Don't allow conditionals with concat
16024.2.diff (1.5 KB) - added by aaroncampbell 13 years ago.
Conditionals that play nice with concat
16024.3.patch (2.5 KB) - added by ethitter 12 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 11 years ago.
16024.5.diff (5.3 KB) - added by georgestephanis 11 years ago.
16024.6.diff (4.6 KB) - added by azaozz 9 years ago.
16024.unit-test.diff (1002 bytes) - added by valendesigns 9 years ago.
16024.unit-test.2.diff (974 bytes) - added by valendesigns 9 years ago.
16024.7.diff (3.8 KB) - added by valendesigns 9 years ago.
16024.8.diff (5.4 KB) - added by valendesigns 9 years ago.
16024.9.diff (8.5 KB) - added by azaozz 9 years ago.
16024.10.diff (3.4 KB) - added by valendesigns 9 years ago.

Download all attachments as: .zip

Change History (119)

#1 @filosofo
13 years ago

  • Description modified (diff)

#2 @aaroncampbell
13 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.

@aaroncampbell
13 years ago

Don't allow conditionals with concat

@aaroncampbell
13 years ago

Conditionals that play nice with concat

#3 @demetris
12 years ago

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

#4 @chipbennett
12 years ago

  • Cc chip@… added

#5 @Seags
12 years ago

  • Cc Seags added

#6 @F J Kaiser
12 years ago

  • Cc 24-7@… added

#7 @Mamaduka
12 years ago

  • Cc georgemamadashvili@… added

#8 @jeremyfelt
12 years ago

  • Cc jeremy.felt@… added

#9 @ethitter
12 years ago

  • Cc ehitter@… added

#10 @danielbachhuber
12 years ago

  • Cc wordpress@… added

#11 @pizzli
12 years ago

  • Cc pizzli added

#12 @kobenland
12 years ago

  • Cc obenland@… added

#13 @kobenland
12 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?

#14 @mintindeed
12 years ago

  • Cc gabriel.koen@… added

#15 @l3rady
12 years ago

  • Cc scott@… added

#16 @CharlesKakiDCM
12 years ago

  • Cc CharlesKakiDCM added

#17 @lancewillett
12 years ago

  • Cc lancewillett added

#18 @WraithKenny
12 years ago

  • Cc Ken@… added

#19 follow-up: @mattwiebe
12 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/

#20 @wpsmith
12 years ago

  • Cc travis@… added

#21 @cgrymala
12 years ago

  • Cc curtiss@… added

#22 @sepster
12 years ago

  • Cc sepster added

#23 @mercime
12 years ago

  • Cc mercijavier@… added

@ethitter
12 years ago

Expand conditional comments to permit targeting browsers other than IE

#24 in reply to: ↑ 19 @ethitter
12 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 )

#25 @SergeyBiryukov
12 years ago

  • Version changed from 3.4.1 to 3.1

Version field indicates when the enhancement was initially suggested.

#26 @jazbek
12 years ago

  • Cc j.yzbek@… added

#27 @maorb
11 years ago

  • Cc maor@… added

#28 @georgestephanis
11 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

#29 @georgestephanis
11 years ago

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

#30 @ryanve
11 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

#31 @georgestephanis
11 years ago

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

#32 @flix
11 years ago

  • Cc flix added

#33 @helen
11 years ago

#23106 was marked as a duplicate.

#34 @desrosj
11 years ago

  • Cc desrosj@… added

#35 @mfields
11 years ago

  • Cc michael@… added

#36 @cais
11 years ago

  • Cc edward.caissie@… added

#37 @sabreuse
11 years ago

  • Cc sabreuse added

#38 @pauldewouters
11 years ago

  • Cc pauldewouters@… added

#39 @ocean90
11 years ago

  • Cc ocean90 added

#40 follow-ups: @georgestephanis
11 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?

#41 @ryanve
11 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.

#42 @ethitter
11 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?

#43 @sabreuse
11 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/

#44 @azaozz
11 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.

#45 @mcbenton
11 years ago

  • Cc morgan.benton@… added

#46 @lkraav
11 years ago

  • Cc leho@… added

#47 @wedi
11 years ago

  • Cc wedi added

#48 @philiparthurmoore
11 years ago

  • Cc philip@… added

#49 @SergeyBiryukov
11 years ago

#24570 was marked as a duplicate.

#50 @Otto42
11 years ago

  • Cc Otto42 added

#51 @BandonRandon
11 years ago

  • Cc BandonRandon added

#52 @patrickhopman
11 years ago

  • Cc patrickhopman added

#53 in reply to: ↑ 40 @WraithKenny
11 years 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.

#54 @MikeHansenMe
11 years ago

  • Cc mdhansen@… added

#55 @mindctrl
11 years ago

  • Cc dailyrants@… added

#56 @buffler
11 years ago

  • Cc jeremy.buller@… added

#57 in reply to: ↑ 40 @retlehs
10 years 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

#58 @nacin
10 years 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.

#59 follow-up: @retlehs
10 years 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.

#60 @georgestephanis
10 years 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.

#61 @pauldewouters
10 years 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.

#62 in reply to: ↑ 59 @jdgrimes
10 years 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.

#63 @kevdotbadger
10 years 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.

#64 @QuarkSEO
10 years ago

i agree ! :-)

#65 @fgirardey
10 years 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 :/

#66 @timersys
10 years ago

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

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


10 years ago

#68 @samuelaguilera
10 years 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.

#69 @GregLone
9 years ago

4 years!
Unbelievable :|

#70 @patrickhopman
9 years ago

+1, please implement this in 4.1.

#71 @sarukuku
9 years ago

  • Keywords dev-feedback added

Agreed. This is an annoying inconsistency.

#72 @nikonratm
9 years ago

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

#73 @Howdy_McGee
9 years 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.

#74 @GregLone
9 years ago

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

#75 @grapplerulrich
9 years ago

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

#76 follow-up: @sarukuku
9 years 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?

#77 in reply to: ↑ 76 @GregLone
9 years 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).

#78 @kevdotbadger
9 years 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.

#79 @sarukuku
9 years ago

@kevdotbadger fair enough, agreed

#80 @fgirardey
9 years ago

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

@azaozz
9 years ago

#81 @azaozz
9 years 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.

#82 @kevdotbadger
9 years ago

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

#83 @pento
9 years ago

  • Keywords needs-unit-tests added

#84 follow-up: @morganestes
9 years 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]>";

#85 @valendesigns
9 years 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

@valendesigns
9 years ago

#86 @valendesigns
9 years 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.

#87 in reply to: ↑ 84 @valendesigns
9 years 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.

@valendesigns
9 years ago

#88 @valendesigns
9 years ago

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

#89 @nikonratm
9 years ago

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

Last edited 9 years ago by nikonratm (previous) (diff)

@azaozz
9 years ago

#90 follow-up: @azaozz
9 years 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.

#91 in reply to: ↑ 90 ; follow-up: @valendesigns
9 years 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?

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


9 years ago

#93 @azaozz
9 years 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.

#94 in reply to: ↑ 91 @azaozz
9 years 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.

#95 follow-ups: @nacin
9 years 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.

#96 @saas
9 years ago

Glad it finally arrived in 4.2. CHEERS :)

#97 @DrewAPicture
9 years 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.

#98 in reply to: ↑ 95 @valendesigns
9 years 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.

#99 in reply to: ↑ 95 @azaozz
9 years 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 :)

#100 @azaozz
9 years ago

In [31317]:

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

#101 @mikkelbreum
9 years ago

This will be a great improvement in 4.2

#102 follow-up: @DrewAPicture
9 years ago

  • Status changed from reopened to reviewing

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

#103 in reply to: ↑ 102 @azaozz
9 years 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.

#104 follow-up: @reypm
9 years 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

#105 in reply to: ↑ 104 @jdgrimes
9 years 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.

#106 @DrewAPicture
9 years ago

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