validator / htmlparser

The Validator.nu HTML parser https://about.validator.nu/htmlparser/
Other
56 stars 26 forks source link

Streamline the warning about self-closing-tag syntax #75

Closed sideshowbarker closed 1 year ago

sideshowbarker commented 1 year ago

“widely discouraged” is subjective; and regardless, what’s actually relevant is what the spec states — so let’s focus the warning into closer alignment just with what the spec states, which is:

On void elements, [the solidus] 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.

Further, we don’t really need the statement about considering to switch tools; it makes the message longer than it needs to be.

sideshowbarker commented 1 year ago

Aside: Did this message start out as a "note"

It started out as a https://github.com/validator/htmlparser/blob/master/src/nu/validator/htmlparser/impl/ErrorReportingTokenizer.java#L155 “note” — which isn’t really a note but actually causes either a warning or an error.

Since what’s really getting called here is an org.xml.sax.ErrorHandler, I think with the current code we’re limited to just error(), fatalError(), and warn(), right?

and got upgraded to "warning" along the way? I don't know how to resolve the competing goals of not bothering authors about removing stuff that has no effect and is intentionally permitted and avoidance of active addition of useless syntax

In the checker code, at https://github.com/validator/validator/blob/main/src/nu/validator/messages/MessageEmitterAdapter.java#L660 we have the ability to call an info() message — as well as some other special message types, but setting different MessageType.INFO, etc., flags in the message calls.

But none of that is currently exposed to the htmlparser code — so from the htmlparser code, we are currently limited to just error/fatalError/warn.

However, I could look into figuring out if we can somehow plumb the htmlparser message handling into the checker’s specialized MessageType.INFO, etc., handling — when the htmlparser is called from the checker code.

I guess it might be possible to do that by calling setErrorHandler() on the checker’s nu.validator.htmlparser.sax.HtmlParser and setting it to the same MessageEmitterAdapter the checker service uses as its errorHandler?

Alternatively, I guess it should be possible to move the message handling for this completely out of the htmlparser code and into the checker code, where we already have the ability to make it an “info” note message.

But if we did that, we’d then need some way to expose the “this element has a trailing slash” from the htmlparser code to the checker code. I can’t right now imagine a very straightforward way to do that, but maybe there’s something I’ve overlooked.

sideshowbarker commented 1 year ago

Aside: Did this message start out as a "note" and got upgraded to "warning" along the way? I don't know how to resolve the competing goals of not bothering authors about removing stuff that has no effect and is intentionally permitted and avoidance of active addition of useless syntax.

OK, I figured out how we can make it get emitted as an Info note by the checker — rather than a warning.

I’ll make a patch for that for you to review.

It depends on doing a match against the message string — which is of course suboptimal and fragile in the face of any future changes to message string — but I can’t see any other way we could currently do it except by matching against the message string.

sideshowbarker commented 1 year ago

Aside: Did this message start out as a "note" and got upgraded to "warning" along the way? I don't know how to resolve the competing goals of not bothering authors about removing stuff that has no effect and is intentionally permitted and avoidance of active addition of useless syntax.

OK, I figured out how we can make it get emitted as an Info note by the checker — rather than a warning.

I’ll make a patch for that for you to review.

Done — please see https://github.com/validator/validator/pull/1436