unquoted attribute at end of self-closing tag consumes trailing slash

[ianjosephwilson]

Confusing situation where we are missing the space before the self-closing component tag:

from string.templatelib import Template

from tdom import html


def AuthStatus(logged_in: bool = False) -> Template:
    msg = 'In' if logged_in else 'Out'
    return t'<div>{msg}</div>'


test_t = t'<{AuthStatus} logged_in={True}/>'

try:
    print (html(test_t))
except ValueError as e:
    print (e)


test_t = t'<div><{AuthStatus} logged_in={True}/></div>'

try:
    print (html(test_t))
except ValueError as e:
    print (e)


test_t = t'<div><{AuthStatus} logged_in={True} /></div>'

try:
    print (html(test_t))
except ValueError as e:
    print (e)

Invalid HTML structure: unclosed tags remain.
Mismatched closing tag </div> for component starting at AuthStatus.
<div><div>In</div></div>

[ianjosephwilson]

The interpolation just makes this seem like it should work but this same issue comes up with a regular unquoted attribute as well.

assert html(t'<div x=a />') == '<div x="a"></div>'
# raises ValueError: Invalid HTML structure: unclosed tags remain.
html(t'<div x=a/>') 

[davepeck]

Ugh, yup.

[ianjosephwilson]

I kind of think the best we could do here is just try to introspect things during the "unclosed tags remain" error and if it looks like there is a dangling "/" in the last attribute of that tag then we'd put in a different error message, ie. 'If you meant to self-close the tag {tag} then you need a space before the "/".'. Seems "not great" but trying to outguess the input seems worse. We could also just leave it as-is, ie. do nothing, and see how often it comes up.

That being said I think we might reconsider how we handle self closing tags because it is similar to react but it seems non-standard and might cause problems later on. Maybe we should only allow void elements, components and foreign elements to be self-closing. Then we could consider components to be "foreign elements". Maybe we were already doing that and I'm out of the loop but we would have guidance and could maybe add/change parsers later on.

syntax-start-tag

Then, if the element is one of the void elements, or if the element is a foreign element, then there may be a single U+002F SOLIDUS character (/), which on foreign elements marks the start tag as self-closing. On void elements, it does not mark the start tag as self-closing but instead is unnecessary and has no effect of any kind. For such void elements, it should be used only with caution — especially since, if directly preceded by an unquoted attribute value, it becomes part of the attribute value rather than being discarded by the parser.

[ianjosephwilson]

Maybe related here -- https://github.com/t-strings/tdom/blob/main/tdom/processor.py#L777 -- and furthermore maybe void slash thing should be off by default. Then we'd match the html spec better. Maybe this could be configurable though? Feels a little inconsistent.

>>> html(t'<div />')
'<div></div>'
>>> html(t'<div/>')
'<div></div>'
>>> html(t'<img src=path/>')
'<img src="path/" />'

[davepeck]

You know it's fun when you have cause to visit the HTML5 tokenization spec. For my notes: html5lib is a useful conforming parser; html5lib.HTMLParser(strict=True) raises on things browsers swallow.

Right now, tdom is inspired by JSX: everything self-closes. But, as this issue points out, tdom does this badly, because JSX doesn't allow unquoted attributes. We clearly do want to support unquoted attributes, which causes us to run up against something JSX avoids.

Given this, I think we probably need to revisit "everything self-closes" and be closer to HTML5-conforming.

A conforming parser treats <br x=a/> as a self-closing <br> with x="a/". The / stays in the attribute value; we self-close because br is void, not because of the ending />. On the other hand, <div /> does not self-close and is a parse error. We would need to treat <div x=a/> as an open <div> with x="a/".

For attribute interpolations without a space before />, I'm fine saying <br x={a}/> is a self-closing <br> with x=<value_of_a> and no slash. There's little ambiguity here. <div x={a}/> is an error, since div can't self-close.

