Make WordPress Core

Opened 13 years ago

Closed 17 months ago

#16118 closed enhancement (wontfix)

Support for wp_enqueue_style with negative conditional comments

Reported by: sayontan's profile sayontan Owned by:
Milestone: Priority: normal
Severity: normal Version: 3.0.4
Component: Script Loader Keywords: has-patch needs-testing dev-feedback close
Focuses: Cc:

Description (last modified by nacin)

Please refer to #10891. It refers to the support for conditional comments using the global variable, wp_styles.

I have noticed that if you pass a negative conditional comment, however, this breaks. E.g. Let's say you have a lot of CSS3 rules, which don't apply to IE. You would not include that CSS:

<!--[if !IE]>-->
<link rel="stylesheet" id="my-handle-css" href="http://my.url.com/css3.css" type="text/css" media="all"/>
<!--<![endif]-->

I know that IE9 supports CSS3, but I am using the above for illustrative purposes. One would expect that to include the conditional comment above you would do this between the register and the enqueue commands:

$GLOBALS['wp_styles']->add_data('my-handle', 'conditional', '!IE');

If you add a conditional tag to wp_styles, however, the generated markup is incorrect:

<!--[if !IE]>
<link rel='stylesheet' id='my-handle-css'  href='http://my.url.com/css3.css' type='text/css' media='all' />
<![endif]-->

Note the missing --> after [if !IE]>, and <!-- before <![endif]. This has the effect of hiding the CSS from all browsers, which wasn't the original intention. So probably a separate handling approach is required for "show" type of conditional comments than for "hide".

Attachments (7)

16118.patch (773 bytes) - added by SergeyBiryukov 13 years ago.
greuben-16118.diff (1.2 KB) - added by greuben 13 years ago.
garyc40.16118.diff (902 bytes) - added by garyc40 13 years ago.
included edge cases. eg. [if !IE 8]
garyc40.16118.2.diff (909 bytes) - added by garyc40 13 years ago.
fixed typo
sanchothefat-01052012-class.wp-styles.php.patch (772 bytes) - added by sanchothefat 12 years ago.
A new patch that handles complex as well as non IE CCs.
sanchothefat.16118.patch (772 bytes) - added by sanchothefat 12 years ago.
More complete patch that handles complex & non IE CCs.
umbercode.16118.patch (1.0 KB) - added by Umbercode 12 years ago.
uses (!IE) as trigger to circumvent false positives

Download all attachments as: .zip

Change History (30)

#1 @nacin
13 years ago

  • Description modified (diff)
  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Future Release
  • Type changed from defect (bug) to enhancement

#2 @SergeyBiryukov
13 years ago

  • Keywords has-patch added; needs-patch removed

#3 @greuben
13 years ago

oops my patch is wrong.
Sergey's patch looks good

@garyc40
13 years ago

included edge cases. eg. [if !IE 8]

#4 @garyc40
13 years ago

  • Keywords needs-testing added

In case the condition if !IE 8 is specified, the valid code should be:

<!--[if !IE 8]><!-->HTML<!--<![endif]-->

not:

<!--[if !IE 8]>-->HTML<!--<![endif]-->

Otherwise, in IE 7 or 6, for example, this will be displayed:

-->HTML

My patch addresses this edge case.

@garyc40
13 years ago

fixed typo

@sanchothefat
12 years ago

A new patch that handles complex as well as non IE CCs.

@sanchothefat
12 years ago

More complete patch that handles complex & non IE CCs.

#5 @sanchothefat
12 years ago

  • Keywords dev-feedback added
  • Version changed from 3.0.4 to 3.4

