dperini / nwsapi

Fast CSS Selectors API Engine
MIT License
105 stars 36 forks source link

:scope selector direct children query is not working because of tag name resolution strategy #63

Closed sasensi closed 1 year ago

sasensi commented 1 year ago

This was originally reported as part of the jsdom repo here. I decided to try to fix it (because I had the exact same issue) and was able to track down the issue until here.

So here's the case: given the following DOM tree:

<div>
  <div class="ok"></div>
  <div class="ok">
    <div class="ko"></div>
  </div>
</div>

Getting a reference to the root <div> and doing the following query:

rootElement.querySelectorAll(':scope > div')

Doesn't only return the 2 expected "ok" divs but also the "ko" one.

Here's the full reproduction code allowing to easily switch between an ok and a ko case:

NW.Dom.install();

const SHOW_BUG = true;

const rootElementTagName = SHOW_BUG ? 'div' : 'main';

const html = `<${rootElementTagName}>
  <div class="ok"></div>
  <div class="ok">
    <div class="ko"></div>
  </div>
</${rootElementTagName}>`;

const dom = new window.DOMParser().parseFromString(html,'text/html');
const rootElement = dom.querySelector(rootElementTagName);
const queryResult = rootElement.querySelectorAll(':scope > div');

for (const element of queryResult) {
  console.log(element.className);
}

// This should always output:
// - ok
// - ok
//
// But the actual output is:
// - ok
// - ok
// - ko

And here's a fiddle hosting it.


I don't know this library codebase well, but I investigated a bit and I have the feeling that this is caused by the fact that this pattern : :scope > div is turned into a more generic one : div > div which is solved using a parent tag name resolution strategy and ultimately returns all the element globally matching this pattern: div > div, somewhat ignoring the element context. I hope that this can help saving time to fix it.

dperini commented 1 year ago

@sasensi I am not 100% sure the following is the correct way of definitely solving the ":scope" pseudo-class. However in the "test/scope" directory there are a few test passing and I would add your test to the existing list. So please try to substitute the 7 lines long "makeref" method found on lines 1330-1336 with the following modified code:

  makeref =
    function(selectors, element) {
      return selectors.replace(/:scope/ig,
        element.localName + 
        (element.id ? '#' + element.id : ':not([id])') +
        (element.className ? '.' + element.classList[0] : ':not([class])'));
    },

Thank you in advance for your contribution and for reporting the bug. Let me know if this change completely solves your current problem.

sasensi commented 1 year ago

Hey @dperini, thank you for you solution proposal, I'll try it asap and get back to you.

sasensi commented 1 year ago

@dperini, I looked at your solution and I also looked more closely at the codebase. I now understand that the handling of :scope is quite basic for now, and rely on the fact that the root element must be identifiable via a global selector (e.g. an id attribute). I think that a more robust and flexible solution would be to implement a way to filter out all elements that are not direct children of the root element by using the element.parentElement property.

As an example here's a simple way of solving this specific case outside of the library:

const rootElement = dom.querySelector(rootElementTagName);
const queryResult = rootElement.querySelectorAll(':scope > div').filter((it) => it.parentElement === rootElement);

Do you think that this kind of strategy could be included into the library ? (I saw that you already use internal callbacks to do checks in the compiled code, so maybe the same mechanism could work for this case ?)

dperini commented 1 year ago

@sasensi there were no advances that I know on browsers consensum about the functionality of the :scope pseudo class. I cannot consider this a bug nor an issue. Feel free to reopen in case you have reproducible results on all browsers.