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

Revert to v2.2.2 #104

Open
asamuzaK opened this issue Feb 15, 2024 · 15 comments
Open

Revert to v2.2.2 #104

asamuzaK opened this issue Feb 15, 2024 · 15 comments

Comments

@asamuzaK
Copy link

Most of the issue reports over the past year, e.g. #82, #83, #88, #89, #90, #95, #99, #100, #103 etc. indicate that the regular expressions used in v2.2.3 and later are incorrect.
I suggest reverting to v2.2.2 once and review the regexp.

@asamuzaK
Copy link
Author

As far as I tested, nwsapi does not handle the followings correctly.

  • namespaced elements or attribute selectors such as ns|E or [ns|attr]
  • Pseudo-element selectors
  • nth-child(an+b of s)
  • logical combinations with complex selectors, e.g. :is(.foo > .bar)
  • Almost all other pseudo-classes

@dperini
Copy link
Owner

dperini commented Feb 18, 2024

@asamuzaK you are welcome to suggest changes/improvements that you see fits.
All of the above problems are already listed in the issues.
I am going to commit attempts to fixes for all of them.

@dperini
Copy link
Owner

dperini commented Apr 30, 2024

@asamuzaK
a few fixes landed in "nwsapi" and some are related to the problems you listed.
Can you go through the faulty queries and see if the validator logic is now working ?
Thank you for your help.

@asamuzaK
Copy link
Author

asamuzaK commented May 4, 2024

Could you please publish the pre-release version to npm?
Then I think I can help you with the test.

@dperini
Copy link
Owner

dperini commented May 4, 2024

Do you mean publish a 2.2.10 release with the latest changes in github master or something else ?

I was already planning a release for tomorrow afternoon and I would prefer that if you can wait just a few more hours.

Validator problem is fixed and is the next fix I would commit on github later today, the problem was brought up by recent use of pseudo-classes names containing dashes (those should be escaped).

A meaningful release is planned for tomorrow after some clean up and testing of the fixes for focus-xxx, dir(rtl/ltr), media states and remaining open issues. Many of the issue will be cleared by the validator fixes.

I am sorry for the hassle this created in various projects, but I will do my best to have everything running smooth again with due additions and improvements with a new way/tool to test my work based on WPT testing suite that would give extra testing capabilities to have users help me in the maintenance of "nwsapi".

New commits should start flowing in a few hours for you to review and suggest.

Thank you in advance for your help and contributions.

@asamuzaK
Copy link
Author

asamuzaK commented May 4, 2024

I was already planning a release for tomorrow afternoon and I would prefer that if you can wait just a few more hours.

Great, I'll wait for it.

As a side note, I recommend you to write unit tests for each commit.

@dperini
Copy link
Owner

dperini commented May 5, 2024

@asamuzaK
I have started the commits I was talking about.
Maybe you can give a look to the validator and suggest improvements.
Will continue to do commits to fix bugs and add new capabilities in Selectors Level 4.

@asamuzaK
Copy link
Author

asamuzaK commented May 6, 2024

Some feedback.
I will explain with some logs based on jsdom+nwsapi.

First is the log of v2.2.2.
jsdom_nwsapi2_2_2.txt
There are 2 errors compared to v2.2.7.
The first error is related to :focus, but this is a bug in jsdom itself, so it is expected to throw an error.
jsdom does not lose focus and continues to maintain focus even if the attr of the focused element changes and the element is expected to lose focus.
In other words, v2.2.2 has the correct implementation regarding :focus.
The second error has the message Hey, did you fix a bug?.
The expected error did not occur and it means that there is a regression between v2.2.2 and v2.2.7.
It shows that the implementation of :is(), :where() added after v2.2.2 is incorrect.

Next is a comparison between v2.2.7 and v2.2.9.
jsdom_nwsapi2_2_9.txt
There are 12 errors.
8 of them are Hey, did you fix a bug?; validation errors related to :has() did not occur.
But please note that the implementation of :has() itself has not been resolved.
The 9th error is related to :scope, this is a step forward. Great work.
The remaining 3 errors are regressions related to namespace and CSS ident-token, that means the implementation in v2.2.7 was much more reasonable.