Component invocation is our own to define. It sits outside the "void vs non-void" distinction; self-closing is a convenience feature. But it does seem like the straightforward answer is to maintain symmetry: <{Component} x=a/> is an open tag with x="a/", I suppose?

Ugh. How does that sit with you, @ianjosephwilson ?

[davepeck]

(We seem to have crossed streams here.)

[ianjosephwilson]

I think there is another variable I sort of implicitly mentioned here as well in that we actually put a trailing slash on void elements when we render them, even though it is explicitly ignored by the spec I think.

[davepeck]

I'm okay with explicitly rendering the trailing slash on void elements; I think that's visually easier to read.

[ianjosephwilson]

Yeah I think trying to go closer to the spec makes sense (as much as the spec makes sense). I was proposing we sort of hand-wavy consider the components more like "foreign elements" as defined by the spec. Then drop self closing void elements and non-void elements completely UNLESS they are svg/mathml or components.

https://html.spec.whatwg.org/multipage/syntax.html#foreign-elements

I guess rendering vs parsing are two different scenarios and the rendering could be configurable.

[ianjosephwilson]

If we don't allow self-closing non-foreign elements then can ignore some of the weird slash-in-attribute problems because that then only means one thing. Then for components we could either throw a specialized error or trying to auto-correct it for that special case.

[davepeck]

rendering could be configurable

Agree; the whole question of rendering opts feels like a separate issue, and a good thing to address.

[AhnafCodes]

tdom is inspired by JSX

But, its not JSX, ad JSX is popular/familiar but not a standard. since we're not bound to JSX, we're free to pick a stricter, less ambiguous rule.

we don't allow self-closing non-foreign elements then can ignore some of the weird slash-in-attribute problems because that then only means one thing. Then for components we could either throw a specialized error or trying to auto-correct it for that special case.

We could:

• NOT allow self-closing foreign(non-html) elements excepting component.
  <{Link} href="https://example.com" text="Example" data-value={42} target="_blank" />' ✅

<{MyComponent} id='comp1'> ✅ but NOR allow the<{True} /> <{False} />better be disallowed as we might have logged_in={True}/> atleast for avoiding of confusion and readablity.

✅ - custom-elements(like lithtml) is allowed html and also used with Angular.
So, can be left as is to be used on client-side Js.

NO auto-correction, at least now. As that could open door for a set of bugs.

Error or Warning for other things.
invalid html, like <div x=a /> better error out. Why let people generated html which leaves latter might result into intermittent issue due to browser's autocorrection. And worse this could be abused by used for other purpose.

[ianjosephwilson]

I should have looked into this more before bringing it up because I don't think there is a good way to ignore the self-closing for plain html tags because there is no way to tell if we are inside a "svg"/"math" context during parsing where self-closing should be allowed, ie. <circle />. We'd have to have some giant whitelist or something. So if what I said earlier made sense then we can't do that anyways. I think this gets us closer to something more consistent:

Potential Solution

Ex1: Just to reset a bit the single case that I think is problematic is this case because it will probably come up all the time:

t'<{Component} title={value}/>'

This would fail because title eats up the "/" as mentioned before but I think we could just put a check here and just not allow this for probably any form. The check seems to just be IF the parser calls handle_starttag (ie. not handle_startend_tag) AND the starttag_text ends with "/>" (ie. looks like it should have been self-closing) THEN error out. (This is in #136 )

Ex2: We'd recommend doing this:

t'<{Component} title={value} />'

Ex3: Although this should also work:

t'<{Component} title="{value}"/>'

Alternative Solution

Another approach is to try to allow this special case but override the default parsing and convert it from handle_starttag to handle_startendtag, ie. self-closing. (This is in #137 )

We'd need to check if the tag ended in /> and then we'd maybe just leave handle_starttag for handle_startend_tag. I guess this makes me a little nervous we'd somehow get caught in some parsing quagmire trying to figure out what to do with the attribute values. We'd have to remove the last attribute and re-construct it without the "/". This feels like it might get us stuck parsing ourselves but I'm not sure?