Infinite loop fixed but still getting error in selectors #83

rocioimpa · 2023-04-13T14:17:22Z

Hi, the infinite loop issue has been fixed but still getting this test failure in tests that were passing prior to release 2.2.3:
SyntaxError: 'style):not(script):not(noscript):not(link):not([tabIndex="-1"]' is not a valid selector

at emit (node_modules/nwsapi/src/nwsapi/js:557:17)
at Object._matches [as match] (node_modules/nwsapi/src/nwsapi/js:1400:9)
at Array.Resolver (eval at compile (node_modules/nwsapi/src/nwsapi/js:760:17))
at collect (node_modules/nwsapi/src/nwsapi/js:1552:21)

alexgwolff · 2023-04-13T19:48:28Z

See: #82

workaround:

"overrides": {
    "nwsapi": "2.2.2"
  }

dperini · 2023-04-13T23:36:09Z

@rocioimpa
to be able to help you debugging the issue you reported I need you to write
a small code example that can reproduce the error you are getting.
However I retested the selector and found it being parsed correctly.

Furthermore I received confirmation from other devs telling the fix resolved the issue.
So please double check everything and report back the results you get.
Thank you!

cole-adams · 2023-04-14T19:19:58Z

Hello,
I'm also experiencing tests failing in >=2.2.3 that were passing in 2.2.2.

SyntaxError: 'slot):not([inert]' is not a valid selector
      at emit (node_modules/nwsapi/src/nwsapi.js:557:17)
      at Object._matches [as match] (node_modules/nwsapi/src/nwsapi.js:1400:9)
      at Array.Resolver (eval at compile (node_modules/nwsapi/src/nwsapi.js:760:17), <anonymous>:3:105)
      at collect (node_modules/nwsapi/src/nwsapi.js:1552:21)
      at Object._querySelectorAll [as select] (node_modules/nwsapi/src/nwsapi.js:1509:36)

To reproduce:

it("should not throw", () => {
  expect(() =>
    document.querySelectorAll("[tabindex]:not(slot):not([inert])")
  ).not.toThrow();
});

asamuzaK · 2023-04-15T07:42:06Z

Same here with v2.2.4.

Code:

const nodes = document.querySelectorAll('.foo:not(.bar):not([data-id="1"]');

Result:

SyntaxError: '.foo):not(.bar):not([data-id="1"]' is not a valid selector
      at emit (node_modules\nwsapi\src\nwsapi.js:557:17)
      at Object._matches [as match] (node_modules\nwsapi\src\nwsapi.js:1400:9)
      at Array.Resolver (eval at compile (node_modules\nwsapi\src\nwsapi.js:760:17), <anonymous>:3:105)
      at collect (node_modules\nwsapi\src\nwsapi.js:1552:21)
      at Object._querySelectorAll [as select] (node_modules\nwsapi\src\nwsapi.js:1509:36)

dperini · 2023-04-15T08:17:01Z

@asamuzaK & @cole-adams
both the following DOM queries are passing here on my machines testing what you reported in the console:

document.querySelectorAll("[tabindex]:not(slot):not([inert])")

document.querySelectorAll('.foo:not(.bar):not([data-id="1"]');

are parsed correctly in version 2.2.4 of "nwsapi", not throwing errors in the console.
Please ensure you are loading "nwsapi" from the npm repository and not from a secondary fork.
If you load "nwsapi.js" in the browser you can verify current version by the following command in console:

NW.Dom.Version

Also, other developers have confirmed that these bugs were fixed, both the infinite recursion and the multiple ":not()".
Try using "nwsapi" independently of other packages in browser console first.

asamuzaK · 2023-04-15T08:37:31Z

Failed log: Update libs asamuzaK/sidebarTabs@99768e5

I'm not using nwsapi directly, I'm using jsdom which depends on nwsapi.
In v21.1.1 of jsdom, the nwsapi version is ^2.2.2, so v2.2.4 should be installed when you run tests with GitHub actions.

In package.json, temporary added overrides field and fixed nwsapi version to 2.2.2.

  "overrides": {
    "jsdom": {
      ".": "$jsdom",
      "nwsapi": "2.2.2"
    }
  },

Running tests again, it passed.
Add overrides field asamuzaK/sidebarTabs@01b8bb2

So it looks like v2.2.4 still has a bug.

Additional test for dperini/nwsapi#83

asamuzaK · 2023-04-15T08:56:17Z

Updated nwsapi to v2.2.4, it failed.

  "overrides": {
    "jsdom": {
      ".": "$jsdom",
      "nwsapi": "2.2.4"
    }
  },

Update nwsapi · asamuzaK/sidebarTabs@37c2664

asamuzaK · 2023-04-16T07:54:17Z

It looks like the regexp in logicalsel is not correct.

Refer to https://github.com/dperini/nwsapi/blob/master/src/nwsapi.js#L1002

console.log(':not(.foo):not(.bar):not([data-baz="1"])'.match(Patterns.logicalsel));

/*
[
  ':not(.foo):not(.bar):not([data-baz="1"])',
  'not',
  '.foo):not(.bar):not([data-baz="1"]',
  '',
  index: 0,
  input: ':not(.foo):not(.bar):not([data-baz="1"])',
  groups: undefined
]
*/

console.log(':not(.foo):not(.bar, [data-baz="1"])'.match(Patterns.logicalsel));

/*
[
  ':not(.foo):not(.bar, [data-baz="1"])',
  'not',
  '.foo):not(.bar, [data-baz="1"]',
  '',
  index: 0,
  input: ':not(.foo):not(.bar, [data-baz="1"])',
  groups: undefined
]
*/

console.log(':not(.foo):not(.bar[data-baz="1"])'.match(Patterns.logicalsel));

/*
[
  ':not(.foo):not(.bar[data-baz="1"])',
  'not',
  '.foo):not(.bar[data-baz="1"]',
  '',
  index: 0,
  input: ':not(.foo):not(.bar[data-baz="1"])',
  groups: undefined
]
*/

If there is an attribute selector at the end of the last :not(), it does not match correctly.

spuente · 2023-04-16T09:43:03Z

Hi, the infinite loop issue has been fixed but still getting this test failure in tests that were passing prior to release 2.2.3: SyntaxError: 'style):not(script):not(noscript):not(link):not([tabIndex="-1"]' is not a valid selector

at emit (node_modules/nwsapi/src/nwsapi/js:557:17) at Object._matches [as match] (node_modules/nwsapi/src/nwsapi/js:1400:9) at Array.Resolver (eval at compile (node_modules/nwsapi/src/nwsapi/js:760:17)) at collect (node_modules/nwsapi/src/nwsapi/js:1552:21)

Hi @rocioimpa, I was facing a similar issue. The error seems to occur when there's an attribute selector (like [tabIndex="-1"]) inside of a :not and after a selector that is not an attribute (like link, script, .my-class, etc) inside of a :not.
I'm not very familiar with the internals of this library, but it seems like the fix for this issue (which was not occurring on v2.2.2) would probably involve some reverts, code updates and generation of a new patch version.

As of now and using v2.2.4, I've found a couple of workarounds that could help:

