ryansolid / dom-expressions

A Fine-Grained Runtime for Performant DOM Rendering
MIT License
863 stars 125 forks source link

Add more elements to alwaysClose #229

Closed nicketson closed 1 year ago

nicketson commented 1 year ago

Found some more elements that are broken when not closed.

nicketson commented 1 year ago

I searched through https://html.spec.whatwg.org/ for all elements that had "Neither tag is omissible" for https://html.spec.whatwg.org/#concept-element-tag-omission

Surprisingly div and span are among this rather large list.

ryansolid commented 1 year ago

At minimum we have the full list here. That's good. I do wonder though under what conditions they aren't omissible. I'm trying to decide if we should just do the full list, or try to patch up the ones that appear to be problematic. In any case this shortens the potential list.

EDIT: I reviewed the list using the patterns I've seen to fail closing and found several more ones that should have been added thanks to this list. Not to say I have every case out there.

One common piece in this list is, most block elements will break this omission if children of an inline element. Technically that isn't legal but browsers cure it.

nicketson commented 1 year ago

EDIT: I reviewed the list using the patterns I've seen to fail closing and found several more ones that should have been added thanks to this list. Not to say I have every case out there. ul and ol absolutely should be added at a minimum, they are definitely broken.

In my opinion either the full list should be used or no closing tags should be omitted at all. I'm leaning slightly towards not omitting them. When these bugs occur, they could be very confusing for anyone not familiar with how dom-expressions works under the hood.

nicketson commented 1 year ago

I've thought about this issue a little bit more and I think that if tag omission is done then the opposite approach needs to be taken: instead of having a list of elements that need to be closed, there should be a (much smaller) list of elements that allow closing tag omission (along with checking the conditions under which omission is allowed).

ryansolid commented 1 year ago

Yeah. I think one thing we have going for us is that some of these scenarios the browser corrects and screws us over anyway. So we have compiler checks that prevent code being written that way for a certain number of these cases. I'm going to do another run through of the list to double check against that. Because there might be a few the browser doesn't correct and in that case I need to add them. If that list is sufficiently large then I tend to agree with what you said.

EDIT: Yeah.. it looks like the browser will let you put a block element inside an inline element but if you don't close that block element stuff gets messy. I thought these would get corrected out, but they don't in most cases. Which means pretty much every block element is in the list. But on the positive this state is detectable. So I should be able to narrow down where it is applied.

EDIT2: I noticed the comment around ol and ul. Where you found this was ol or ul inside an inline element by chance? Like a span.

In general: I think the one thing that is always safe is if there are no siblings after at any parent level. I've also noticed that inside block elements all except the really messed up ones (the shortlist we currently have) seem to work. The problem is mostly that the browser lets us put block elements in inline elements and that breaks everything.

nicketson commented 1 year ago

EDIT2: I noticed the comment around ol and ul. Where you found this was ol or ul inside an inline element by chance? Like a span.

In this case no, they were inside of a div.

ryansolid commented 1 year ago

In this case no, they were inside of a div.

You wouldn't happen to have an example of this. I've never seen any issue with ol or ul when under a div. I've tried multiple variations. What browser are you using? I've been going through all the reproductions the last couple days and the latest version (0.36.7) seems to have fixed them all. So just want to make sure I'm not missing something.

All that being said at this point the distinction between the original PR and what I'm doing is so minor it is tempting to just go with the original since that leaves no doubts. I'm just very curious.

nicketson commented 1 year ago

In this case no, they were inside of a div.

You wouldn't happen to have an example of this. I've never seen any issue with ol or ul when under a div. I've tried multiple variations. What browser are you using? I've been going through all the reproductions the last couple days and the latest version (0.36.7) seems to have fixed them all. So just want to make sure I'm not missing something.

I only had a couple minutes to sanitize and throw this together but it looked something like this:

    const blah = (
      <div>
        <div>
          <div>
            <button>
              <span>
                <span>blah</span> <span>blah</span>
              </span>
            </button>
            <ul>
              <li><a href="">asdfasdsad</a></li>
              <li><a href="">asdfasdsad</a></li>
              <li><a href="">asdfasdsad</a></li>
              <li><a href="">asdfasdsad</a></li>
            </ul>
          </div>

          <div>
            <button>
              <span>
                <span>blah</span> <span>blah</span>
              </span>
            </button>
            <ul>
              <li><a href="">asdfasdsad</a></li>
              <li><a href="">asdfasdsad</a></li>
            </ul>
          </div>

          <div>
            <button>
              <span>
                <span>blah</span> <span>blah</span>
              </span>
            </button>
            <ul>
              <li><a href="">asdfasdsad</a></li>
              <li><a href="">asdfasdsad</a></li>
              <li><a href="">asdfasdsad</a></li>
            </ul>
          </div>

          <div>
            <button>
              <span>
                <span>blah</span> <span>blah</span>
              </span>
            </button>
            <ul>
              <li><a href="">asdfasdsad</a></li>
              <li><a href="">asdfasdsad</a></li>
              <li><a href="">asdfasdsad</a></li>
            </ul>
          </div>

          <div>
            <button>
              <span>
                <span>blah</span> <span>blah</span>
              </span>
            </button>
            <ul>
              <li><a href="">asdfasdsad</a></li>
              <li><a href="">asdfasdsad</a></li>
            </ul>
          </div>
        </div>
        <div />
      </div>
    );

This was with the commit right before my pull request branch was forked, not sure if one of your commits since fixed this particular case in some way.

ryansolid commented 1 year ago

Yeah it appears fine now.

Some updates. I discovered fieldset is one of the always close elements and it wasn't in the list. Also some environments like Happy DOM can't handle most of the cases. So it definitely makes it tricky. I've added an option to disable this template shrinking.

ryansolid commented 1 year ago

I changed the behavior to never close nested things without opt in. So this issue was valuable but won't be merged. Thank you so much.