#16024 closed enhancement (fixed)
Conditional Comments for JS
Reported by: |
|
Owned by: |
|
---|---|---|---|
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 )
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)
Change History (119)
#3
@
13 years ago
Seems like a duplicate of the older #10891. Closing the older one, leaving this open.
#13
@
13 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?
#19
follow-up:
↓ 24
@
13 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/
#24
in reply to:
↑ 19
@
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
@
12 years ago
- Version changed from 3.4.1 to 3.1
Version field indicates when the enhancement was initially suggested.
#28
@
12 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
#31
@
12 years ago
Eh, this is just pulling across the infastructure that's already in place for styles. Hurrah consistency and stuff!
#40
follow-ups:
↓ 53
↓ 57
@
12 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
@
12 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
@
12 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
@
12 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
@
12 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.
#53
in reply to:
↑ 40
@
12 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.
#57
in reply to:
↑ 40
@
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?
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
@
11 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:
↓ 62
@
11 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
@
11 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
@
11 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
@
11 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
@
11 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.
#65
@
11 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 :/
This ticket was mentioned in IRC in #wordpress-dev by georgestephanis. View the logs.
11 years ago
#68
@
11 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.
#72
@
10 years ago
Yes, please! Sadly the day is not yet here when we can truly forget about supporting IE...
#73
@
10 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.
#76
follow-up:
↓ 77
@
10 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
@
10 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
@
10 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.
#80
@
10 years ago
If the WordPress API is providing conditional comments for styles, it should be the same for scripts for sure...
#81
@
10 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.
#84
follow-up:
↓ 87
@
10 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
@
10 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
#86
@
10 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
@
10 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.
#88
@
10 years ago
The latest patch 16024.8.diff adds a few more unit tests plus the ability to wrap conditionals around data.
#90
follow-up:
↓ 91
@
10 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:
↓ 94
@
10 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.
10 years ago
#93
@
10 years ago
- Owner set to azaozz
- Resolution set to fixed
- Status changed from new to closed
In 31223:
#94
in reply to:
↑ 91
@
10 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:
↓ 98
↓ 99
@
10 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.
#97
@
10 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
@
10 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
@
10 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 :)
#102
follow-up:
↓ 103
@
10 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
@
10 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:
↓ 105
@
10 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
@
10 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.
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.