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.

Sign up for GitHub

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

Jump to bottom

Implement the latest select parsing changes #3317

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

Implement the latest select parsing changes #3317

wants to merge 4 commits into from

Conversation

flavorjones
Copy link
Member

@flavorjones flavorjones commented Oct 16, 2024 •
edited
Loading

What problem is this PR intended to solve?

In whatwg/html#10557 there are some changes being made to how <select> tags are parsed.

Have you included adequate test coverage?

Tests are under development on a branch at html5lib/html5lib-tests#178

Does this change affect the behavior of either the C or the Java implementations?

HTML5 is only supported by the C impl.

@flavorjones

This comment was marked as outdated.

Sign in to view
@flavorjones flavorjones mentioned this pull request Oct 16, 2024
Open
@flavorjones flavorjones force-pushed the flavorjones-html5-relaxed-select branch from 3cefda2 to eb62613 Compare October 16, 2024 19:45
@flavorjones
Copy link
Member Author

Ugh, sorry about that -- PEBKAC -- I just needed to force-recompile and that particular test is passing. There are other ones that I'll look into now.

@stevecheckoway
Copy link
Contributor

I see that whatwg/html PR has had a bunch of changes since I last looked at it. I can take a look at this PR in more detail next week.

@flavorjones
Copy link
Member Author

flavorjones commented Oct 16, 2024 •
edited
Loading

@stevecheckoway Thanks. Actually, my changes look pretty good! Only two tests are failing for non-error-message reasons, and they're both very similar. (This PR is using a fixed-up version of html5lib/html5lib-tests#178 with appropriate error messages.)

Zooming in on the simpler of the two (CI failure here):

#data
<select><b><option><select><option></b></select>
#errors
#document
| <html>
|   <head>
|   <body>
|     <select>
|       <b>
|         <option>
|     <b>
|     <select>
|       <b>
|         <option>

This pr results in the following tree:

<body>
  <select>
    <b>
      <option>
  <b>
    <select>
      <option>

so I wanted to double check if I've missed something or if these tests are incorrect.

@flavorjones
Copy link
Member Author

@stevecheckoway Before you dig in on this, please catch up on the chat I'm having upstream in html5lib/html5lib-tests#178, TLDR I think I'm right and the test (and chromium) are wrong.

@flavorjones
Copy link
Member Author

Updated the gumbo tests.

Note that these changes break rails-html-sanitizer, I'll need to work a bit upstream before these changes are safe to merge and release.

@flavorjones flavorjones changed the title Implement the latest select parsing changes Implement the latest select parsing changes Nov 17, 2024
@flavorjones flavorjones force-pushed the flavorjones-html5-relaxed-select branch 2 times, most recently from 1442b66 to 4e0111b Compare November 23, 2024 14:47
@flavorjones flavorjones force-pushed the flavorjones-html5-relaxed-select branch from 4e0111b to 6b09d9c Compare December 19, 2024 23:51
flavorjones and others added 2 commits December 20, 2024 08:20
@flavorjones
feat: implement the new select parsing rules
9d56278 
whatwg/html#10557

This is green (modulo errors) up to whatwg/html@f8d14341 plus the
correction I suggested to the spec in
whatwg/html#10557 (comment)
@flavorjones
test: commenting out html5 error testing
e5e51e2 
because most implementations don't seem to be paying attention to
them?
@flavorjones flavorjones force-pushed the flavorjones-html5-relaxed-select branch from 6b09d9c to e5e51e2 Compare December 20, 2024 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@flavorjones @stevecheckoway