Newline character in HtmlTag causes unwanted whitespace

This needs to go into D8 first.

Patch no longer applies to D7 – requires a reroll.

Also, please provide a patch for D8 also.

You have to change the test too, because the test is expecting the theme function to output the "\n".

8: newline-causes-unwanted-whitespace-1268180-6.patch queued for re-testing.

Probably the most relevant issue to look at is #1825090: Remove theme_html_tag() from the theme system, not sure if this is still relevant for 8.x or not.

#8 makes BC

+++ b/core/modules/system/system.module
@@ -316,6 +316,7 @@ function system_element_info() {
+    '#newline' => TRUE,

why this was removed in reroll?

Step by Step. The theme function was converted to a render array so the code is very different. It's not a typical reroll, but a start from scratch. So, now #16 shows which tests break without making the newline optional.

*If someone wants to add that parameter in, I probably won't have any time to work on it for a few days.

Do we really need this to be BC? I'd suggest we don't add the BC layer for this and just fix it. Whitespace around block elements shouldn't be expected, IMO. It breaks more things than it solves.

fix remaining tests

fix leftovers

Thank you for fixing the remaining tests @andypost

Themey things.

+++ b/core/lib/Drupal/Core/Render/Element/HtmlTag.php
@@ -87,13 +87,13 @@ public static function preRenderHtmlTag($element) {
-      $markup .= " />\n";
+      $markup .= " />";
...
-      $markup .= '</' . $escaped_tag . ">\n";
+      $markup .= '</' . $escaped_tag . ">";

Let's change these to single quotes. Considering we've changed this elsewhere.

Fixed #29 + trailing comma

Looks like this needs a reroll after #2687897: Convert system module's kernel tests to NG otherwise looking good I'd say. I do wonder if we should consider adding a change record here because contrib may be relying on the whitespace in automated tests as well. I'd also like to bump this to 8.2.x for that reason and I agree no BC layer is needed.

Checking patch core/lib/Drupal/Core/Render/Element/HtmlTag.php...
Checking patch core/modules/big_pipe/src/Tests/BigPipePlaceholderTestCases.php...
Checking patch core/modules/big_pipe/src/Tests/BigPipeTest.php...
Checking patch core/modules/system/src/Tests/Common/AttachedAssetsTest.php...
error: core/modules/system/src/Tests/Common/AttachedAssetsTest.php: No such file or directory
Checking patch core/modules/system/src/Tests/Common/RenderElementTypesTest.php...
error: core/modules/system/src/Tests/Common/RenderElementTypesTest.php: No such file or directory
Checking patch core/tests/Drupal/KernelTests/KernelTestBaseTest.php...
Checking patch core/tests/Drupal/Tests/Core/Render/Element/HtmlTagTest.php...

Rerolled #29.
Patch against 8.2.x

CR filed https://www.drupal.org/node/2742955 needs review

I think that ready, CR review could be done later

This is where a visual regression test would be super useful.

Discussed #34 with @alexpott; this is more a proposal for followup work. For this issue, thorough manual testing and the CR are sufficient. We are also making the change in 8.2.x only per @Cottser's feedback in #30, because of the risk of disruption.

Assigning to @Cottser for review. Thanks everyone!

After poking around with this one I think we need to consider this further. This makes the page source rather hard to read in spots. Maybe everyone uses a web inspector but I'm not sure about this for DX/TX and so on. One potential idea would be to always include the whitespace for elements that we know are non-rendering like meta, link, script, etc. but not sure how viable that is.

View source before the patch:

<meta charset="utf-8" />
<meta name="Generator" content="Drupal 8 (https://www.drupal.org)" />
<meta name="MobileOptimized" content="width" />
<meta name="HandheldFriendly" content="true" />
<meta name="viewport" content="width=device-width, initial-scale=1.0" />
<link rel="shortcut icon" href="/core/misc/favicon.ico" type="image/vnd.microsoft.icon" />
<link rel="alternate" type="application/rss+xml" title="" href="http://d8.dev/en/rss.xml" />

After:

<meta charset="utf-8" /><meta name="Generator" content="Drupal 8 (https://www.drupal.org)" /><meta name="MobileOptimized" content="width" /><meta name="HandheldFriendly" content="true" /><meta name="viewport" content="width=device-width, initial-scale=1.0" /><link rel="shortcut icon" href="/core/misc/favicon.ico" type="image/vnd.microsoft.icon" /><link rel="alternate" type="application/rss+xml" title="" href="http://d8.dev/en/rss.xml" />

If you look at the scripts at the bottom of the page it's an even bigger pile of messiness.

That is a good point @Cottser. Let's add the \n character back to the elements that go into the head. There are only a few places that would be needed.

See attached patch and interdiff

May still have a fail in there, but this is the tweak because one of the test is rendering them through the asset collection renderers so it has the \n

#36 is really debatable, 99% of time better to have page size smaller but for me having new line for header is fine

This is going to fail too on bigpipe. There is an extra line in the output. Fixes the other test, the order of the keys was the fail there.

@Joel I can't find why https://www.evernote.com/shard/s710/sh/fc945925-4056-4035-846f-8ca414577... that happens, I will try to find time this WE

No worries, I think I found it. Went a bit far on removing the \n from a test case for a <link that wasn't being built by the other stuff.

Yay, thanx a lot!!! Let's get this finally in

Back to @Cottser for the word

Hmm I was thinking the change would be in the HtmlTag element, not a bunch of places where we use that element. This method seems like it'll be inconsistent if anyone outside of core uses '#type' => 'html_tag' with link, meta, etc.

@Cottser, I'd rather not muddy the tag with exceptions/conditional code. IMO it shouldn't be expected that we are putting whitespace on tags or specific tags, and if someone wants to that they can do as I did in the patch with '#suffix' => "\n"

The exceptions are explicit to the parts of the code where they are 'needed'. They are only really useful in dev, in production I'd even remove those '\n', but don't want to scope creep this.

@Cottser there's 2 separate tasks discussed:
1) *bug* the issue - unwanted whitespace
2) dev time setting to have head "fine-printed - looks like follow-up

