Problem/Motivation

Currently insert command has following implementation:

      var new_content_wrapped = $('<div></div>').html(response.data);
      var new_content = new_content_wrapped.contents();
      if (new_content.length !== 1 || new_content.get(0).nodeType !== 1) {
        new_content = new_content_wrapped;
      }

When ajax command is "insert" (most of times) and returns a piece of render that produced with template then this code inserts this "div-wrapper" around new fragment.
This because HTML-tag + new line always detected as 2 nodes - ELEMENT_NODE + TEXT_NODE

Proposed resolution

Add condition about this case in ajax.js to allow templates have new line at the end

Remaining tasks

discus and file patch

User interface changes

no

API changes

no

Comments

markhalliwell’s picture

Title: new line in twig template leads to DOM duplication after AJAX » Trim trailing EOF whitespace from templates
Component: javascript » theme system
Category: Bug report » Task
Issue tags: -JavaScript, -frontend performance +Twig, +theme system cleanup, +dreammarkup
Related issues: +#1939214: Provide a Twig filter for cleaning up whitespace around classes. , +#1268180: Newline character in HtmlTag causes unwanted whitespace

This is actually kind of a feature request (this dilemma exists in 7.x as well), but I suspect it could be considered part of the theme system/twig changes (not JS related), so I'll mark it as a task (since it doesn't really break anything and really just a nice to have).

This is also rather minor and cosmetic. I really don't see how this "impacts performance" at all. Yes, there's a new line at the end of templates (mainly because of git's requirements: http://stackoverflow.com/questions/729692/why-should-files-end-with-a-ne... and http://robots.thoughtbot.com/no-newline-at-end-of-file), but whitespace (especially empty text nodes) doesn't really affect the DOM that much, as they're typically ignored and affects more of the stylistic aspects of the DOM (ie: CSS, which can also be controled).

The twig team has talked about this for quite a while (about a year), but has never actually "officially" addressed this issue until now. @joelpittet came up with a nifty "hack", so to speak, a while back that uses Twig's comments and effectively gets rid of the git required EOF newline. Using Twig comments, it strips all trailing whitespace until non-whitespace is detected. This is designed so that the docblocks at the beginning of templates don't introduce new whitespace:

{#\n
/**\n
  * ...\n
  */\n
#}\n
<img .... />{##}\n
\n

We have never really liked the idea of putting a comment on the last line of each file. We could make it part of Twig's parsing to just automatically trim each template, but that begs the question: is it necessary for every template? Would we start getting output like the following?

<div>
...
</div><button>
...
</button>

I'm not entirely sure the best approach here, none of us are. However, I think we might as well turn this into the official issue to discuss it and come up with a game plan.

edit: the reason this isn't a JS issue is because JS uses cases vary, one can easily use trim() around the string before it's sent back via ajax insert. That being said, if the templates automatically do this, then this manual process wouldn't be necessary.

star-szr’s picture

To me the easiest "fix" that would work for both PHPTemplate and Twig is to tweak $output near the end of _theme() to remove the last \n (or whatever).

markhalliwell’s picture

joelpittet’s picture

That hack that I used is from @see http://twig.sensiolabs.org/doc/templates.html#whitespace-control

The first newline after a template tag is removed automatically (like in PHP.) Whitespace is not further modified by the template engine, so each whitespace (spaces, tabs, newlines etc.) is returned unchanged.

And if we auto remove the last \n from the EOF we run into an issue where if we want that \n in the output we have to write two. \n\n Which I believe would also look weird and may produce a git error as well.

Personally I'd prefer, no hacks, no auto removals, just if possible, remove the git constraint *.twig files to require blank-at-eof.

nod_’s picture

but whitespace (especially empty text nodes) doesn't really affect the DOM that much, as they're typically ignored and affects more of the stylistic aspects of the DOM (ie: CSS, which can also be controled).

Just to point out that it's not the case. Whitespace in DOM is significant and as andypost explained even more in JS. It's multiple whitespace that's meaningless and treated as a single whitespace.

From a JS-Ajax perspective I'd want everything trimed. we talked about removing the condition in ajax.js to not cater to people trying to add several siblings nodes. That would be the right thing to do but that would break everything.

star-szr’s picture

Issue tags: +JavaScript

Re #4: \n\n (two blank lines at the end of a Twig template) might look weird but there wouldn't be any git errors and I can't see any reason to have templates like that in core anyway. I'd prefer that the newline at EOF rule in the coding standards should be as consistent as possible.

andypost’s picture

So maybe better to fix that in ajax.js just additionally checking for "if second DOM element is empty text" and remove it.
OTOH ajax could be able to pass text elements to page as well so maybe better to fix that some other way when ajax contend is rendered

andypost’s picture

So the only problem that coding standards said to have new lines at the end of templates?
Maybe better to fix standards and proceed here?

star-szr’s picture

It's not just coding standards, it's git too. It will show as "No newline at end of file" in every single diff.

joelpittet’s picture

So far we haven't run into an issue in the conversions that needed the trailing EOF whitespace removed explicitly. We've added new lines to many of the templates during the conversions. The only ones that would have any real effect in HTML would be inline elements AKA span tags in templates needing to butt up against another span tag.

<p>
  <span>template 1</span><span>template 2</span>
</p>
<p>
  <span>template 1</span>
  <span>template 2</span>
</p>

Results in

template 1template 2

template 1 template 2 
joelpittet’s picture

Status: Active » Closed (won't fix)

We've mostly got by without any hacks and you can use whitespace modifiers {{ output -}}

Triaging theme system for 8.0.0