Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

proposal: html/template: auto trim whitespace #20879

Closed
carl-mastrangelo opened this issue Jul 1, 2017 · 20 comments
Closed

proposal: html/template: auto trim whitespace #20879

carl-mastrangelo opened this issue Jul 1, 2017 · 20 comments

Comments

@carl-mastrangelo
Copy link
Contributor

carl-mastrangelo commented Jul 1, 2017

In Go 1.6, text/template was modified to be able to automatically trim whitespace if the - character was added to the beginning or end of a template directive. This is really useful, but using it can make the template more verbose than it could be.

For example, when templating html, precise whitespace control is necessary (html rendering is whitespace dependent). Option 1 is to litter html with <!-- --> comments, to trim whitespace between elements. This greatly obscures the original logic and bracing:

<div class="index"><!--
  -->{{template "nav" .}}<!--
  -->{{if .Pic}}<!--
  --><ul class="thumbnail-list"><!--
    -->{{range .Pic}}<!--
    --><li><!--
      --><div class="thumbnail-cntr"><!--
        --><a href="{{$p.Viewer .Id}}"><!--
          --><img 
	          class="thumbnail{{if .PendingDeletion}} deleted{{end}}"
	          src="{{$p.PicThumb .ThumbnailRelativeUrl}}" /><!--
	      --></a><!--
      --></div><!--
    --></li><!--
    -->{{end}}<!--
  --></ul><!--
  -->{{end}}<!--
  -->{{template "nav" .}}<!--
--></div>

Additionally, it doesn't work for wrapped html elements like the img tag.

Option two is use whitespace consuming template directives:

<div class="index">
  {{- template "nav" . -}}
  {{- if .Pic -}}
  <ul class="thumbnail-list">
    {{- range .Pic -}}
    <li>{{- /**/ -}}
      <div class="thumbnail-cntr">{{- /**/ -}}
        <a href="{{$p.Viewer .Id}}">{{- /**/ -}}
          <img {{/**/ -}}
	          class="thumbnail{{if .PendingDeletion}} deleted{{end}}" {{/**/ -}}
	          src="{{$p.PicThumb .ThumbnailRelativeUrl}}" />{{- /**/ -}}
	      </a>{{- /**/ -}}
      </div>{{- /**/ -}}
    </li>{{- /**/ -}}
    {{- end -}}
  </ul>
  {{- end -}}
  {{- template "nav" . -}}
</div>

This is cleaner than the html based comment since there is no line wrapping comments, and works with wrapped elements. However, it still includes a lot of - characters.

Proposal

To this end, I would like to raise the idea of adding a complementary form of templates directives which use '+' delimiters, and introduce a template.Option string trim which will implicitly make all directives act like {{- pipeline -}}. For example the code above would become:

<div class="index">
  {{template "nav" .}}
  {{if .Pic}}
  <ul class="thumbnail-list">
    {{range .Pic}}
    <li>{{/*trim*/}}
      <div class="thumbnail-cntr">{{/*trim*/}}
        <a href="{{$p.Viewer .Id}}">{{/*trim*/}}
          <img {{+ /*trim*/}}
	          class="thumbnail{{if .PendingDeletion}} deleted{{end}}" {{+ /*trim*/}}
	          src="{{$p.PicThumb .ThumbnailRelativeUrl}}" />{{/*trim*/}}
	      </a>{{/*trim*/}}
      </div>{{/*trim*/}}
    </li>{{/*trim*/}}
    {{end}}
  </ul>
  {{end}}
  {{template "nav" .}}
</div>

The comment directives would still be necessary to drop whitespace between lines, but all the - dash characters can then be removed. This makes it easier to see the layout and structure of the template without worrying about whitespace.

@odeke-em
Copy link
Member

odeke-em commented Jul 2, 2017

/cc @golang/proposal-review

@bradfitz
Copy link
Contributor

bradfitz commented Jul 2, 2017

@odeke-em, no need to cc the proposal-review group. We go through various search queries to find stuff. The Proposal label is enough. Thanks.

@odeke-em
Copy link
Member

odeke-em commented Jul 3, 2017

Gotcha, thanks @bradfitz!

@rsc
Copy link
Contributor

rsc commented Jul 17, 2017