Suppose better commit #31 and discover "developer setting" in follow-up issue

Suppose page size optimization is small but it is

Assigning to @alexpott for a second opinion since he reviewed this as well.

I think we should get this into 8.3.x early so we have a chance to catch any regressions this might cause during the cycle. The issue makes sense and seems a good idea.

Let's get this in early then. @Cottser, does the rational hold water or need more discussion?

The potential effects of this behaviour change concern me. I agree with the change but I don't think we should be making it Drupal 8. The potential to break contrib and real sites in unexpected and untested ways is high. I think we need to add a #nonewline option HtmlTag's properties and use it where appropriate. And then if anyone wants to suppress it they can. And we can change the behaviour in Drupal 9.

I'm not sure this issue should have the performance tags.

@Alexpott this about of extra browser timing - lots of useless text elements created and bit more bytes transfered, also this very related to #736066: ajax.js insert command sometimes wraps content in a div, potentially producing invalid HTML and other bugs

#nonewline makes sense but default better to set to optimized

I think 6 month adoption cycle should be enough to revert in case of problems

@andypost I understand this might cause bugs but so might changing the default behaviour. Wrt to frontend performance - I like to see evidence of how impactful this is going to be.

Reroll

reroll, removed unneeded changes in core/modules/big_pipe/src/Tests/BigPipePlaceholderTestCases.php

+ added fixes for core/tests/Drupal/Tests/Core/Render/Element/HtmlTagTest.php after #2694535: Support rect property and nested render arrays in html_tag for dynamic SVGs

Another reroll

New test from #2660124: Dynamically build layout icons based on well formed config needs fixes

Re-roll & fix broken tests

Re-rolled.
Was conflict in CssCollectionRenderer.php and CssCollectionRendererUnitTest.php cos IE 9 support removed from core and render function was cahnged.

I think we need to add a #nonewline option HtmlTag's properties and use it where appropriate. And then if anyone wants to suppress it they can. And we can change the behaviour in Drupal 9.

So instead of suffix it could be option of element, so contrib/custom can override default

Patch adds this option with test & reverts mostly all changes

bot flux

So the new solution is not to remove the space, but to allow a way to remove it? Is anyone really going to use this feature and even know that it exists? Probably, they may just fix it with CSS.

As we can see, fixing the default behaviour breaks some tests (which cause us to have to fix those tests). We now were able to remove the changes to those tests because we aren't changing the default behavior. That will get revisited when the TODO is looked at for D9.

That's main problem to move this forward - how to allow change for D9

+++ b/core/lib/Drupal/Core/Render/Element/HtmlTag.php
@@ -98,6 +102,10 @@ public static function preRenderHtmlTag($element) {
+    if (empty($element['#nonewline'])) {
+      // @todo Consider to change default behaviour in Drupal 9.
+      $close_tag .= "\n";

Probably this todo should be removed...

Does not apply against 8.9.x anymore. Here's a re-roll.

fixed the test cases.

For "drupal/core": "~9.2.5"

broken patch ... sorry for the noise

The test changes in #83 onwards are all wrong, void elements should not have a closing tag.

I am not sure if we need to introduce a new property, can we go back to using #suffix and warn users with something like this?

-    $suffix = isset($element['#suffix']) ? $close_tag . $element['#suffix'] : $close_tag;
+    if (isset($element['#suffix'])) {
+      $suffix = $close_tag . $element['#suffix'];
+    }
+    else {
+      $suffix = $close_tag . "\n";
+      @trigger_error('HTML tags will no longer be suffixed with newlines from Drupal 10. Set #suffix to "\\n" to keep the existing behaviour.');
+    }

Then we can explicit set '#suffix' => '' where we want to strip the newline already.

Addressed #90

- reverted changes to #81 (interdiff against it)
- added deprecation (needs to update CR)

After full test suite gonna add test for deprecation (looking for better wording)

bit more clean-up

Added test and fixed existing to prevent throwing deprecation

The test failures show it. How do we trigger warnings without causing a lot of noise for tests?

+++ b/core/lib/Drupal/Core/Render/Element/HtmlTag.php
@@ -105,7 +105,13 @@ public static function preRenderHtmlTag($element) {
-    $suffix = isset($element['#suffix']) ? $close_tag . $element['#suffix'] : $close_tag;
+    if (isset($element['#suffix'])) {
+      $suffix = $close_tag . $element['#suffix'];
+    }
+    else {
+      $suffix = $close_tag . "\n";
+      @trigger_error('Addition of newline to HTML tags is deprecated in drupal:10.2.0 and is removed from drupal:11.0.0. Set #suffix to "\\n" to keep the existing behaviour. See https://www.drupal.org/node/2742955', E_USER_DEPRECATED);
+    }

This condition needs improvement at least a global kill-switch in settings.php to enable BC mode

Wonder if we just make this change in a minor, and a change record/release note is enough. Deprecating on unset #suffix is very noisy and I think 99% of users won't care that we make this change.

#102++

...in a light of https://wiki.php.net/rfc/opt_in_dom_spec_compliance

probably better to split out deprecation of #suffix into separate issue at least to unlock it