And the current master (commit 720c5e8)
jsdom_nwsap_master_720c5e8.txt
All 8 errors are regressions.

From above, I have to say that it has gotten worse and worse with each version since v2.2.2.
I strongly recommend to revert all of the commits since v2.2.9.

I'm planning to cut a branch from v2.2.9 and help to fix below.

  • ident-token implementation
  • :focus implementation
  • :is(), :not() implementation
  • namespace implementation

@dperini
Copy link
Owner

dperini commented May 10, 2024

@asamuzaK
now the logical selectors regular expression is the one used in 2.2.2 and many things are fixed.
Is it worth a 2.2.10 release or should we wait for more fixes ?

@asamuzaK
Copy link
Author

Maybe it's because of my poor English, I'm frustrated with myself that I can't communicate well with you...

The code in current nwsapi's master branch has too many regressions.
No matter how much you try to modify the regular expressions from the master, there will be few improvements.
It's better to just go back to some point i.e. v2.2.2.

For example, see Review source by asamuzaK · Pull Request #113 · dperini/nwsapi
It is a new branch cut from v2.2.2 and added some modifications.
Main difference from the master is that it lacks :has() and focus-visible supports at the moment, but no regressions.
To test this PR:

  • Fork jsdom
  • Cut new branch for testing
  • Replace node_modules/nwsapi/src/nwsapi.js file with src/nwsapi.js of the PR
  • Run npm run test-wpt and wait for a while (it takes long...)

@dperini
Copy link
Owner

dperini commented May 10, 2024

@asamuzaK
I can fully understand what you say and I wish to thank you for the help.
I normally execute the tests as you describe using jsdom, but I also execute all the tests I can in wpt and all the ones in the /test directory of my own repository.

If you want I can talk in a conference call through Skype, I have it currently open and my alias name there is "nwbox_tech".

It will be helpful to have a chat, writing is not as powerful in this case (due to a problem of mine).
But we are good to go no more errors that I see locally (minor errors exists but are not urgent like the blocker I have been fixing.

@asamuzaK
Copy link
Author

I told you that there are many regressions in the changes made after 2.2.9, so you should revert it.
I do not recommend releasing it as 2.2.10.

If you want I can talk in a conference call through Skype,

No thank you.

@dperini
Copy link
Owner

dperini commented May 11, 2024

@asamuzaK
everything is back to normal on my side, late though but that's how I can do it (slowly).
Going back to 2.2.2 is not an option for me, I solved all the necessary bits and in the afternoon I plan to release 2.2.10 ... only test failing is the ":has(> :scope)" mangled selector which I never supported anyway. Also ":has()" is getting tentative support and recently a bug was fixed.

@asamuzaK
Copy link
Author

Ref #115
As I said, there are a lot of regressions.

cee-chen added a commit to cee-chen/eui that referenced this issue Aug 9, 2024
- works correctly in browser but causes test failures

- see dperini/nwsapi#104 (comment)

- `:has()` support is apparently flaky, so we need to pin/revert the resolution
@cee-chen
Copy link

cee-chen commented Aug 9, 2024

only test failing is the ":has(> :scope)" mangled selector which I never supported anyway. Also ":has()" is getting tentative support and recently a bug was fixed.

:has() is now fully supported by all evergreen browsers - is there any update on when nwsapi will support the selector more fully? For context, we've had to pin to 2.2.2 to prevent test failures on the following selector, which works perfectly in browsers:

*:has(*:is(.classA, .classB)) + *:is(.classC, .classD) {
  color: red;
}

interestingly, the above works fine without the second :is() and also works fine with :not() instead of :has().

edit: The following previous sibling selector also throws nwsapi errors:

&:has(+ .classA) {
  color: red;
}

cee-chen added a commit to cee-chen/eui that referenced this issue Aug 9, 2024
- works correctly in browser but causes test failures

- see dperini/nwsapi#104 (comment)

- `:has()` support is apparently flaky, so we need to pin/revert the resolution
cee-chen added a commit to cee-chen/eui that referenced this issue Aug 9, 2024
- works correctly in browser but causes test failures

- see dperini/nwsapi#104 (comment)

- `:has()` support is apparently flaky, so we need to pin/revert the resolution
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

3 participants