code review 5043042: exp/template/html: allow quotes on either side of condi...

exp/template/html: allow quotes on either side of conditionals and dynamic HTML names

This addresses several use cases:

(1) <h{{.HeaderLevel}}> used to build hierarchical documents.
(2) <input on{{.EventType}}=...> used in widgets.
(3) <div {{" dir=ltr"}}> used to embed bidi-hints.

It also makes sure that we treat the two templates below the same:

<img src={{if .Avatar}}"{{.Avatar}}"{{else}}"anonymous.png"{{end}}>
<img src="{{if .Avatar}}{{.Avatar}}{{else}}anonymous.png{{end}}">

This splits up tTag into a number of sub-states and adds testcases.

[MikeSamuel, 2011-09-17 02:14:47]

Hello nigeltao@golang.org (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[nigeltao, 2011-09-18 02:42:12]

http://codereview.appspot.com/5043042/diff/3/src/pkg/exp/template/html/conten...
File src/pkg/exp/template/html/content.go (right):

http://codereview.appspot.com/5043042/diff/3/src/pkg/exp/template/html/conten...
src/pkg/exp/template/html/content.go:28: // for example: dir="ltr".
Enclose the example in `` quotes.

http://codereview.appspot.com/5043042/diff/3/src/pkg/exp/template/html/escape.go
File src/pkg/exp/template/html/escape.go (right):

http://codereview.appspot.com/5043042/diff/3/src/pkg/exp/template/html/escape...
src/pkg/exp/template/html/escape.go:302: // forceEpsilon takes a context and
returns the context
Unfinished comment. The "takes a context" language may or may not read better if
this was a method, instead of a function with one arg.

http://codereview.appspot.com/5043042/diff/3/src/pkg/exp/template/html/escape...
src/pkg/exp/template/html/escape.go:358: if e.eq(c) {
Why isn't this just "d.eq(c)", or "c.eq(d)"?

http://codereview.appspot.com/5043042/diff/3/src/pkg/exp/template/html/escape...
src/pkg/exp/template/html/escape.go:364: // This concludes that
s/concludes/means/?

http://codereview.appspot.com/5043042/diff/3/src/pkg/exp/template/html/escape...
src/pkg/exp/template/html/escape.go:365: //   foo={{if .C}}{{.}}{{end}}
Maybe s/foo/<p foo/ to emphasize that this is for a tag attribute.

http://codereview.appspot.com/5043042/diff/3/src/pkg/exp/template/html/escape...
File src/pkg/exp/template/html/escape_test.go (right):

http://codereview.appspot.com/5043042/diff/3/src/pkg/exp/template/html/escape...
src/pkg/exp/template/html/escape_test.go:38: tests := []struct {
It's outside the scope of this CL, but it occurred to me that it might be
worthwhile for these tests to be defined in a ".dat" file, instead of in Go
code, so that it was possible to check that this Go implementation performed
identically to a C++ or JavaScript implementation, say. This would be similar to
the WebKit HTML parse tests that are copied to
$GOROOT/src/pkg/html/testdata/webkit/*.dat.

I'm happy to try extracting this myself, but (1) I'll wait while you're still
working on adding features / bug fixes, and (2) let me know if you've already
got something similar.

OTOH, if Go's template syntax isn't exactly the same as e.g. jstemplate's
syntax, then I'm not sure if it's worth worrying about for now.

[nigeltao, 2011-09-18 09:19:01]

Thinking out loud: "nudge" is shorter than "forceEpsilon", and might
work just as well. Or it might not.

[MikeSamuel, 2011-09-18 20:01:30]

Incremental diffs at
http://codereview.appspot.com/5043042/diff2/3:4/src/pkg/exp/template/html/con...

http://codereview.appspot.com/5043042/diff/3/src/pkg/exp/template/html/conten...
File src/pkg/exp/template/html/content.go (right):

http://codereview.appspot.com/5043042/diff/3/src/pkg/exp/template/html/conten...
src/pkg/exp/template/html/content.go:28: // for example: dir="ltr".
On 2011/09/18 02:42:12, nigeltao wrote:
> Enclose the example in `` quotes.

Done.

http://codereview.appspot.com/5043042/diff/3/src/pkg/exp/template/html/escape.go
File src/pkg/exp/template/html/escape.go (right):

http://codereview.appspot.com/5043042/diff/3/src/pkg/exp/template/html/escape...
src/pkg/exp/template/html/escape.go:302: // forceEpsilon takes a context and
returns the context
On 2011/09/18 02:42:12, nigeltao wrote:
> Unfinished comment. The "takes a context" language may or may not read better
if
> this was a method, instead of a function with one arg.

Renamed nudge per your later comment.

In general, do you prefer methods or functions.  I could change join & nudge to
be methods.

// nudge returns the context that would result from following empty string
// transitions from the input context.
// For example, parsing:
//     `<a href=`
// will end in context{stateBeforeValue, attrURL}, but parsing one extra rune:
//     `<a href=x`
// will end in context{stateURL, delimSpaceOrTagEnd, ...}.
// There are two transitions that happen when the 'x' is seen:
// (1) Transition from a before value to a start of value state without
//     consuming any character.
// (2) Consume 'x' and transition past the first value character.
// In this case, nudging produces the context after (1) happens.

http://codereview.appspot.com/5043042/diff/3/src/pkg/exp/template/html/escape...
src/pkg/exp/template/html/escape.go:358: if e.eq(c) {
This case is unnecessary.  The nudge below handles it.

http://codereview.appspot.com/5043042/diff/3/src/pkg/exp/template/html/escape...
src/pkg/exp/template/html/escape.go:364: // This concludes that
On 2011/09/18 02:42:12, nigeltao wrote:
> s/concludes/means/?

Done.

http://codereview.appspot.com/5043042/diff/3/src/pkg/exp/template/html/escape...
src/pkg/exp/template/html/escape.go:365: //   foo={{if .C}}{{.}}{{end}}
On 2011/09/18 02:42:12, nigeltao wrote:
> Maybe s/foo/<p foo/ to emphasize that this is for a tag attribute.

Done.

http://codereview.appspot.com/5043042/diff/3/src/pkg/exp/template/html/escape...
File src/pkg/exp/template/html/escape_test.go (right):

http://codereview.appspot.com/5043042/diff/3/src/pkg/exp/template/html/escape...
src/pkg/exp/template/html/escape_test.go:38: tests := []struct {
On 2011/09/18 02:42:12, nigeltao wrote:
> It's outside the scope of this CL, but it occurred to me that it might be
> worthwhile for these tests to be defined in a ".dat" file, instead of in Go
> code, so that it was possible to check that this Go implementation performed
> identically to a C++ or JavaScript implementation, say. This would be similar
to
> the WebKit HTML parse tests that are copied to
> $GOROOT/src/pkg/html/testdata/webkit/*.dat.
> 
> I'm happy to try extracting this myself, but (1) I'll wait while you're still
> working on adding features / bug fixes, and (2) let me know if you've already
> got something similar.
> 
> OTOH, if Go's template syntax isn't exactly the same as e.g. jstemplate's
> syntax, then I'm not sure if it's worth worrying about for now.

It would be great maintenance-wise, but every template language I've mucked with
has just enough minor syntactic differences that a common test-suite would
probably require writing tests in some kind of template language that the
testrunner translates into the language under test.

[nigeltao, 2011-09-19 01:10:42]

LGTM.

As for functions vs methods, there's no big difference either way. What you have
now reads fine.

http://codereview.appspot.com/5043042/diff/4/src/pkg/exp/template/html/contex...
File src/pkg/exp/template/html/context.go (right):

http://codereview.appspot.com/5043042/diff/4/src/pkg/exp/template/html/contex...
src/pkg/exp/template/html/context.go:315: return fmt.Sprintf("illegal attr %d",
a)
I think you need to change the final "a" to "int(a)". While you're here, please
fix for the other types in this file.

http://codereview.appspot.com/5043042/diff/4/src/pkg/exp/template/html/escape.go
File src/pkg/exp/template/html/escape.go (right):

http://codereview.appspot.com/5043042/diff/4/src/pkg/exp/template/html/escape...
src/pkg/exp/template/html/escape.go:311: // (1) Transition from a before value
to a start of value state without
Maybe put "" quotes around "before value" and "start of value", or add dashes to
write it as "from a before-value to a start-of-value state".

http://codereview.appspot.com/5043042/diff/4/src/pkg/exp/template/html/escape...
src/pkg/exp/template/html/escape.go:361: if c.state < b.state {
It's odd to compare c to b. Either compare a to b, or compare c to d.

Also, please add a little comment on why the swap is needed.

As it's evolved, each individual line of this join function still makes sense,
but I think I'm losing the overall picture a little.

http://codereview.appspot.com/5043042/diff/4/src/pkg/exp/template/html/escape...
src/pkg/exp/template/html/escape.go:370: if c, d := nudge(a), nudge(b);
!(c.eq(a) && d.eq(b)) {
s/nudge(a)/nudge(c)/ ??

http://codereview.appspot.com/5043042/diff/4/src/pkg/exp/template/html/escape...
File src/pkg/exp/template/html/escape_test.go (right):

http://codereview.appspot.com/5043042/diff/4/src/pkg/exp/template/html/escape...
src/pkg/exp/template/html/escape_test.go:450: `<img
on{{"load"}}="alert({{"loaded"}})">`,
Say, do we have a TestErrors test for `<img {{"onload"}}={{.X}}`? Escape has to
reject this, right?

[MikeSamuel, 2011-09-19 01:53:46]

http://codereview.appspot.com/5043042/diff/4/src/pkg/exp/template/html/contex...
File src/pkg/exp/template/html/context.go (right):

http://codereview.appspot.com/5043042/diff/4/src/pkg/exp/template/html/contex...
src/pkg/exp/template/html/context.go:315: return fmt.Sprintf("illegal attr %d",
a)
On 2011/09/19 01:10:43, nigeltao wrote:
> I think you need to change the final "a" to "int(a)". While you're here,
please
> fix for the other types in this file.

Done.

http://codereview.appspot.com/5043042/diff/4/src/pkg/exp/template/html/escape.go
File src/pkg/exp/template/html/escape.go (right):

http://codereview.appspot.com/5043042/diff/4/src/pkg/exp/template/html/escape...
src/pkg/exp/template/html/escape.go:311: // (1) Transition from a before value
to a start of value state without
On 2011/09/19 01:10:43, nigeltao wrote:
> Maybe put "" quotes around "before value" and "start of value", or add dashes
to
> write it as "from a before-value to a start-of-value state".

hyphenated.

http://codereview.appspot.com/5043042/diff/4/src/pkg/exp/template/html/escape...
src/pkg/exp/template/html/escape.go:361: if c.state < b.state {
On 2011/09/19 01:10:43, nigeltao wrote:
> It's odd to compare c to b. Either compare a to b, or compare c to d.
> 
> Also, please add a little comment on why the swap is needed.
> 
> As it's evolved, each individual line of this join function still makes sense,
> but I think I'm losing the overall picture a little.

The swap is no longer needed.  It thought it was needed to compare stateTag and
stateBeforeValue but that's handled by the below.
I got rid of the swap.

http://codereview.appspot.com/5043042/diff/4/src/pkg/exp/template/html/escape...
src/pkg/exp/template/html/escape.go:370: if c, d := nudge(a), nudge(b);
!(c.eq(a) && d.eq(b)) {
On 2011/09/19 01:10:43, nigeltao wrote:
> s/nudge(a)/nudge(c)/ ??

Nope, the swap was vestigial.

http://codereview.appspot.com/5043042/diff/4/src/pkg/exp/template/html/escape...
File src/pkg/exp/template/html/escape_test.go (right):

http://codereview.appspot.com/5043042/diff/4/src/pkg/exp/template/html/escape...
src/pkg/exp/template/html/escape_test.go:450: `<img
on{{"load"}}="alert({{"loaded"}})">`,
On 2011/09/19 01:10:43, nigeltao wrote:
> Say, do we have a TestErrors test for `<img {{"onload"}}={{.X}}`? Escape has
to
> reject this, right?

Yep.  And <img {{"style"}}>.
IIRC, those are in the testsuite I'm porting, but are not in this CL.

[MikeSamuel, 2011-09-19 02:10:20]

*** Submitted as http://code.google.com/p/go/source/detail?r=e357d4d799ec ***

exp/template/html: allow quotes on either side of conditionals and dynamic HTML
names

This addresses several use cases:

(1) <h{{.HeaderLevel}}> used to build hierarchical documents.
(2) <input on{{.EventType}}=...> used in widgets.
(3) <div {{" dir=ltr"}}> used to embed bidi-hints.

It also makes sure that we treat the two templates below the same:

<img src={{if .Avatar}}"{{.Avatar}}"{{else}}"anonymous.png"{{end}}>
<img src="{{if .Avatar}}{{.Avatar}}{{else}}anonymous.png{{end}}">

This splits up tTag into a number of sub-states and adds testcases.

R=nigeltao
CC=golang-dev
http://codereview.appspot.com/5043042