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

Pass remaining Web Platform Tests for selectors #10

Closed
4 of 5 tasks
Zirro opened this issue Apr 17, 2018 · 8 comments
Closed
4 of 5 tasks

Pass remaining Web Platform Tests for selectors #10

Zirro opened this issue Apr 17, 2018 · 8 comments

Comments

@Zirro
Copy link
Contributor

Zirro commented Apr 17, 2018

nwsapi is nearly passing all Web Platform Tests that browsers do, which is very exciting. Since these tests are used by browsers to ensure that they are compatible with each other, it would be excellent if it could pass the final ones as well. These are the last tests from the dom/nodes/ directory which pass in browsers, but fail in nwsapi:

dom/nodes/ParentNode-querySelector-All.html:

  • Document.querySelectorAll: ID selector, matching multiple elements with duplicate id: #id-li-duplicate
  • Fragment.querySelectorAll: Namespace selector, matching element with any namespace: #any-namespace *|div
  • Fragment.querySelectorAll: Namespace selector, matching div elements in no namespace only: #no-namespace |div
  • Fragment.querySelector: Namespace selector, matching div elements in no namespace only: #no-namespace |div

dom/nodes/Element-closest.html:

  • Element.closest with context node 'test4' and selector 'div > :scope'

Let me know if you have any questions about the tests.

@Zirro
Copy link
Contributor Author

Zirro commented Apr 17, 2018

The commit you pushed while I was creating this issue solved the failures related to namespaces 🎉

@dperini
Copy link
Owner

dperini commented Apr 17, 2018

@Zirro correct, there was a specific commit for the namespace issue... I hope that commit finally solves both the remaining SVG and more generally the XML issues with mixed-content.

I am also ready to commit the changes needed for the "duplicate IDs", just need a bit more time to do some code lifting and cleanup.

The last commit I did is also a performance improvement for comma separated groups of selectors, they where fast already but now they should be even faster and the code is smaller.

@Zirro
Copy link
Contributor Author

Zirro commented Apr 17, 2018

I've just tested beta2 with jsdom, and unfortunately the SVG tests are still an issue. I don't know if it helps, but some quick debugging shows that it ends up running this code at one point where e.nodeName is compared against uppercase "SVG":

(function(s
/*``*/) {
"use strict";return function Resolver(c,f,x){var r=[],e,n,o,j=-1,k=-1;c=c(true);main:while(e=c[++k]){if((e.nodeName=="SVG")){r[++j]=c[k];if(f(c[k])){break main;}continue main;}}return r; }
})

I'll have more time for debugging in a few days.

@dperini
Copy link
Owner

dperini commented Apr 18, 2018

@Zirro thank you for noticing the issue, clearly every strangeness you notice and report might help.
I believe with the latest commits I did today the remaining issues should have been resolved.

I was forced to accept a minor performance loss to try a new path aimed at resolving the mixed-namespace problem. I hope this solves the SVG and similar issues with tag names upper/lower case.

@Zirro
Copy link
Contributor Author

Zirro commented Apr 19, 2018

Yes - the latest commits have finally solved the SVG problem, as well as the one with duplicate IDs! 🎉

Unfortunately, there are also some new failures. 924d5b4 appears to be the cause for most of them, as several more tests pass if I revert it or set FASTCOMMA to false. Can you explain how this function works and when it is needed?

With FASTCOMMA set to false, there's still a failure where context becomes null on line 314, since context is set to firstElementChild even for a body (<body>Body text content</body>) that only has a text node rather than an element.

I'll need some time to figure out if the other issues are in jsdom or nwsapi, but thanks for resolving everything related to mixed namespaces.

@dperini
Copy link
Owner

dperini commented Apr 20, 2018

@Zirro thank you for the feedback, really appreciated (and needed).

I normally test everything with standard browsers first, then when I publish a new release to npm I test with jsdom by simply replacing current "nwmatcher-noqsa.js" with the content of the new "nwsapi". I understand this is a bit of a skew procedure but as soon as I have more spare time I am going to dig myself a bit more into yarn documentation and learn more about it. I believe that is the current tool used for testing.

