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 the RegExp flag used for input pattern validation #3681

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

stnguyen90
Copy link

According to MDN (https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes/pattern#overview), the RegExp flag was changed from 'u' to 'v' around mid-2023, so jsdom should update to match that.

Fixes: #3680

@stnguyen90
Copy link
Author

I think it'd be good to add a test-case too, but I'm not sure what the best approach is. Should I create a new test file or copy https://github.com/web-platform-tests/wpt/blob/master/html/semantics/forms/constraints/form-validation-validity-patternMismatch.html into test/web-platform-tests/to-upstream/html/semantics/forms/constraints?

@stnguyen90
Copy link
Author

I think it'd be good to add a test-case too, but I'm not sure what the best approach is. Should I create a new test file or copy https://github.com/web-platform-tests/wpt/blob/master/html/semantics/forms/constraints/form-validation-validity-patternMismatch.html into test/web-platform-tests/to-upstream/html/semantics/forms/constraints?

Never mind, I guess the test was skipped in jsdom, so I updated test/web-platform-tests/to-run.yaml to not expect failure for some of the related tests.

@stnguyen90
Copy link
Author

Looks like node 18 fails because it doesn't support the v flag:

image

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work!!

@@ -1107,7 +1107,6 @@ formaction.html: [fail, Unknown]

DIR: html/semantics/forms/constraints

form-validation-validity-patternMismatch.html: [fail, Unknown]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use fail-node18 here (and below) to signal the right support matrix, and the tests should pass, I think!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the tip! I added it, but there were more tests that fail. Essentially, any test where the input has a pattern will fail. Should I add more fail-node18 to those tests?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see. Well, we don't want to make jsdom (or the pattern parts of it) completely unusable on Node 18, at least until it goes out of LTS support. So I guess we need a fallback, that try/catches with "v" and then falls back to "u".

@stnguyen90 stnguyen90 marked this pull request as draft February 13, 2024 00:42
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.

Pattern works in Chrome, but not jsdom
2 participants