Critic review: https://critic.hoppipolla.co.uk/r/1752

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

Oops, I must have run the tests wrong—didn't see all those failures. I guess they imply that we want a ParseError with the specific character still intact.

It's deliberate. Our behaviour is what the spec defines. Go complain at Hixie if you want this changed!

I won't, but thanks :)

@gsnedders, would a patch containing a subclass of html5lib.tokenizer.HTMLTokenizer that does the mentioned replacement be a better approach? That way it would be optional.

@marciof if you're trying to sort out the lxml stuff, you just want to fix ihatexml and ensure everything for the tree-builder goes through it; if you want it for other reason, say what it is?

@gsnedders, I think I understand what you mean. etree_lxml uses ihatexml when building a tree for lxml, and ihatexml.InfosetFilter.coerceCharacters to clean up inserted text (among other things). So if we add code in the latter method to transform control characters, that will fix the problem.

If I understood you correctly before, the original patch was rejected because removing these characters violates the spec. If we change InfosetFilter, it is acceptable because using lxml and etree is optional—dom is still available to follow the spec.

Correct?

InfosetFilter by definition creates trees different to what the spec requires; it should roughly do what the spec says for infoset coercion. It should do all the coercion through finding invalid characters using ihatexml, and the fact that it doesn't is the bug.

Thanks, that clears it up!