dperini / nwsapi

Fast CSS Selectors API Engine
MIT License
103 stars 35 forks source link

:active and :hover always returning true since 2.2.10 #119

Open Maxim-Mazurok opened 1 month ago

Maxim-Mazurok commented 1 month ago

Regression of https://github.com/dperini/nwsapi/issues/99

This issue has regressed in v2.2.10. It was fixed in v2.2.8 and v2.2.9, but has reappeared in v2.2.10.

I can confirm, also see regression in the latest v2.2.12

Waley-Z commented 1 month ago

2.2.12 does not work for me either

dperini commented 1 month ago

@Maxim-Mazurok @Waley-Z I did several test with the ":active" pseudo-class but I can't replicate your results. From what you write I guess you use the "element.matches" api but I need to see how and where you use it to be able to give you a correct answer. Also I used the web-platform-etsts to check these pseudo-class and it looks like they pass. Please post a minimal code showing the bug and the wrong results you get. Thank you !

Maxim-Mazurok commented 1 month ago

Thanks for looking into this, I was using minimal reproduction from the following issue: https://github.com/jsdom/jsdom/issues/3607#issue-1936055742

Unfortunately I don't know how to use nwsapi without jsdom, so I hope that linked repro is fine.

Hope this helps, cheers!

alopix commented 1 month ago

Agreed, can definitely reproduce this with the example from the other ticket: failing-example-code.zip

Waley-Z commented 1 month ago

@Maxim-Mazurok @Waley-Z I did several test with the ":active" pseudo-class but I can't replicate your results. From what you write I guess you use the "element.matches" api but I need to see how and where you use it to be able to give you a correct answer. Also I used the web-platform-etsts to check these pseudo-class and it looks like they pass. Please post a minimal code showing the bug and the wrong results you get. Thank you !

Thanks for the reply. I used jsdom as well.

dperini commented 1 month ago

@Maxim-Mazurok , @Waley-Z, @alopix let's do a recap here because I am missing something in your messages and code. In the code above there are no usage instances of neither the :active nor the :hover pseudo-classes.

https://www.w3.org/TR/selectors-4/#the-active-pseudo the :active pseudo-class is meant to report the state of a focusable element, one that can receive keyboard focus. input controls and their labels are an example. Not all types of element are focusable

https://www.w3.org/TR/selectors-4/#the-hover-pseudo the :hover pseudo-class designates the element that is under the cursor or its parent, input controls and their labels are an example.

As simple examples, please run these lines in the browser console: document.body.appendChild(document.createElement('iframe')).focus(); document.activeElement; // iframe focusable document.body.appendChild(document.createElement('input')).focus(); document.activeElement; // input focusable document.body.appendChild(document.createElement('div')).focus(); document.activeElement; // div not focusbale

The result of the last line above will be different since div elements are not focusable, so the console textarea will be the result of querying the active element, since it was the last active element before running the test and it willnot change.

My knowledge about what you are trying to achieve in jsdom is scarce. However with the help of someone who knows the inner working of the code you sent and a minimal explanation I might be able to debug and see if :active and :hover pseudo-classes are the failing points in nwsapi or something else is happening.

alopix commented 1 month ago

I basically just copied the code from the other PR but remember having had this issue in live tests in previous versions.

Basically the result of the example code is, that nwsapi reports the button’s background colour to be the value set via the though the button would not be hovered, so it should return the button’s normal non-hover background colour.

Maxim-Mazurok commented 1 month ago

@dperini the JSDOM reproduction code that relies on nwsapi is this:

const { JSDOM } = require("jsdom");

const dom = new JSDOM('<div>hello</div>');

const element = dom.window.document.querySelector('div');

console.log('active', element.matches(':active'));

I copied it from here: https://github.com/jsdom/jsdom/issues/3610#issue-1950403910

Same as in the issue linked above, it logs active true, but should log active false, because the element is not in the :active state by default.

I think that element.matches is defined here: https://github.com/jsdom/jsdom/blob/ee8b6156bee3c7d0d201ccdd2135ee635fb8afcd/lib/jsdom/living/nodes/Element-impl.js#L589-L593

Hope that this helps to reproduce and narrow down the issue, cheers!

Maxim-Mazurok commented 1 month ago

Actually I recognise it's not very fair to ask you look into other project code to figure this out. So here's just NWSAPI reproduction:

<!DOCTYPE html>
<html lang="en">
  <head>
    <meta charset="UTF-8" />
    <meta name="viewport" content="width=device-width, initial-scale=1.0" />
    <title>Document</title>
    <script src="https://cdn.jsdelivr.net/npm/nwsapi@2.2.12/src/nwsapi.min.js"></script>
  </head>
  <body>
    <div id="test">test</div>
    <script>
      const element = NW.Dom.byId("test", document);
      const isActive = NW.Dom.match(":active", element);
      document.write("isActive: " + isActive);
    </script>
  </body>
</html>

Important note: it will only reproduce the issue (report div as :active) if you focus on devtools window and reload the window using F5 or Ctrl+R, instead of using the UI button. Here's a demo: Code_EhnutbL3s7

