Change container tag check to void tag check

[asmala]

As per discussion on #91, I'm submitting a pull request to change container tag checks to void tag checks. I had to update a few tests, since the default behavior is to render everything as a container tag unless proven otherwise. This may break backwards compatibility for anyone using non-standard void tags in their HTML.

[weavejester]

We can't break backward compatibility, unless we're certain that what we're doing is wrong (i.e. a bug).

We have access to the *html-mode* var, which tells us our rendering strategy. Currently we have :sgml, :xml and :html. We can do the right thing on :html, and perhaps add in a :xhtml to do the right thing for XHTML documents as well.

Also, as an aside, could you keep the commit summary message under 70 characters, otherwise it gets truncated in most tools.

[asmala]

The question of backwards compatibility boils down to how should [:my-own-tag] be rendered. Currently hiccup renders it as a self closing void tag, e.g."<my-own-tag />" in XML mode. No standard governs this tag so rendering it as "<my-own-tag></my-own-tag>" seems equally valid. If we want to keep the current behavior, then checking for void tags (as opposed to container tags) is not a viable approach. What do you prefer?

Thanks for the heads up on commit message length. Seems like GitHub numbers among those tools.

[weavejester]

If the *html-mode* is set to :html, then we can use the same rules as HTML5. I believe the rules for XHTML are similar, so we could add in a :xhtml mode as well.

[asmala]

Sorry, it seems I'm not communicating very clearly. What I meant to ask was how should empty non-HTML tags be treated? As void tags or as container tags?

The current implementation assumes void tag unless declared otherwise, whereas the pull request assumes container tag unless declared otherwise.

[weavejester]

It depends on the *html-mode*.

In XHTML, the following tags are considered void:

area, base, basefont, br, col, hr, img, input, isindex, link, meta, param

In HTML5:

area, base, br, col, embed, hr, img, input, keygen, link, menuitem, meta,
param, source, track, wbr

Since HTML5 doesn't contain basefont and isindex, it seems reasonable to combine the two lists:

area, base, basefont, br, col, embed, hr, img, input, isindex, keygen, link,
menuitem, meta, param, source, track, wbr

Currently Hiccup has three modes: :sgml, :xml and :html. I suggest adding one more mode, xhtml, that's treated the same as xml, but uses the above list of void tags. The :sgml and :xml modes should be changed to have no void tags - this might break compatibility in rare cases, but is the correct implementation.

Finally, we change the default mode to :xhtml, as it's the closest to the current behaviour.

[asmala]

Gotcha. That makes sense. And what about people mixing in their custom tags inside an HTML5 hiccup "document"? Currently those would get rendered as void tags. Are you okay with those getting rendered as container tags going forward?

[weavejester]

Yes, I think so, because if they're specifically choosing to render their documents in HTML5, they should obey the rules. In this case we can class the current behaviour as a bug, because it produces incorrect HTML.

[weavejester]

My only concern was ensuring that people using the :xml and :sgml modes are not affected.

[asmala]

Okay, cool. I'll rewrite the logic to reflect this thread and send another pull request.

[weavejester]

You could just rewrite the commits on your branch, and therefore use the same PR.

Thanks for being patient with this process, btw.

[asmala]

No problem, it's better for everyone if we get this right.

Now I'm confused about the modes, however. Based on what I'm hearing, you're suggesting the following behavior:

; Default mode is now :xhtml
(html [:br]) ;=> "<br />"
(html {:mode :xhtml} [:br]) ;=> "<br />"
(html {:mode :html} [:br]) ;=> "<br>"
(html {:mode :xml} [:br]) ;=> "<br></br>"
(html {:mode :sgml} [:br]) ;=> "<br></br>"

This would definitely break current behavior with :xml and :sgml modes, at least according to the tests.

Are you suggesting we change the behavior or did I miss something?

[weavejester]

No, the behaviour I mean would be:

(html [:br] [:p])                ;=> "<br /><p></p>"
(html {:mode :xhtml} [:br] [:p]) ;=> "<br /><p></p>"
(html {:mode :html} [:br] [:p])  ;=> "<br><p></p>"
(html {:mode :xml} [:br] [:p])   ;=> "<br /><p />"
(html {:mode :sgml} [:br] [:p])  ;=> "<br><p>"

That's correct for XHTML and HTML, technically correct for XML, and as correct for SGML as it can be.

[asmala]

Roger that. The :xhtml, :html, and :xml modes make sense.

I wonder about the :sgml mode, though. My memory is a bit fuzzy on this but doesn't SGML allow the omission of closing tags, meaning that a generalized SGML parser could interpret "<br><p>" as either [:br] [:p] or [:br [:p]]? If that's true, then I would suggest using an explicit closing tag for empty SGML tags to avoid the ambiguity.

[weavejester]

The SGML interpretation is ambiguous without a DTD, as far as I'm aware. Given that SGML is purely a legacy format, I'm not too concerned about supporting anything particularly complex with it. This interpretation allows people to write [:p] if they want <p> or [:p ""] if they want <p></p>. Adding a closing tag automatically would prevent people from using the first option.

[asmala]

Sounds good. Let's go for that.

[asmala]

I updated the branch. Should work as we discussed.

[weavejester]

Could you squash your commits?

[asmala]

There you go.