Switch the order of your selectors to have :not([tabIndex="-1"] before the rest of :not selectors:

:not([tabIndex="-1"]):not(style):not(script):not(noscript):not(link)

Use a list of selectors inside of :not (See https://github.com/dperini/nwsapi/wiki/CSS-supported-selectors#logical-combination-pseudo-classes-selectors). It will look something like :not(selector1, selector2, ..., selectorN) and order won't matter, so any of the following would work:

:not([tabIndex="-1"], style, script, noscript, link)

:not(style, script, noscript, link, [tabIndex="-1"])

spuente · 2023-04-16T10:13:44Z

Hello, I'm also experiencing tests failing in >=2.2.3 that were passing in 2.2.2.

SyntaxError: 'slot):not([inert]' is not a valid selector
      at emit (node_modules/nwsapi/src/nwsapi.js:557:17)
      at Object._matches [as match] (node_modules/nwsapi/src/nwsapi.js:1400:9)
      at Array.Resolver (eval at compile (node_modules/nwsapi/src/nwsapi.js:760:17), <anonymous>:3:105)
      at collect (node_modules/nwsapi/src/nwsapi.js:1552:21)
      at Object._querySelectorAll [as select] (node_modules/nwsapi/src/nwsapi.js:1509:36)

To reproduce:

it("should not throw", () => {
  expect(() =>
    document.querySelectorAll("[tabindex]:not(slot):not([inert])")
  ).not.toThrow();
});

Hi @cole-adams, you could work around this issue by changing the order of the :not selectors to set :not([inert]) before :not(slot):

document.querySelectorAll("[tabindex]:not([inert]):not(slot)")

Or you could group the selectors inside a single :not(...) (See https://github.com/dperini/nwsapi/wiki/CSS-supported-selectors#logical-combination-pseudo-classes-selectors). In this case, the order won't matter, so both of the following snippets should work:

document.querySelectorAll("[tabindex]:not([inert], slot)")

document.querySelectorAll("[tabindex]:not(slot, [inert])")

I'm not very familiar with the internals of this library, so the above suggestions are just meant to work around the issue as a consumer of the library. I hope someone with more context of this codebase can find a definite fix, as this issue was not present on v2.2.2.

asamuzaK · 2023-04-16T11:38:15Z

:is() takes <forgiving-selector-list> as an argument, while :not() takes <complex-real-selector-list>.
IMHO, you should use different regexp for :is() and :not().

stefcameron · 2023-04-26T13:45:04Z

Or you could group the selectors inside a single :not(...) (See https://github.com/dperini/nwsapi/wiki/CSS-supported-selectors#logical-combination-pseudo-classes-selectors). In this case, the order won't matter, so both of the following snippets should work:
document.querySelectorAll("[tabindex]:not([inert], slot)")
document.querySelectorAll("[tabindex]:not(slot, [inert])")
I'm not very familiar with the internals of this library, so the above suggestions are just meant to work around the issue as a consumer of the library. I hope someone with more context of this codebase can find a definite fix, as this issue was not present on v2.2.2.

This suggestion involves using :not() with multiple arguments, which is not widely-enough supported yet across browsers.

We could change the order of the selectors as suggested here in https://github.com/focus-trap/tabbable/blob/master/src/index.js#L12

Hi @cole-adams, you could work around this issue by changing the order of the :not selectors to set :not([inert]) before :not(slot):
document.querySelectorAll("[tabindex]:not([inert]):not(slot)")

but since the order should not matter, it seems like not the right thing to do (or at least not something that should be necessary to do). I think nswapi should fix the bug instead of tabbable (and who knows how many other libraries) changing the order of their selectors just to make it work, aside from trying to mitigate this issue until it's fixed.

I might see if I can apply that v2.2.2 override to tabbable's package.json instead and then wait for the fix here.

dperini/nwsapi#83

stefcameron · 2023-05-03T23:08:11Z

[email protected] has been published which temporarily overrides nwsapi and pins it to v2.2.2 which does not cause the issue. I hope that helps folks while we wait for a fix here.

dperini · 2023-05-04T00:02:25Z

@stefcameron
I am really sorry for the troubles this gives you and the other users, however I am working on a fix for the multiple :not() selectors and specifically when one of them contains an attribute. Also suggestions are welcome and appreciated.

stefcameron · 2023-05-04T00:24:56Z

@dperini Thank you for working to fix the issue! Hopefully my fix takes a bit of pressure off for now. 😄

dperini · 2023-05-04T01:23:42Z

I want to share the regular expresssion that I am currently refining to solve the first part of the problem.
The objective is to split the multiple :not() selectors at the right position in the various possible cases.

https://regex101.com/r/QZFy85/1

by adding a ^ (caret sign) at the start of the RE (hooking the RE to the start of the input selector),
or adding a $ (dollar sign) at the end of the RE (hooking the RE to the end of the input selector),

I am able to pop out the first or the last :not() selector from the string at the correct place.
These selectors will then be handed one by one to Javascript which will resolve the selector series recursively.

Please, if anybody see a missing or failing case or can spot any inconsistency with this strategy report or suggest.

Thank you !

dperini · 2023-05-25T13:06:23Z

@ALL
please test if the new changes to the Regular Expression works in your cases.
This will be in 2.2.5 as soon as everybody confirm their case is resolved by this change.

stefcameron · 2023-05-29T13:52:40Z

@dperini I don't see a PR open with a fix against this issue. What line of code in nwsapi would you be updating to this new regex you're thinking of using in #83 (comment) ?

dperini · 2023-06-19T14:45:47Z

@stefcameron
forgive me, I missed this question which is now 3 weeks old, but anyway ...
The regex I was talking about is the one in L #81 named "logicalsel",
the one in charge of resolving :is(), :not() and :has() pseudo-classes.
I finally changed it yesterday in commit 01216f8

stefcameron · 2023-06-19T15:36:23Z

@dperini Better late than never! 😉 Thank you for the commit and the updated code. I'll try to give this a try locally later this week when I have time for focus-trap and will report back here with my findings.

stefcameron · 2023-06-26T20:31:50Z

@dperini I installed nwsapi v2.2.5, replaced that one line with your updated one, and ran our tabbable tests. All passed! Though I'll be honest, they also passed with v2.2.5 and the old Regex as-is -- while they did not when this issue first surfaced. I don't understand why. But all passed with your changes.

asamuzaK · 2023-06-26T23:09:35Z

https://regex101.com/r/QZFy85/1
:not(:is([foo=bar], .baz[qux]))
only matches :is([foo=bar], .baz[qux])

dperini · 2023-06-28T10:36:19Z

@asamuzaK
cannot reproduce currently published copy on nwsapi GitHub repository works for me.
Both the followed selector queries:

    :not(:is([foo=bar], .baz[qux]))
    :is(:not([foo=bar], .baz[qux]))

return the same results using "nwsapi" or "native" methods.
Please setup a minimal test that shows the problem, maybe I misunderstood where you are testing.
On RegEx101 all the test input lines return correct results.
PS: there were a few commit to "nwsapi" recently.

asamuzaK · 2023-06-28T23:45:33Z

The previous comment is the result of testing with https://regex101.com/r/QZFy85/1

go to https://regex101.com/r/QZFy85/1
clear test string
input :not(:is([foo=bar], .baz[qux]))

Expected:
:not(:is([foo=bar], .baz[qux])) is highlighted

Actual:
:is([foo=bar], .baz[qux]) is highlighted

dperini · 2023-06-29T10:14:55Z

@asamuzaK
I got it now ... sorry for the misunderstanding but 1 month has gone from when I published that message.
I should have explained more about that RE. It should apply only to the expression contained in the first logical selector.
The matched logical selector (let's say the :not() selector as in this case) is removed before the expression is evaluated and the compound expression is then handed back to Javascript for the recursion to work.

Have a look at the included RE.txt document which I quickly created trying to explain this process:

RE.txt

asamuzaK mentioned this issue Apr 15, 2023

Update nwsapi dependency jsdom/jsdom#3528

Closed

asamuzaK added a commit to asamuzaK/sidebarTabs that referenced this issue Apr 15, 2023

Update nwsapi

37c2664

Additional test for dperini/nwsapi#83

asamuzaK mentioned this issue Apr 15, 2023

Add workarounds for nwsapi and remove overrides field asamuzaK/sidebarTabs#258

Merged

This was referenced Apr 17, 2023

Concatenated :not selector doesn't work since version 2.2.3 #85

Closed

Upgrade bootstrap to 5.3.0-alpha3 plone/mockup#1305

Merged

thumbsupep mentioned this issue Apr 25, 2023

jsdom issue: 'slot):not([inert]' is not a valid selector focus-trap/tabbable#982

Closed

mmso added a commit to ProtonMail/WebClients that referenced this issue May 2, 2023

Lock nwsapi to 2.2.2

08e9c93

dperini/nwsapi#83

stefcameron mentioned this issue May 3, 2023

Remove nwsapi v2.2.2 override once bug is fixed focus-trap/tabbable#997

Open

steveworkman mentioned this issue May 11, 2023

document.querySelector throws error with bad selector bezoerb/inline-critical#300

Open

stefcameron mentioned this issue May 11, 2023

SyntaxError: 'slot):not([inert]' is not a valid selector focus-trap/focus-trap-react#986

Closed

asamuzaK mentioned this issue Feb 15, 2024

Revert to v2.2.2 #104

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Infinite loop fixed but still getting error in selectors #83

Infinite loop fixed but still getting error in selectors #83

rocioimpa commented Apr 13, 2023

alexgwolff commented Apr 13, 2023

dperini commented Apr 13, 2023

cole-adams commented Apr 14, 2023

asamuzaK commented Apr 15, 2023

dperini commented Apr 15, 2023

asamuzaK commented Apr 15, 2023

asamuzaK commented Apr 15, 2023 •

edited

Loading

asamuzaK commented Apr 16, 2023

spuente commented Apr 16, 2023 •

edited

Loading

spuente commented Apr 16, 2023 •

edited

Loading

asamuzaK commented Apr 16, 2023

stefcameron commented Apr 26, 2023

stefcameron commented May 3, 2023

dperini commented May 4, 2023

stefcameron commented May 4, 2023

dperini commented May 4, 2023

dperini commented May 25, 2023

stefcameron commented May 29, 2023

dperini commented Jun 19, 2023 •

edited

Loading

stefcameron commented Jun 19, 2023

stefcameron commented Jun 26, 2023

asamuzaK commented Jun 26, 2023

dperini commented Jun 28, 2023

asamuzaK commented Jun 28, 2023

dperini commented Jun 29, 2023

Infinite loop fixed but still getting error in selectors #83

Infinite loop fixed but still getting error in selectors #83

Comments

rocioimpa commented Apr 13, 2023

alexgwolff commented Apr 13, 2023

dperini commented Apr 13, 2023

cole-adams commented Apr 14, 2023

asamuzaK commented Apr 15, 2023

dperini commented Apr 15, 2023

asamuzaK commented Apr 15, 2023

asamuzaK commented Apr 15, 2023 • edited Loading

asamuzaK commented Apr 16, 2023

spuente commented Apr 16, 2023 • edited Loading

spuente commented Apr 16, 2023 • edited Loading

asamuzaK commented Apr 16, 2023

stefcameron commented Apr 26, 2023

stefcameron commented May 3, 2023

dperini commented May 4, 2023

stefcameron commented May 4, 2023

dperini commented May 4, 2023

dperini commented May 25, 2023

stefcameron commented May 29, 2023

dperini commented Jun 19, 2023 • edited Loading

stefcameron commented Jun 19, 2023

stefcameron commented Jun 26, 2023

asamuzaK commented Jun 26, 2023

dperini commented Jun 28, 2023

asamuzaK commented Jun 28, 2023

dperini commented Jun 29, 2023

asamuzaK commented Apr 15, 2023 •

edited

Loading

spuente commented Apr 16, 2023 •

edited

Loading

spuente commented Apr 16, 2023 •

edited

Loading

dperini commented Jun 19, 2023 •

edited

Loading