Document the #noscript property

I wonder about that one:

+   *   - #noscript: A flag indicating if the tag should be inserted in a
+   *     <noscript> element, for users that have disabled scripts in their
+   *     browser or have a browser that doesn't support script.

Maybe too long, maybe not a flag..

Hope it helps anyway...

+++ b/core/lib/Drupal/Core/Render/Element/HtmlTag.php
@@ -94,7 +97,7 @@ public static function preRenderHtmlTag($element) {
+      $element['#markup'] = SafeMarkup::format('<noscript>!markup</noscript>', array('!markup' => $markup));

We can actually use @ here. ! is now more meant for plain text use-cases. As long as the variable is safe it won't be escaped similar to what happens in the twig template.

Regarding the comment. I think we could probably stop at this without explaining what a noscript tag is for:

#noscript: A flag indicating if the tag should be inserted in a
+ * element.

What do you think?

Thank you for the patch @pguillard!

Sorry, made a silly miss! Submitting the corrected one next

Thank you @umarzaffer for the patch.

@joelpittet, the patch #7 looks good to me.

This patch is using @ instead of !
Moving this to RTBC.

Thanks @umarzaffer and @RavindraSingh!

1. +++ b/core/lib/Drupal/Core/Render/Element/HtmlTag.php
   @@ -64,6 +64,9 @@ public function getInfo() {
   +   *   - #noscript: A flag indicating if the tag should be inserted in a
   +   *     <noscript> element, for users that have disabled scripts in their
   +   *     browser or have a browser that doesn't support script.
   

   I think we can remove everything after the comma here since the tag is a part of the HTML spec.

2. +++ b/core/lib/Drupal/Core/Render/Element/HtmlTag.php
   @@ -94,7 +97,7 @@ public static function preRenderHtmlTag($element) {
   -      $element['#markup'] = '<noscript>' . $markup . '</noscript>';
   +      $element['#markup'] = SafeMarkup::format('<noscript>!markup</noscript>', array('!markup' => $markup));
   

   The scope of the issue is just to document it; this change is out of scope. Can we remove it?

Thank you @xjm, for giving quick feedback.

Thanks @ashutoshsngh for new patch and @lokapujya for changes summary.

The patch added in #12 looks what @xjm has suggested. So moving this to RTBC

Apologies for the slightly hurried review before; I think there are a couple more things we need to do here:

1. It needs to have (optional) at the beginning since it's not a required key.
2. It should specifically say "the markup" rather than "the tag" since the noscript goes around everything, including the prefix. suffix, etc.
3. We should add a sentence for what the allowed values are. Looking at the code, it looks like any non-empty value will result in the noscript being added.

So something like:

(optional) If TRUE, the markup (including any prefix or suffix) will be wrapped in a <noscript> element. (Note that passing any non-empty value here will add the <noscript> tag.)

Sorry for not catching this the first time around. :)

Updating the beta evaluation. It doesn't matter here, but be sure to review what's unfrozen for other issues. (Converting code to best practices is not unfrozen.) Thanks!

Thanks @xjm for reviewing and providing suggestions. Have added the documentation for #noscript as per your suggestion.

I have not taken the time to continue the work. I think it is fair to move to RTBC.

Just to confirm that the docs were complete and not missing anything, I went to look for when the element was originally added. It was in #2068471: Normalize Controller/View-listener behavior with a Page object to work around something:

The real problem was that when not in maintenance mode, the #prefix/#suffix (noscript) was being ignored. This approach was hashed out between me, Crell, and msonnabaum.

he noscript tag around the meta-refresh tag is missing, which causes the page to refresh on its own out of sync with the ajax code. The problem is that the fix for that for batch API assumes the rendered code for the page runs through template_preprocess_page, but the maintenance page doesn't.

Those problems were addressed in two interdiffs in #190 and #207 of that issue. Those comments didn't make it clear to me at first why setting the noscript tags as prefix and suffix didn't work, and I was trying to decide if we should document that here...

\Drupal\Tests\Core\Render\Element\HtmlTagTest has test coverage and those tests only assert that the element does exactly what we say here. :) So this is fine I think, even if it is a weird internal-use edgecase thing.

Since I kind of wrote the comment in its current form via my review, I think I probably shouldn't commit this myself, so assigning to @jhodgdon in case she has a chance to take a look at it.

Looks good to me! Committed to 8.0.x. Thanks all!