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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stop reporting redundant fragment parser error in foreign content #9809

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

not-my-profile
Copy link
Contributor

@not-my-profile not-my-profile commented Sep 29, 2023

I have tested this change against the test cases from html5lib-tests.

In document parsing this doesn't change any test results.

In fragment parsing this fixes 7 test cases by reporting one parser error less. (That is all existing HTML parsers that pass html5lib-tests must have already implemented these steps in the order they are changed to by this commit.)

The test cases where one error is reported less fall in two categories:

  • fragment parsing e.g. <svg></table> with a tbody HTML element as the context element => one "unexpected end tag" error is reported less

  • fragment parsing e.g. <g></path> with a path SVG element as the context element => one "found special tag while closing generic tag" error is no longer reported

Fixes html5lib/html5lib-tests#173.


/parsing.html ( diff )

@not-my-profile not-my-profile force-pushed the fix-redundant-fragment-parser-error-in-foreign-content branch 2 times, most recently from d119c56 to d04ee3d Compare September 29, 2023 06:57
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.

@hsivonen can you take a look please?

source Show resolved Hide resolved

<li><p>Otherwise, process the token according to the rules given in the section corresponding
to the current <span>insertion mode</span> in HTML content.</li>
<li><p>Return to the step labeled <i>loop</i>.</p></li>
Copy link
Member

Choose a reason for hiding this comment

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

(Rewriting this set of steps as a https://infra.spec.whatwg.org/#iteration-while "While true" could be nice.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Personally I don't really find "While true" any better than "loop" ... it's just as undescriptive regarding when the loop will terminate. In any case I think such a change should be done in a followup PR ... so that the diff of this clarification is minimal.

Copy link
Member

Choose a reason for hiding this comment

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

While true with nested steps is much better than goto imo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes that's true ... there however aren't any nested steps here.

Copy link
Member

Choose a reason for hiding this comment

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

Right, @annevk suggests switching from goto without nested steps to while true with nested steps.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right ... I think Anne meant that the real advantage of while is if you have multiple nested loops that is a loop within a loop, which as I pointed out isn't the case here.

In any case as I said I'd like to keep the changes in this commit minimal (since it fixes something that's wrong) and keep editorial changes to future PRs.

@annevk annevk added topic: parser clarification Standard could be clearer labels Sep 29, 2023
I have tested this change against the test cases from html5lib-tests.

In document parsing this doesn't change any test results.

In fragment parsing this fixes 7 test cases by reporting one parser
error less. (That is all existing HTML parsers that pass html5lib-tests
must have already implemented these steps in the order they are changed
to by this commit.)

The test cases where one error is reported less fall in two categories:

* fragment parsing e.g. `<svg></table>` with a `tbody` HTML element as
  the context element => one "unexpected end tag" error is reported less
  (previously it was reported twice)

* fragment parsing e.g. `<g></path>` with a `path` SVG element as the
  context element => one "found special tag while closing generic tag"
  error is no longer reported

Fixes html5lib/html5lib-tests#173.
@not-my-profile not-my-profile force-pushed the fix-redundant-fragment-parser-error-in-foreign-content branch from d04ee3d to 1d479e3 Compare September 29, 2023 08:02
@domenic domenic requested review from zcorpan and hsivonen October 24, 2023 01:20
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.

7 tests assume non-compliant step-order in foreign content
4 participants