Hope this is more concrete now, cheers!

dperini commented 1 month ago

@Maxim-Mazurok thank you for your effort and it is not a problem for me if you ask to look deeper even if this could be software from other sources. Don't worry, really it is not a problem for me to take it again and resolve this.

If every issue had code showing the problem like you did it would be easier for me to invest some time trying to fix it but things are not like that most of the times and it will take too much resources starting like a blind on other's code.

Will be back to you soon, hopefully with a solution but most probably with more questions.

dperini commented 1 month ago

@Maxim-Mazurok well I have news about this issue. I guess that what you are trying to do is detect if the console is active and the user is interacting with it.

First, I see that you are incorrectly using "nwsapi.js" as a standalone library in a browser, you have to invoke the "NW.Dom.install" method to initialize the environment and have "nwsapi.js" replacing the browser original Selector API. This has to be done as follows:

<script src="nwsapi.js" "onload=NW.Dom.install"></script>

Second, the "NW.Dom.byId" api call returns an array of matching elements and that is following the specifications.

Given the above I fixed the sample code you submitted as follow:

<!DOCTYPE html>
<html lang="en">
  <head>
    <meta charset="UTF-8" />
    <meta name="viewport" content="width=device-width, initial-scale=1.0" />
    <title>Document</title>
      <script src="https://cdn.jsdelivr.net/npm/nwsapi@2.2.12/src/nwsapi.min.js" onload="NW.Dom.install"></script>
  </head>
  <body>
    <div id="test">test</div>
    <script>
      const element = NW.Dom.byId("test", document)[0];
      const isActive = NW.Dom.match(":active", element);
      document.write("isActive: " + isActive);
    </script>
  </body>
</html>

The two changes I did to your sample code are the addition of "[0]" at the end of the "NW.Dom.byId" api call, which limit the return value to only one element and I added the correct initialization of the "nwsapi.js" library by adding the "onload" attribute, thus invoking "NW.Dom.install" after the script "load" event is triggered.

Maxim-Mazurok commented 1 month ago

I guess that what you are trying to do is detect if the console is active and the user is interacting with it.

Not really, our original use-case is outside of the browser environment, in NodeJS where we're using JSDOM. The reason why I mentioned the focused console is because that's the way I was able to reproduce the issue in the browser. I couldn't reproduce it in NodeJS because that would involve using JSDOM for document implementation and wouldn't be as fair/clean reproduction of NWSAPI issue. So long story short, dev tools is just a way to reproduce issue, not the original use-case that uncovered the issue.

Given the above I fixed the sample code you submitted as follow

Makes sense! It was working the same with/without NW.Dom.install, and didn't know about array. Thanks for fixing it. The fixed version still reproduces issue tho, showing isActive: true on latest Chrome when dev tools are open and focused on reload. Let me know if you need any more input on this from me, cheers :)

asamuzaK commented 4 weeks ago

https://www.w3.org/TR/selectors-4/#the-active-pseudo the :active pseudo-class is meant to report the state of a focusable element, one that can receive keyboard focus. input controls and their labels are an example. Not all types of element are focusable

It doesn't matter whether the element is focusable or not.

However, the :active pseudo-class matches only during a primary mouse button (usually left button) is pressed, that is, during a mousedown event and a mouseup event. Similarly, the :hover pseudo-class only matches during mouseover and mouseout events.

In any case, it can only be determined inside the event listeners, e.g. MouseEvent.

Event order Event type State
1 mouseover :hover start matching
2 mousedown :active start matching
3 mouseup :active does not match anymore
4 click note that :active does not match on click event
5 mouseout :hover does not match anymore

Ref https://w3c.github.io/uievents/#events-mouseevent-event-order

Test case:

<!DOCTYPE html>
<html>
<head>
</head>
<body>
<div id="target" style="border: 1px solid">
  Click Me
</div>
<script>
  const sleep = () => new Promise((resolve, reject) => {
    setTimeout(resolve, 0);
  });
  const node = document.getElementById('target');
  node.addEventListener('mousedown', async evt => {
    const { target, type } = evt;
    await sleep();
    console.log(`target matches :active on ${type}: ${target.matches(':active')}`); // true
  });
  node.addEventListener('mouseup', async evt => {
    const { target, type } = evt;
    await sleep();
    console.log(`target matches :active on ${type}: ${target.matches(':active')}`); // false
  });
  node.addEventListener('click', async evt => {
    const { target, type } = evt;
    await sleep();
    console.log(`target matches :active on ${type}: ${target.matches(':active')}`); // false
  });
  node.addEventListener('mouseover', async evt => {
    const { target, type } = evt;
    await sleep();
    console.log(`target matches :hover on ${type}: ${target.matches(':hover')}`); // true
  });
  node.addEventListener('mouseout', async evt => {
    const { target, type } = evt;
    await sleep();
    console.log(`target matches :hover on ${type}: ${target.matches(':hover')}`); // false
  });
</script>
</body>
</html>