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
Change container tag check to void tag check #94
Conversation
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 Also, as an aside, could you keep the commit summary message under 70 characters, otherwise it gets truncated in most tools. |
The question of backwards compatibility boils down to how should Thanks for the heads up on commit message length. Seems like GitHub numbers among those tools. |
If the |
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. |
It depends on the In XHTML, the following tags are considered void:
In HTML5:
Since HTML5 doesn't contain
Currently Hiccup has three modes: Finally, we change the default mode to |
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? |
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. |
My only concern was ensuring that people using the |
Okay, cool. I'll rewrite the logic to reflect this thread and send another pull request. |
You could just rewrite the commits on your branch, and therefore use the same PR. Thanks for being patient with this process, btw. |
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 Are you suggesting we change the behavior or did I miss something? |
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. |
Roger that. The I wonder about the |
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 |
Sounds good. Let's go for that. |
I updated the branch. Should work as we discussed. |
Could you squash your commits? |
There you go. |
Change container tag check to void tag check
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.