Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarify the fake token step in fragment parsing #9430

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

zcorpan
Copy link
Member

@zcorpan zcorpan commented Jun 15, 2023

Fixes #9428


馃挜 Error: Wattsi server error 馃挜

PR Preview failed to build. (Last tried on Jun 15, 2023, 2:18 PM UTC).

More

PR Preview relies on a number of web services to run. There seems to be an issue with the following one:

馃毃 Wattsi Server - Wattsi Server is the web service used to build the WHATWG HTML spec.

馃敆 Related URL

If you don't have enough information above to solve the error by yourself (or to understand to which web service the error is related to, if any), please file an issue.


<p>Let this start tag token be the start tag token of the <var
data-x="concept-frag-parse-context">context</var> node, e.g. for the purposes of determining
data-x="concept-frag-parse-context">context</var> node. This token is only used for determining
if it is an <span>HTML integration point</span>.</p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you turn this into a single sentence along the lines I suggested? Using "Let" to set an internal member of the context element (or node, we should be consistent) is weird.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

I vaguely wonder if we should run this by at least one other parser expert, e.g., Henri, but if you feel confident I think that ought to suffice.

@zcorpan zcorpan requested a review from hsivonen June 15, 2023 15:04
@hsivonen
Copy link
Member

hsivonen commented Jul 4, 2023

We currently have interop on this point: The attributes of the context node are ignored: https://software.hixie.ch/utilities/js/live-dom-viewer/saved/11842

It seems like a bug, but given that it's interoperable, do we want to keep it that way?

In any case, implementation-wise, I don't like writing the spec in the way that copies all the attributes if we only look at one (or currently, as implemented, none). I'd much prefer taking the local name for the context node, the namespace for the context node, and whatever flags actually need to be inspected. Currently in Gecko, there's a flag for quirks mode. If we decide we want to change the browser behavior here, I'd prefer to have a flag that true iff the context node is annotation-xml in the MathML namespace and it has the attribute encoding whose value is an ASCII-case-insensitive match for either "text/html" or "application/xhtml+xml".

That way, it would be clear that the fragment parsing algorithm doesn't actually need a copy of all the attributes.

@annevk
Copy link
Member

annevk commented Jul 4, 2023

That's true, but then we'd have to change a whole lot more, to pass through that flag and make it do the correct thing. We could make it more precise though and only copy that attribute and only if it has one of those values. That seems like a more minimal fix that still has the desired effect.

And it does seem bad that this would not work as expected, so I'd recommend we all fix it.

Also, I'm guessing we can't turn annotation-xml into a general HTML sink (i.e., ignore the encoding attribute and assume it contains HTML), though maybe @bkardell or @fred-wang could say more about that.

cc @mfreed7

@annevk annevk added clarification Standard could be clearer topic: parser labels Jul 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clarification Standard could be clearer topic: parser
Development

Successfully merging this pull request may close these issues.

HTML fragment parser creates a token which is only used for <annotation-xml encoding>
3 participants