Follow up to: #2324371:Fix common HTML escaped render #key values due to Twig autoescape

Problem/Motivation

#2324371 introduced some conditional comment expressions into "preRenderConditionalComments". However they have been inconsistently introduced. 

$element['#prefix'] = SafeMarkup::set("\n<!--[if $expression]>\n" . $prefix);
$element['#suffix'] = SafeMarkup::set($suffix . "<![endif]-->\n");
$element['#prefix'] = SafeMarkup::set("\n<!--[if $expression]><!-->\n" . $prefix);
$element['#suffix'] = SafeMarkup::set($suffix . "<!--<![endif]-->\n")

Proposed resolution

Conditional comments should always begin with <!-- and end with -->

Remaining tasks

User interface changes

API changes

Fix it.

Comments

star-szr’s picture

star-szr
he/him
Primary language English
commented

Thanks for creating this as a follow-up issue!

I could be missing something but I don't think #2324371: Fix common HTML escaped render #key values due to Twig autoescape changed anything with the syntax:

+++ b/core/lib/Drupal/Core/Render/Element/HtmlTag.php
@@ -139,27 +139,33 @@ public static function preRenderConditionalComments($element) {
     if (!$browsers['!IE']) {
       // "downlevel-hidden".
-      $element['#prefix'] = "\n<!--[if $expression]>\n" . $element['#prefix'];
-      $element['#suffix'] .= "<![endif]-->\n";
+      $element['#prefix'] = SafeMarkup::set("\n<!--[if $expression]>\n" . $prefix);
+      $element['#suffix'] = SafeMarkup::set($suffix . "<![endif]-->\n");
     }
     else {
       // "downlevel-revealed".
-      $element['#prefix'] = "\n<!--[if $expression]><!-->\n" . $element['#prefix'];
-      $element['#suffix'] .= "<!--<![endif]-->\n";
+      $element['#prefix'] = SafeMarkup::set("\n<!--[if $expression]><!-->\n" . $prefix);
+      $element['#suffix'] = SafeMarkup::set($suffix . "<!--<![endif]-->\n");
     }

Also I think this is pretty standard IE conditional comment stuff, I'm not sure there's anything to be fixed in here but maybe you can provide more of an explanation.

star-szr’s picture

star-szr
he/him
Primary language English
commented

http://www.quirksmode.org/css/condcom.html

I hate to say it but these do actually all start with <!-- and end with -->, so to me this works as designed.

fabianx’s picture

fabianx commented

To clarify: #2324371: Fix common HTML escaped render #key values due to Twig autoescape did not introduce the comments, but rather marked them as safe:

     if (!$browsers['!IE']) {
       // "downlevel-hidden".
-      $element['#prefix'] = "\n<!--[if $expression]>\n" . $element['#prefix'];
-      $element['#suffix'] .= "<![endif]-->\n";
+      $element['#prefix'] = SafeMarkup::set("\n<!--[if $expression]>\n" . $prefix);
+      $element['#suffix'] = SafeMarkup::set($suffix . "<![endif]-->\n");
     }
     else {
       // "downlevel-revealed".
-      $element['#prefix'] = "\n<!--[if $expression]><!-->\n" . $element['#prefix'];
-      $element['#suffix'] .= "<!--<![endif]-->\n";
+      $element['#prefix'] = SafeMarkup::set("\n<!--[if $expression]><!-->\n" . $prefix);
+      $element['#suffix'] = SafeMarkup::set($suffix . "<!--<![endif]-->\n");
     }

is the relevant part of the diff.

mbrett5062’s picture

mbrett5062 commented

Comment removed to reduce noise as my original was incorrect.

mbrett5062’s picture

mbrett5062 commented

Thanks Cottser, I see now that most was correct, still a couple of things in question though. Ignore my last comment.

The only issue I still see is following.

$element['#prefix'] = SafeMarkup::set("\n<!--[if $expression]><!-->\n" . $prefix);

Has another empty conditional comment at the end.

Should be:

$element['#prefix'] = SafeMarkup::set("\n<!--[if $expression]>\n" . $prefix);

Plus

$element['#suffix'] = SafeMarkup::set($suffix . "<!--<![endif]-->\n");

Has a double opening of the conditional comment.

Should be:
$element['#suffix'] = SafeMarkup::set($suffix . "<![endif]-->\n");

Sorry for the noise and my confusion.

mbrett5062’s picture

mbrett5062 commented

And having looked further I am able to close this issue. The special syntax on the last 2 is intentional, for browsers that are not IE.
Sorry for the noise, and thanks for your patience.

mbrett5062’s picture

mbrett5062 commented
Status: Active » Closed (works as designed)

Closing this after further investigation. Thanks to @Cottser for pointing me in the right direction.

star-szr’s picture

star-szr
he/him
Primary language English
commented

Thanks for following it through @mbrett5062!