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

Fix case insensetivity for attribute selectors #59

Conversation

ypapouski
Copy link

@ypapouski ypapouski commented Jun 30, 2022

The fix for the issue #60.

@dperini
Copy link
Owner

dperini commented Jul 16, 2022

Ouch ... maybe this is still valid. I confused this to be part of the last regression which made selectors case-sensitive.
But this is about attributes case-sensitivity.
I will check this out asap.

@dperini
Copy link
Owner

dperini commented Jul 22, 2022

@ypapouski
thank you for your effort and contribution for this improvement.
I find it strange the wpt do not have a test for detecting this inconsistency with attributes case sensitivity.
I tested your changes with both my local test files and with those of jsdom + web_platform_tests and everything run smooth.

Now I find myself facing a minor problem that I wish to discuss with you as owner of this patch and contributor of the code change.

The minor problem is related to ES6 syntax you have used in this patch that chokes with the ancient ES3 syntax used previously, so I will be happy to have your suggestion here. I really like ES6 but for now I would like to maintain ES3 syntax for several reasons.

So to apply this patch do you prefer to change the syntax to ES3 and redo the pull request or should I do it myself ?
Obviously, in both cases, I would keep the reference to you as the contributor of this patch.
Appreciated your work and help refining this problem, thank you again !

@ypapouski
Copy link
Author

@dperini
I am glad to hear that my fix works correctly :)

About ES3 and ES6, well, I have no problems if you convert my changes made in ES6 into ES3.

Thanks for your feedback.

@dperini
Copy link
Owner

dperini commented Apr 6, 2023

This was fixed in #60

@dperini dperini closed this Apr 6, 2023
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

Successfully merging this pull request may close these issues.

2 participants