Can you share more info about how you do testing of WPT selectors, I have tried yarn test-wpt but that is not what I need, I need something that goes into more detailed test of only "ParentNode-querySelector-All-xht.xht" (1859 test) or the complete "dom/nodes/" path where this file resides.

I can do it easily from a browser but I will prefer doing the same from terminal running "node <file.js>" that will allow me to test and benchmark without having to run the complete test suite each time.

For the "" problem you noticed I don't know what to say now, but I will check that out if you send me the test or the link were I can see that problem. As a first guess the problem might be in a different place because the "firstElementChild" property should not return a text node in any circumstance that I am aware of. The line you pointed out (L314) is needed by the engine to handle elements in a DocumentFragment (nodeType 11) to allow using native gEBTN method to collect nodes from a fragment since DocumentFragment nodes do not have those methods available in their prototype.

I am writing down details of the process bound to the "FASTCOMMA" flag as you asked in another message to keep things apart.

@dperini
Copy link
Owner

dperini commented Apr 20, 2018

@Zirro about the comma group selectors and the "FASTCOMMA" configuration flag ...

The basic is that the "collect()" method (L1442-1480), in the main "else" block, is caching the needed HTMLCollection lists (which are live lists) needed in resolution of comma groups selectors.

The objective is to resolve comma group selectors in a faster way but at the same time ensure that those initial HTMLCollection lists have not been changed since their last caching time to ensure error free select() operation. The checking process is a method named "validate()" and is invoked every time a query of that type is served/resolved.

So, as an example, for the "div.notes, span.detail" selector I am caching the "div" HTMLCollection and the "span" HTMLCollection" and then build the usual resolver function that only checks these elements for their corresponding class names without checking the tag names again.

Since I do this in "document order" there is no need to go through an extra reordering pass nor to check the resulting array for uniqueness, which further improves the process performances.

A second example, to better explain a different case where this is important. For a selector like "h1, h2, h3, h4, h5, h6", used to automate TOC building for HTML documents, it will be even faster because the six HTMLCollection lists alone will makeup the result with only one pass to have them returned in "document order". Again no need to perform extra reorder/unique passes.

Not sure the explanation was clear enough, you will have to bear with my English skills.
For any other question or doubt just ask, I would like to have "nwsapi" ready and integrated asap.

@Zirro
Copy link
Contributor Author

Zirro commented Apr 20, 2018

Can you share more info about how you do testing of WPT selectors, I have tried yarn test-wpt but that is not what I need, I need something that goes into more detailed test of only "ParentNode-querySelector-All-xht.xht" (1859 test) or the complete "dom/nodes/" path where this file resides.

Absolutely - you can use the --fgrep flag to run a smaller number of tests. For example:

yarn test-wpt --fgrep "ParentNode-querySelector-All.html" to run that particular test. (don't worry about the .xht version - I think that fails in jsdom for unrelated reasons)

yarn test-wpt --fgrep "dom/nodes" to run all tests in the dom/nodes directory.

You will also need to change the list of failing tests - to-run.yaml - so that this line is behind a #-comment. Otherwise the test is disabled when you run yarn test-wpt.

As a first guess the problem might be in a different place because the "firstElementChild" property should not return a text node in any circumstance that I am aware of.

Yeah, the problem is not that it returns a text node, but it returns null when there are no child elements. For example, if you try to use document.body.firstElementChild on the following document:

<body>No elements here - only text</body>

Thanks for explaining FASTCOMMA and the validate()-method! That's an interesting approach with a HTMLCollection-based cache for each element you find in a selector. It makes sense since the classes are more likely to change than the structure of the document. Your explanation was perfectly clear too - no need to worry about your English in this case :-)

It's still strange that it would cause these failures though, but perhaps it has some unintended side-effect in jsdom. I will debug it over the weekend.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants