Allow dashes in comments

[zcorpan]

• Make dashes in comments no longer be parse errors

  The spec disallowed dashes in comments in order to have conformance
  checkers point out the problem for pre-2006 Firefox that implemented
  SGML-style comment parsing, and to match the syntax requirements in
  HTML4 and XML. These are no longer relevant and instead people are
  getting unhelpful error messages when validating for markup like:

  </div><!-- end .foo-bar--baz -->

  • A comment that ends with a dash is now allowed: <!-- --->
  • A comment that contains -- is also allowed: <!-- -- -->
  • A comment that contains --! is also allowed: <!-- --! -->
  • The following are still parse errors: <!-->, <!--->, <!-- --!>

  Fixes Remove parse error for dashes in comments <!-- -- --> #1351.

  Also remove obsolete comments in the spec's source about rewinding
  when hitting EOF (see [Tokenization] Question about the security comment in comment end state #1058).

• Editorial: use more 'reconsume' in comment states
  This makes it easier to add new states to introduce a parse error
  for <!-- <!-- -->.

• Make nested comments a parse error
  Nesting comments like <!--<!---->--> is an authoring mistake since
  the comment will end on the first -->. Now that -- in comments is
  allowed, we need new states to make "<!--" in a comment a parse
  error.

---

cc @sideshowbarker @hsivonen @RReverser @inikulin @gsnedders

[zcorpan]

Stuff to convert to test cases:

errors
<!----!>
<!----!
<!----
<!--->
<!-----
<!-->
<!--
<!--x
<!--<
<!--<!
<!--<!-
<!--<!--
<!--<!--!
<!--<!--!>
<!--<!---
<!--<!--->
<!--<!--x
<!--<!--x-
<!--<!--x--
<!--<!--x-->
<!--<!-x
<!--<!-x-
<!--<!-x--
<!--<!x
<!--<!x-
<!--<!x--
<!--<<!--x-->
<!--<!<!--x-->
<!--<!-<!--x-->
<!----!->
<!----!x>
<!-----x>


no errors
<!---->
<!----!-->
<!----!x-->
<!----->
<!-----x-->
<!--x-->
<!--<!-x-->
<!--<!x-->
<!--<<!x-->
<!--<<!-x-->
<!--<x-->
<!--<>-->
<!--<-->
<!--<--->
<!--<!-->

(And also a variant of each test that has an "x" where the "x" is replaced with U+0000, and those are parse errors.)

(Edit: moved <!--<!--> to no errors per #1456 )

[kosek]

How this could be reserialized into XHTML? It just introduces additional discrepancy between HTML and XML syntaxes of HTML5.

[zcorpan]

https://html.spec.whatwg.org/multipage/xhtml.html#xml-fragment-serialisation-algorithm says to throw InvalidStateError. Happy to tweak those rules though if there is impl interest.

https://html.spec.whatwg.org/multipage/syntax.html#coercing-an-html-dom-into-an-infoset has suggestions for how to deal with that situation outside a browser.

[zcorpan]

Also need to update this bit

Errors that can result in infoset coercion
When a user agent based on XML is connected to an HTML parser, it is possible that certain invariants that XML enforces, such as comments never containing two consecutive hyphens, will be violated by an HTML file. Handling this can require that the parser coerce the HTML DOM into an XML-compatible infoset. Most syntax constructs that require such handling are considered invalid.

https://html.spec.whatwg.org/multipage/introduction.html#syntax-errors

(Edit: now fixed.)

[zcorpan]

<!--<!-->

This was previously without parse error, but with the new spec it is a parse error. I think that's OK.

I also realize now that the syntax section needs to disallow the comment data ending with <! or <!-. (Edit: now fixed.)

[RReverser]

Looks interesting to me, will take a more detailed look tomorrow when less sleepy :)

[hsivonen]

Making more things that can't be serialized in XHTML conforming in HTML makes me sad. I do realize that serializability to XHTML is of interest to fewer people than not getting errors for consecutive dashes in comments.

Still, I wish this change wasn't made and in general I wish we didn't tweak stuff like this anymore.

[RReverser]

@kosek @hsivonen Note that reserialization of the parsed HTML even into HTML already doesn't result in the same structure (see #1280). Those particular changes just change parse errors, but similar content was already allowed, and in 1.8 spec notes explicitly:

The DOM, the HTML syntax, and the XHTML syntax cannot all represent the same content. ... Comments that contain the string "-->" can only be represented in the DOM, not in the HTML and XHTML syntaxes.

So when serializing comments back, you probably already need to sanitize dashes inside.

[kosek]

@hsivonen I agree that changes to parsing rules should be very rare now. This is something what should be realy stable. Change should be supported by really strong use-case, and allowing two dashes is not IMHO strong use-case.

@zcorpan Strong use-case could be allowing nested comments. This is something what is really missing in both HTML and XML. In XML it will never be added. But this could be added to HTML and this would be of benefit to users. But change to parser algorithm would be much more distrupting, of course.

[RReverser]

Change should be supported by really strong use-case, and allowing two dashes is not IMHO strong use-case.

@kosek Note that this is not a change to the parsing algorithm in any way - this was already allowed and correctly parsed by any HTML5 compliant implementation; this just removes parsing error for validators which is obsolete due to the reasons outlined above; outside of the validators, everything works as earlier.

[zcorpan]

@kosek adding support for nested comments would be a change to the parsed tree, which most likely would be breaking Web content. So we can't do that.

@hsivonen OK. It is indeed a tradeoff between needing to do infoset coercion even for conforming HTML syntax when using an XML pipeline, and having conformance checkers not emit messages that are useless for the vast majority of authors (only have anecdotal evidence but from Twitter responses and on stackoverflow, it seems people generally don't understand why -- in comments is not allowed in the first place).

I am under the assumption that when you have a tool or UA with an XML pipeline with an HTML parser in one end, you will in general want to accept broken HTML also, which can already have dashes in comments and so you need to do infoset coercion anyway. But I could be wrong about this of course. Are there examples where this is not the case?

[hsivonen]

@RReverser, as you say, #1280 was already a conformance error. In general, it's sad to have non-round-trippable conforming documents. We should treat such cases as spec bugs if feasible (i.e. make those cases non-conforming if feasible).

@zcorpan, yes, it's likely that HTML consumers that use an XML pipeline internally would want to do infoset coercion regardless of the conformance status of the input.

[inikulin]

I like the change. The bad part is that we need to introduce 3 new state just to support parse error (about which AFAIK most implementers don't care). But I guess it's the only way to go, so lgtm.

[zcorpan]

An implementation that doesn't care about parse errors can indeed optimize away the new states (and a bunch of other states elsewhere in the parser, too).

[domenic]

I am not terribly involved in the finer points of document conformance, and so happy to defer to @zcorpan and others as the experts. But as an editor my general thoughts are:

• We should feel free to change document conformance requirements, even ones involving parser states and such, in the service of making authors' lives better by giving them less spurious errors. That is, the fact that this is parser-related should not make us more hesitant to change it.
• Thus the deciding factor should be whether we think this error is spurious or important.
• We should make a principled stand on whether it's important to defining "conforming HTML" such that conforming HTML can be round-tripped into serialized XHTML.
  • As @hsivonen pointed out, in general HTML -> XHTML conversion tools will not actually use the conformance requirements to reject non-conforming documents, so this does not simplify their operation.
  • I guess the hypothetical scenario here is someone consuming other peoples' HTML (not XHTML) documents, who cares about round-trippability to XHTML, but doesn't care about non-conformant HTML documents?
  • In general that population seems likely very small to me so I am not sure we should take a principled stand in favor of their use cases.

So I am in favor of this change, although again I defer to those more involved than I.

[zcorpan]

As a general rule, I think anything that can't be serialized to XML should also not be allowed in the HTML syntax, because compat with XML tools is sometimes useful and aligning with XML's rules typically doesn't hurt (e.g. restricting the allowed characters in element names or attribute names). But in this case it appears that this rule hurts more than it helps, so I think we should deviate from it.

I'd like to wait for @sideshowbarker to have an experimental implementation before merging this though.

[sideshowbarker]

https://checker.html5.org/ now has experimental support conforming to the spec text in this PR branch.

Specifically https://checker.html5.org/ behaves as expected for all the test cases in #1356 (comment).

errors
<!----!>

For that case https://checker.html5.org/ emits a new specific error:

Error: --!> found at end of comment (should just be -->).

Suggestions welcome for any improvements to error-message wording.

The error-message wording for the <!---in-comment case is:

Saw <!-- within a comment. Probable cause: Nested comment (not allowed).

Suggestions for any improvements to that wording also welcome.

[sideshowbarker]

https://checker.html5.org/parsetree/ can be used to view a serialization of the resulting parse tree for, e.g., any of the cases in #1356 (comment).

[zcorpan]

Nice work @sideshowbarker! I've confirmed the errors/no errors are reported as expected.

I found a bug in the parsed tree:

https://checker.html5.org/parsetree/?parser=html5&content=%3C%21--%3C%21-&submit=Print+Tree

vs.

http://software.hixie.ch/utilities/js/live-dom-viewer/saved/4265

i.e. the comment data is <!- instead of <!.