nwsapi icon indicating copy to clipboard operation
nwsapi copied to clipboard

Revert to v2.2.2

Open asamuzaK opened this issue 1 year ago • 15 comments
trafficstars

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 avatar Feb 15 '24 12:02 asamuzaK

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

asamuzaK avatar Feb 18 '24 12:02 asamuzaK

@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 avatar Feb 18 '24 14:02 dperini

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

dperini avatar Apr 30 '24 20:04 dperini

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

asamuzaK avatar May 04 '24 09:05 asamuzaK

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.

dperini avatar May 04 '24 13:05 dperini

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.

asamuzaK avatar May 04 '24 13:05 asamuzaK

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

dperini avatar May 05 '24 19:05 dperini

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

asamuzaK avatar May 06 '24 11:05 asamuzaK

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

dperini avatar May 10 '24 04:05 dperini

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...)

asamuzaK avatar May 10 '24 14:05 asamuzaK

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

dperini avatar May 10 '24 17:05 dperini

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.

asamuzaK avatar May 11 '24 05:05 asamuzaK

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

dperini avatar May 11 '24 09:05 dperini

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

asamuzaK avatar May 26 '24 08:05 asamuzaK

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 avatar Aug 09 '24 16:08 cee-chen