Even with the global option you still have to go through and add {{/*trim*/}} where there otherwise are not tags. Why not push the output through a proper HTML minifier? It might make sense to put that into html/template, since it is already partly parsing HTML and maybe has enough context, but maybe not (I don't know if it knows about <pre> tags, for example).

Does it make sense to adjust this proposal to be about exposing some kind of HTML compaction in html/template, a little like json.Compact?

@rsc rsc changed the title text/template: Proposal to auto trim whitespace proposal: html/template: auto trim whitespace Jul 17, 2017
@gopherbot gopherbot added this to the Proposal milestone Jul 17, 2017
@carl-mastrangelo
Copy link
Contributor Author

@rsc I would LOVE that, but I assumed that it wasn't done for reasons. I was kind of surprised that the x/net/html isn't vendored in and used by html/template. It seems like x/net/html would be in a good position to do such minification.

@rsc
Copy link
Contributor

rsc commented Jul 17, 2017

OK, so html/template certainly understands HTML well enough to know what tags it is inside. What/where are the rules for HTML minification? I know <pre> content can't be trimmed. Anything else?

@carl-mastrangelo
Copy link
Contributor Author

This issue somewhat covers this: #3164. Minification is useful, but this issue is more concerned with trimming, which minification can't always do. For example,

<img src="a.jpg"/> <img src="b.jpg"/>

Isn't the same as

<img src="a.jpg"/><img src="b.jpg"/>

A minifier can't produce the latter because it would be a behavior change. The whitespace will show up in the rendered page. The <li>{{/*trim*/}} example in my post above shows this. The alignment problems would persist.

@spf13
Copy link
Contributor

spf13 commented Jul 18, 2017

@bep what do you think about this? Would this feature help?

@bep
Copy link
Contributor

bep commented Jul 18, 2017

@spf13 It would not solve any real problem that I have experienced; the - syntax added in 1.6 is a little cumbersome, but it works good enough for Hugo (which is a big consumer of Go templates).

I notice that this issue is targeting html/template; and while it may be true that "html rendering is whitespace dependent", I would say that this issue is more relevant for the text/template package. Chrome is more whitespace forgiving than, say, a record based text parser. But even there I would think hard before adding more manual syntax.

@carl-mastrangelo
Copy link
Contributor Author

One alternative would be to redefine this partially: Make an option to trim inter-element whitespace. For example:

<p>A</p> <p>B</p> would become
<p>A</p><p>B</p>

But

<p>A <i>C</i> </p> would become
<p>A <i>C</i></p>

This would alleviate the need for the {+ +} syntax, and mostly solve the formatting problem (and leave the door open for minifies as an orthogonal concern.)

@rsc
Copy link
Contributor

rsc commented Jul 31, 2017

Per discussion with @spf13 and @golang/proposal-review:

  • We can't change the default (clearly).
  • It's unfortunate to make everyone annotate their templates for compactness, as in {{/*trim*/}} or {{- /**/ -}}, at least everywhere whitespace appears.
  • It would be nice to provide some opt-in way for html/template to minimize the HTML output, but we need a spec for when the spaces can be removed entirely and when one space must be retained.
  • In the cases where one space must be retained, we'd still need some way to override that, and the existing {{- /**/ -}} seems OK for that.

Can anyone point at a spec for when we can safely remove spaces between elements vs when one must be preserved?

Does the general approach (opt-in "as small as possible without changing meaning" and then reuse the existing syntax for pulling out the remaining spaces) seem OK?

@rsc
Copy link
Contributor

rsc commented Aug 14, 2017

Does anyone know of a spec for when we can safely remove spaces between elements vs when one must be reserved? Or are there CSS properties that can turn an element (say, div) from whitespace-ignoring to whitespace-caring (like pre)?

@rsc
Copy link
Contributor

rsc commented Aug 14, 2017

@spf13 found it: css "white-space: pre" makes writing a spec basically impossible.

@rsc rsc closed this as completed Aug 14, 2017
@rsc rsc reopened this Aug 14, 2017
@rsc
Copy link
Contributor

rsc commented Aug 14, 2017

Wrong button.

@carl-mastrangelo
Copy link
Contributor Author

carl-mastrangelo commented Aug 14, 2017

@rsc I think you are bloating this proposal. We won't be able to implement it. Looking at the html5 spec for parsing, there is a formally defined parsing standard for doing this. But, it's complicated, and only applies to html5, and will be much much harder to do. I don't think you want that in the standard library.

What I don't understand is why you are mixing two issues into one? The distinct problems at hand are template clarity and control, not really conserving html output characters. Even if minification was done perfectly in Go, it still wouldn't solve this issue.

Edit: There is a WHATWG link with references to all the places whitespace collapsing and stripping happens: https://html.spec.whatwg.org/multipage/infrastructure.html#dependencies I don't know if this is exhaustive.

@rsc
Copy link
Contributor

rsc commented Aug 15, 2017

Fair enough, that's why the proposal is still open, because we now know that minifying can't be done / isn't enough. But it seems clear that we don't want to have to write templates that look like:

<div class="index">
  {{template "nav" .}}
  {{if .Pic}}
  <ul class="thumbnail-list">
    {{range .Pic}}
    <li>{{/*trim*/}}
      <div class="thumbnail-cntr">{{/*trim*/}}
        <a href="{{$p.Viewer .Id}}">{{/*trim*/}}
          <img {{+ /*trim*/}}
	          class="thumbnail{{if .PendingDeletion}} deleted{{end}}" {{+ /*trim*/}}
	          src="{{$p.PicThumb .ThumbnailRelativeUrl}}" />{{/*trim*/}}
	      </a>{{/*trim*/}}
      </div>{{/*trim*/}}
    </li>{{/*trim*/}}
    {{end}}
  </ul>
  {{end}}
  {{template "nav" .}}
</div>

Writing {{/*trim*/}} is not obviously better than {{- /**/ -}}. Rob suggested maybe we could shorten the latter to {{- -}} or {{-}}.

@carl-mastrangelo
Copy link
Contributor Author

@rsc {{- -}} would be an improvement, but it would need some work. For example, if the _ character could be used as a sort of "no output" character, we could write:

  • {{_ -}} to trim right
  • {{- _}} to trim left
  • {{-_-}} to trim both
  • {{_}} to represent empty, equivalent to {{/**/}}

The reason for _ is to be shorter than /**/, which is more jarring. Currently template treats _ as a function.

I don't {{-}} would be usable. That would complicate the example on this line:

          <img {{-}}
	          class="thumbnail{{if .PendingDeletion}} deleted{{end}}"

because it would squash the tag into <imgclass=....

@rsc
Copy link
Contributor

rsc commented Aug 15, 2017

To be clear, what you have to write today is:

<div class="index">
  {{- template "nav" . -}}
  {{- if .Pic -}}
  <ul class="thumbnail-list">
    {{- range .Pic -}}
    <li>{{- "" -}}
      <div class="thumbnail-cntr">{{- "" -}}
        <a href="{{$p.Viewer .Id}}">{{- "" -}}
          <img {{"" -}}
	          class="thumbnail{{if .PendingDeletion}} deleted{{end}}" {{"" -}}
	          src="{{$p.PicThumb .ThumbnailRelativeUrl}}" />{{- "" -}}
	      </a>{{- "" -}}
      </div>{{- "" -}}
    </li>{{- "" -}}
    {{- end -}}
  </ul>
  {{- end -}}
  {{- template "nav" . -}}
</div>

And your proposal is to make the {{- ... -}} the default (under a given mode), where you have to say + to opt out of the default, so that the template would shorten to:

<div class="index">
  {{template "nav" .}}
  {{if .Pic}}
  <ul class="thumbnail-list">
    {{range .Pic}}
    <li>{{""}}
      <div class="thumbnail-cntr">{{""}}
        <a href="{{$p.Viewer .Id}}">{{""}}
          <img {{" "}}
	          class="thumbnail{{if .PendingDeletion}}{{" "}}deleted{{end}}"{{" "}}
	          src="{{$p.PicThumb .ThumbnailRelativeUrl}}" />{{""}}
	      </a>{{""}}
      </div>{{""}}
    </li>{{""}}
    {{end}}
  </ul>
  {{end}}
  {{template "nav" .}}
</div>

where the {{" "}} could be instead {{+ ""}} or {{"" +}} depending on whether they had the right spaces on either side to reuse. It's not obvious to me this is a net win. The shortening of the main control flow elements seems to me clearer in the original. Worse, when you get down to small items, I think the default ends up hurting and causing difficult-to-notice bugs. For example, the original template said:

class="thumbnail{{if .PendingDeletion}} deleted{{end}}"

but the auto-trimmed one that you posted said:

class="thumbnail{{if .PendingDeletion}} deleted{{end}}" {{+ /*trim*/}}

See the bug? The space in " deleted" just got eaten. You'd need to write something like:

class="thumbnail{{if .PendingDeletion +}} deleted{{end}}" {{+ /*trim*/}}
class="thumbnail{{if .PendingDeletion}}{{" "}}deleted{{end}}"{{" "}}

Honestly I'd rather have the - signs in the outer structure than deal with subtle bugs like this in the inner structure. I don't see a compelling win here for flipping the default, and I do see new complexity.

@carl-mastrangelo
Copy link
Contributor Author

I see your point about the subtle bug. CSS classes have to be space separated which forces that. Stepping back, do you even agree with me that there is a problem in the template code today? Specifically, do you agree that the top two examples in the OP are ugly and/or could benefit from a cleaner template? If not, then I guess there is nothing more to do here.

@rsc
Copy link
Contributor

rsc commented Oct 9, 2017

After all the back and forth, I don't see a viable path forward here. Systems that know the entire context (CSS in effect and so on) might be able to do something, but it's not clear that we can. Post-processing with any other kind of HTML minifier (outside this package) seems like a better solution than adding complexity to the API here.

Going to mark this closed, as suggested in previous comment.

@rsc rsc closed this as completed Oct 9, 2017
@golang golang locked and limited conversation to collaborators Oct 9, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants