Update tests for relaxed parser #178 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 Open josepharhar wants to merge 9 commits into html5lib:master base: master Choose a base branch

[josepharhar]

This PR updates the tree-construction dat files for the HTML change which will allow additional tags within

[josepharhar]

I'm not sure what the best practice is for rebaselining errors, but for now I removed all errors from affected tests. There are probably errors in tests I didn't change which may need to be rebaselined as well.

[annevk]

Will there be a separate PR for new tests?

[flavorjones]

FWIW when CI workflows are enabled, Nokogiri (downstream) tests will fail. I've started working on a branch with the proposed changes from whatwg/html#10557

[josepharhar]

Will there be a separate PR for new tests?

I added new test cases to webkit02 including:

• Reconstructing formatting contexts inside and outside of select: Relax parser whatwg/html#10557 (comment)
  Test cases I added a while ago currently in select-parsing.tentative.html: https://github.com/web-platform-tests/wpt/blob/7275b2c6bf4b9312a1945a451a5333e8922ee524/html/semantics/forms/the-select-element/customizable-select/select-parsing.tentative.html

[flavorjones]

Nokogiri work-in-progress at sparklemotion/nokogiri#3317

[flavorjones]

@josepharhar I've got a question about two tests that are very similar. Zooming in on this one from tree-construction/tests1.dat:

#data

#errors
#document
| 
|   
|   
|     
|       
|         
|     
|     
|       
|         

Nokogiri is constructing a different tree:

and I wanted to ask for a double-check that the test's assertion is correct, before I dive into Nokogiri's parser. Thank you!

[josepharhar]

Thanks for asking!

It looks like Nokogiri is nesting one inside the other, and my spec/chromium change intentionally fully closes the outer select before inserting the new one, so yeah I think the test's assertion is correct.

Here's the relevant part of the spec PR: https://whatpr.org/html/10557/parsing.html#:~:text=A%20start%20tag%20whose%20tag%20name%20is%20%22select%22

As for the in between the

and the , I think there is a mismatch between my spec PR and the chromium implementation - specificially the chromium implementations stops in this case before reconstructing formatting contexts, which isn't in the spec: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/parser/html_tree_builder.cc;l=951;drc=9776ce6636ab1d68d2b1c4f1f719eb71becf39a3

I'll take a closer look at which we should do and get back to you on that. Thanks!

[flavorjones]

Thanks for replying so quickly.

It looks like Nokogiri is nesting one select inside the other

No, sorry, unless I'm misunderstanding your comment, this is not a correct description of what Nokogiri's parser is doing. Here's a more graphical representation of the tree from my previous comment:

body
├── select
│   └── b
│       └── option
└── b
    └── select
        └── option

i.e. .

The select tags are not nested. Just wanted to clarify. (And if it's helpful context, Nokogiri is maintaining a fork of libgumbo.)

[josepharhar]

Ah whoops, I failed to read the tree properly 😅

I looked into why chromium is putting the b inside the select instead of the other way around, and i found that the tag in the test is what's making the difference - without it I am getting the same output as Nokogiri.

This is happening due to this call to the adoption agency algorithm when is parsed: https://html.spec.whatwg.org/multipage/parsing.html#parsing-main-inbody:adoption-agency-algorithm-4

I verified that chromium does the same thing as Nokogiri when I comment out that call to the adoption agency algorithm.

There's a lot of steps in that algorithm and I'm having a hard time wrapping my head around it, but does Nokogiri have that call to the adoption agency algorithm too? Or perhaps the implementations of the algorithm are different?

[flavorjones]

@josepharhar Thanks again for your kind reply!

Yes, Nokogiri's libgumbo has implemented the adoption agency algorithm, and I have confirmed that in these tests we are invoking it. It's helpful to know this is likely the source of the behavior difference, so I'll focus my efforts on making sure it matches the current spec (though I'll note it passes every other test in this suite ... 🤷).

[flavorjones]

OK, I think I know what's going on here. I think Chromium has missed this change:

https://github.com/whatwg/html/pull/10557/files#diff-41cf6794ba4200b839c53531555f0f3998df4cbb01a4d5cb0b94e3ca5e23947dR124618

If select is not in the list for default scope tags, then this adoption agency step:

If formattingElement is in the stack of open elements, but the element is not in scope, then this is a parse error; return.

will not trigger a parse-error-and-return, and the algorithm will continue. But select is in the list now, and so the formatting element b is not in scope.

If I remove the select tag from the default scope tags in Nokogiri's libgumbo, then behavior matches the test (and presumably chromium).

Can you check to see if my hunch is right?

[josepharhar]

Thanks so much for figuring this out! Yeah I totally missed that in the chromium implementation but I just added it and updated the tests here.

[flavorjones]

@josepharhar That's great! Thank you!

I've got a patch to get the error messages to a point where libgumbo is passing, would you mind taking a look and potentially applying to this PR? (Renamed to .txt to get by the github attachment filter.)

select-test-errors.patch.txt

[josepharhar]

Looks good, thanks! I applied it.

Sometimes I wonder why chromium throws away parse errors.

[untitaker]

the changes to #errors make webkit02 highly inconsistent and break existing norms in e.g. parse5. See also whatwg/html#1339

[josepharhar]

I just updated this PR, which mostly involved reverting stuff. Should all be up to date and in sync with the whatwg/html PR now.

[flavorjones]

@josepharhar Twelve of the tests changed or added in the recent commits do not have the correct number of error messages in the test data.

I'm not sure if you wish to fix those up given the @untitaker feedback above, but if so here are the failures from the Nokogiri test suite. Please let me know if I can help format these for you.

nokogiri libgumbo error match failures

Finished in 1.434869s, 4618.5409 runs/s, 79923.6865 assertions/s.

  1) Failure:
TestHtml5TreeConstructionTests2#test_37__script_on [test/html5/test_tree_construction.rb:131]:
Expected 2 errors for , found:
1:61: ERROR: Start tag 'select' isn't allowed here. Currently open tags: html, body, select, option.

                                                            ^.
Expected: 2
  Actual: 1

  2) Failure:
TestHtml5TreeConstructionTests2#test_37__script_off [test/html5/test_tree_construction.rb:131]:
Expected 2 errors for , found:
1:61: ERROR: Start tag 'select' isn't allowed here. Currently open tags: html, body, select, option.

                                                            ^.
Expected: 2
  Actual: 1

  3) Failure:
TestHtml5TreeConstructionWebkit01#test_31__script_on [test/html5/test_tree_construction.rb:131]:
Expected 9 errors for ABCDEFG, found:
1:1: ERROR: Expected a doctype token
ABCDEFG
^
1:18: ERROR: Start tag 'select' isn't allowed here. Currently open tags: html, body, select, option.
ABCDEFG
                 ^
1:52: ERROR: Start tag 'select' isn't allowed here. Currently open tags: html, body, option, select, option.
ABCDEFG
                                                   ^
1:86: ERROR: Start tag 'select' isn't allowed here. Currently open tags: html, body, option, select, option.
ABCDEFG
                                                                                     ^
1:120: ERROR: Start tag 'select' isn't allowed here. Currently open tags: html, body, option, select, option.
ABCDEFG
                                                                                                                       ^.
Expected: 9
  Actual: 5

  4) Failure:
TestHtml5TreeConstructionWebkit01#test_31__script_off [test/html5/test_tree_construction.rb:131]:
Expected 9 errors for ABCDEFG, found:
1:1: ERROR: Expected a doctype token
ABCDEFG
^
1:18: ERROR: Start tag 'select' isn't allowed here. Currently open tags: html, body, select, option.
ABCDEFG
                 ^
1:52: ERROR: Start tag 'select' isn't allowed here. Currently open tags: html, body, option, select, option.
ABCDEFG
                                                   ^
1:86: ERROR: Start tag 'select' isn't allowed here. Currently open tags: html, body, option, select, option.
ABCDEFG
                                                                                     ^
1:120: ERROR: Start tag 'select' isn't allowed here. Currently open tags: html, body, option, select, option.
ABCDEFG
                                                                                                                       ^.
Expected: 9
  Actual: 5

  5) Failure:
TestHtml5TreeConstructionWebkit02#test_40__script_on [test/html5/test_tree_construction.rb:131]:
Expected 4 errors for , found:
1:1: ERROR: Expected a doctype token

^
1:17: ERROR: Start tag 'select' isn't allowed here. Currently open tags: html, body, select, button.

                ^
1:25: ERROR: End tag 'select' isn't allowed here. Currently open tags: html, body.

                        ^
1:34: ERROR: End tag 'button' isn't allowed here. Currently open tags: html, body.

                                 ^
1:43: ERROR: End tag 'select' isn't allowed here. Currently open tags: html, body.

                                          ^.
Expected: 4
  Actual: 5

  6) Failure:
TestHtml5TreeConstructionWebkit02#test_38__script_off [test/html5/test_tree_construction.rb:131]:
Expected 3 errors for button, found:
1:1: ERROR: Expected a doctype token
button
^
1:23: ERROR: End tag 'select' isn't allowed here. Currently open tags: html, body, select, button.
button
                      ^.
Expected: 3
  Actual: 2

  7) Failure:
TestHtml5TreeConstructionWebkit02#test_41__script_off [test/html5/test_tree_construction.rb:131]:
Expected 2 errors for 
, found:
1:1: ERROR: Expected a doctype token

^
1:22: ERROR: Start tag 'select' isn't allowed here. Currently open tags: html, body, select, button, div.

                     ^
1:30: ERROR: End tag 'select' isn't allowed here. Currently open tags: html, body.

                             ^.
Expected: 2
  Actual: 3

  8) Failure:
TestHtml5TreeConstructionWebkit02#test_41__script_on [test/html5/test_tree_construction.rb:131]:
Expected 2 errors for 
, found:
1:1: ERROR: Expected a doctype token

^
1:22: ERROR: Start tag 'select' isn't allowed here. Currently open tags: html, body, select, button, div.

                     ^
1:30: ERROR: End tag 'select' isn't allowed here. Currently open tags: html, body.

                             ^.
Expected: 2
  Actual: 3

  9) Failure:
TestHtml5TreeConstructionWebkit02#test_38__script_on [test/html5/test_tree_construction.rb:131]:
Expected 3 errors for button, found:
1:1: ERROR: Expected a doctype token
button
^
1:23: ERROR: End tag 'select' isn't allowed here. Currently open tags: html, body, select, button.
button
                      ^.
Expected: 3
  Actual: 2

 10) Failure:
TestHtml5TreeConstructionWebkit02#test_40__script_off [test/html5/test_tree_construction.rb:131]:
Expected 4 errors for , found:
1:1: ERROR: Expected a doctype token

^
1:17: ERROR: Start tag 'select' isn't allowed here. Currently open tags: html, body, select, button.

                ^
1:25: ERROR: End tag 'select' isn't allowed here. Currently open tags: html, body.

                        ^
1:34: ERROR: End tag 'button' isn't allowed here. Currently open tags: html, body.

                                 ^
1:43: ERROR: End tag 'select' isn't allowed here. Currently open tags: html, body.

                                          ^.
Expected: 4
  Actual: 5

 11) Failure:
TestHtml5TreeConstructionTests7#test_17__script_off [test/html5/test_tree_construction.rb:131]:
Expected 2 errors for X, found:
1:24: ERROR: Start tag 'select' isn't allowed here. Currently open tags: html, body, select.
X
                       ^.
Expected: 2
  Actual: 1

 12) Failure:
TestHtml5TreeConstructionTests7#test_17__script_on [test/html5/test_tree_construction.rb:131]:
Expected 2 errors for X, found:
1:24: ERROR: Start tag 'select' isn't allowed here. Currently open tags: html, body, select.
X
                       ^.
Expected: 2
  Actual: 1

[josepharhar]

Please let me know if I can help format these for you.

That would be great! I could try but I might mess it up 😅

[flavorjones]

@josepharhar I've attached a patch file that indicates the number and nature of the error messages generated by libgumbo.

I want to acknowledge @untitaker's comment above that these error messages do not follow the format of other existing errors or the format suggested by whatwg/html#1339, but this is the best I can do in the time I've allotted.

error_messages.patch.txt

[josepharhar]

Thanks for generating those errors! I applied them in the latest commit