There's one extra tweak you need for this to work properly that's in my attached patch (sorry - didn't name the first one correctly) As soon as there is an instance of '!IE' anywhere in the conditional we need to leave the contents of the CC uncommented.

Take for example a more complex CC:

<!--[if (!IE)|(gte IE 9)]><!--> <link ... /> <!--<![endif]-->

The not IE logic can appear anywhere within the CC. Complex CCs will become more useful with the ability to target IEMobile too.

I'd love to see this in core, not just for the sake of completeness but it makes writing stylesheets for plugins that need to separate support for older IEs and the newest much much better.

I've tested my modification of Gary's patch with the above example and various others and it works fine.

To test it work through the examples listed on this page: http://msdn.microsoft.com/en-us/library/ms537512(v=vs.85).aspx

Note:
In cases like !(IE 7) the contents of the CC should still be commented. Only !IE should cause the contents to be uncommented.

#6 follow-up: @helenyhou
12 years ago

  • Version changed from 3.4 to 3.0.4

Version number indicates when the enhancement was initially suggested.

#7 in reply to: ↑ 6 @sanchothefat
12 years ago

Replying to helenyhou:

Version number indicates when the enhancement was initially suggested.

Apologies, didn't realise that. Thought the ticket had been left behind at 3.0.4 :/

#8 @ocean90
12 years ago

Duplicate: #21162

#9 @Umbercode
12 years ago

All of the attached patches seem to test for '!' or '!IE' which means you get a false positive when testing for '!IEMobile'.

#10 @Umbercode
12 years ago

I had some time so I thougth some more on this "problem". As I see it we have the following possibilities:

  • !IE
  • (!IE)
  • !IE x (where x is a version number)
  • (!IE x)
  • !IEMobile
  • (!IEMobile)
  • !IEy (where y is any string that microsoft in the future might add)

So short of a regex I think we just need to decide which one triggers the requested behaviour.
Since !IE x and !IEMobile can mean just targeting any IE or IE derivative we can safely assume that we want this behaviour only on !IE or (!IE), if needed we can implicitly add it (e.g. if we also want to add all non-IE browsers to <!--[if !IE 7]>--> we can "force" this by using <!--[if (!IE 7) | (!IE)]>-->
As such I propose we designate (!IE) as the trigger, this eliminates the possibility for false positives like !IE 8, !IEMobile, !IE4 etc.
I attached a new patch for this.

@Umbercode
12 years ago

uses (!IE) as trigger to circumvent false positives

#11 @kobenland
12 years ago

  • Cc obenland@… added

#12 @SergeyBiryukov
11 years ago

#22122 was marked as a duplicate.

#13 @helenyhou
11 years ago

  • Keywords 3.6-early added

Want to remember to look at this. Preliminary question: is it a matter of making specific things use the full comment around the start and end of the conditional, or is it a matter of making specific things use the comment around the conditional statement as a whole?

#14 @SergeyBiryukov
10 years ago

#25427 was marked as a duplicate.

#15 @pburke
10 years ago

Just a comment for anyone looking for a workaround: Rather than using a !IE conditional comment, consider testing the $is_IE global variable and enqueueing your non-IE style if it is false:

global $is_IE;

if (!$is_IE) {
	wp_enqueue_style( 'mytheme-style', get_stylesheet_uri() );
}

#16 @nacin
10 years ago

  • Component changed from General to Script Loader

#18 @QC Creative
9 years ago

Hi, I'm new to contributing to core, but I'd really like to see this patch integrated. @Umbercode 's patch looks fine to me — or is at least better than 0 support for negatives, which is what we have now — what needs to be done to get this incorporated in core?

#19 @peterwilsoncc
7 years ago

#38729 was marked as a duplicate.

#20 @shaunjeffrey
7 years ago

Hi All,

I submitted a ticket regarding support for script conditionals, which was marked as duplicate, and while the code for scripts and styles isn't identical, the process is relatively similar for both.

The most adaptable way to deal with this issue is to apply filters to both the before and after conditionals, and pass through at least the handle, and possibly whether it's a script or style tag if we want to use the same filter for both scripts and styles, and whether it's the before or after conditional if we want to use the same filter for both of those as well.

I propose the following changes, or something similar to that effect:

/wp-includes/class.wp-styles.php : Lines 224-225

$conditional_pre  = apply_filters( 'wp_conditional', "<!--[if {$obj->extra['conditional']}]>\n", $handle, $obj, 'style', 'before' );
$conditional_post = apply_filters( 'wp_conditional', "<![endif]-->\n", $handle, $obj, 'style', 'after' );

/wp-includes/class.wp-scripts.php : Lines 272-273

$cond_before = apply_filters( 'wp_conditional', "<!--[if {$conditional}]>\n", $handle, $obj, 'script', 'before' );
$cond_after  = apply_filters( 'wp_conditional', "<![endif]-->\n", $handle, $obj, 'script', 'after' );

Whether we use the same filter or separate filters for each, such as wp_style_conditional_before, would be up to the core developers or community to decide.

Other methods of obtaining the desired result, in order of decreasing usability and scalability, would be to either:

  1. add a new key when setting extra data
    wp_style_add_data( $handle, 'not-conditional', '!IE' );
    
  2. adding a flag specifically for conditionals
    wp_style_add_data( $handle, 'conditional', '!IE', $not_flag );
    
  3. or parse the user's data and hope for the best
    $conditional_pre  = "<!--[if {$obj->extra['conditional']}]>" . (preg_match( "/\!/", $obj->extra['conditional'] ) ? "<!-->" : "") . "\n";
    

#21 @johnbillion
2 years ago

  • Keywords 3.6-early removed

#22 follow-up: @peterwilsoncc
2 years ago

  • Keywords close added

I think this can be closed without adding support given WordPress has dropped support for Internet Explorer and Microsoft Edge dropped support of conditional comments.

#23 in reply to: ↑ 22 @hellofromTonya
17 months ago

  • Resolution set to wontfix
  • Status changed from new to closed

Replying to peterwilsoncc:

I think this can be closed without adding support given WordPress has dropped support for Internet Explorer and Microsoft Edge dropped support of conditional comments.

I agree with Peter. Closing this ticket. Thank you everyone for your contributions!

Note: See TracTickets for help on using tickets.