:active and :hover always returning true since 2.2.10

[Maxim-Mazurok]

Regression of #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]

2.2.12 does not work for me either

[dperini]

@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]

Thanks for looking into this, I was using minimal reproduction from the following issue: jsdom/jsdom#3607 (comment)

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

Hope this helps, cheers!

[alopix]

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

[Waley-Z]

@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]

@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]

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]

@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: jsdom/jsdom#3610 (comment)

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]

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/[email protected]/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:

Hope this is more concrete now, cheers!

[dperini]

@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]

@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/[email protected]/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]

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]

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>

[JakubJagoda]

I've encountered this bug today while debugging a test case that in my opinion should have been passing but it was not, and I can confirm it's a regression introduced by this commit. In 2.2.9 everything works fine, in 2.2.10 it does not.
The repo reproducing the bug: https://github.com/JakubJagoda/nwsapi-hover-bug-reproduce

tl;dr it's a test case in jest using jsdom which under the hood uses nwsapi to calculate computed styles. Test case installs a spreadsheet which defines different styles for element when it's :hovered and simply tests the current computed style of that element. The test fails on "master" branch and passes on branch "fix", which differ only in version of nwsapi.

Now I'm no expert in nwsapi code and don't know how to fix it (otherwise than just reverting the commit I mentioned), but since I already spent much time trying to find out why my test was failing when I was certain it should pass, it would be a shame if I did not share my findings on the bug's root cause.

When compiling selector, nwsapi partially matches a part of the selector, handles the matched part (by producing some code that will get executed later), "pops" the part from the selector and repeats the process until the selector is empty. When it stumbles upon a selector .hoverable:hover, it first handles the .hoverable part by setting up the compiled code with something like this

// it's basically gonna grab "class" attribute of e and check if it matches "hoverable"
if((/(^|\s)hoverable(\s|$)/.test(e.getAttribute("class")))){r=true;}

It pops the matched part and begin inspecting selector ":hover" and this leads us to following code:

case 'hover':
  source = 'hasFocus' in doc && doc.hasFocus() ?
    'if((e===s.doc.hoverElement)){' + source + '}' : source;

hasFocus indeed exists in doc (a Document instance provided by jsdom), but its implementation returns false. The implementation for reference

 hasFocus() {
    return Boolean(this._lastFocusedElement);
  }

this._lastFocusedElement is null, so doc.hasFocus() evaluates to false, therefore - and this is where the bug happens - source is assigned the value it currently holds, the one that was computed in the previous step, meaning that it will just check if element's "class" attribute equals to "hoverable", despite that original selector also required the element to be hovered.

Now let's also explain why it works in 2.2.9. The difference lays in the code produced when matching :hover:

case 'hover':
  source = 'hasFocus' in doc && doc.hasFocus() ?
    'if((e===s.doc.hoverElement)){' + source + '}' :
    'if(false){' + source + '}';

In this case, the value assigned to source is now wrapped with if(false), meaning that the previous check (for the "class" attribute) will not run, producing the correct result.

So to recap:

// in 2.2.10 ".hoverable:hover" compiles to following code (whitespaces added by me):
"use strict";
return function Resolver(c, f, x, r) {
  var e, n, o;
  e = c;
  if (/(^|\s)hoverable(\s|$)/.test(e.getAttribute("class"))) {
    r = true;
  }
  return r;
};
//...which returns true because it only checks for ".hoverable" part,
// while in 2.2.9 the same selector compiles to following code:
"use strict";
return function Resolver(c, f, x, r) {
  var e, n, o;
  e = c;
  if (false) {
    if (/(^|\s)hoverable(\s|$)/.test(e.getAttribute("class"))) {
      r = true;
    }
  }
  return r;
};
// which does not return true because the check for ".hoverable" is not executed

To me this points that how nwsapi handles :hover (and supposedly :active, but I haven't debugged for that) pseudo-classes is the failing point. But then again, I'm not that familiar with nwsapi's source code, therefore I'm not sure why the fix that was already there was reverted and what prevents it from being un-reverted.

Cheers!

[dperini]

@JakubJagoda
both your analysis of the problem and your code are correct.
I am checking to see if the resolver can be reduced to only one loop.
Great ! It will be merged in the next release.
Thank you so much for the help/contribution.

[zuzusik]

Not sure if this is related, but .matches(":autofill") also erroneously returns true all the time.

I have modified one of the examples above to reproduce it:

<!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/[email protected]/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 isAutofill = NW.Dom.match(":autofill", element);
    document.write("isAutofill: " + isAutofill);
</script>
</body>
</html>

Autofill pseudo class: https://developer.mozilla.org/en-US/docs/Web/CSS/:autofill

UPD: created separate ticket to track this: #129

[dperini]

@JakubJagoda @asamuzaK thank you both for the extensive testing and accompanying explanations. Your help, @alopix and @Maxim-Mazurok contributions and @zuzusik review were much appreciated by me and they will be appreciated by nwsapi users as soon as I get the time to transform them in useful code.
Thank you guys !

[dperini]

Hmmm... something went wrong during the process, I have been working too much without sleeping.

@JakubJagoda @Maxim-Mazurok @asamuzaK @alopix
the commit 7aa1f4b is trying to solve the issue by using related mouse events.
It remains to be seen if the events listeners should be always set automatically as is now or by means of a Config flag, on demand or by other means.
Please test and report any remaining issues.
Thanks to all for the help nailing down a solution.

[dperini]

Ok I fixed typos and mangled line in 38e566f
Seems to be working now please test !

[asamuzaK]

38e566f#diff-8081ec5523d50813a65b2ec303d10ae06a5ba508866607db3700779f9bb62f07R1146

It should be e.contains(s.HOVER) instead of e===s.HOVER.
The event target may be the node itself as well as its descendants.
The same goes for e===s.ACTIVE below.

[dperini]

@asamuzaK
thank you for the details.
If you please can rework the other pull request I will apply that too.
Sorry for the extra work. I appreciate your contributions.