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

feat: normalize whitespace to expect().toHaveValue() #34818

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

Conversation

pengooseDev
Copy link
Contributor

Closes #23037

This PR fixes the issue where inputValue returns non-breaking spaces (NBSP, U+00A0) instead of regular spaces. The previous PR attempted to address the problem within toHaveValue, but the root cause was identified as inputValue itself returning U+00A0.

  • Although we initially tried to detect and normalize NBSP within toHaveValue, the normalization did not applied. For now, inputValue has been updated to replace NBSP with a regular space.
  • I'm not entirely sure if modifying inputValue is the ideal solution, but it resolves the issue for the time being.

Additionally, if you could share reproduction steps or documentation (as mentioned in the previous PR feedback regarding web browser tests), it would greatly help in future improvements.

Thank you.

Copy link
Member

@Skn0tt Skn0tt left a comment

Choose a reason for hiding this comment

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

This is a breaking change. Please put this behaviour change behind a flag, as pointed out in #23037 (comment). It should be configurable from expect().toHaveValue() as well.

@pengooseDev pengooseDev marked this pull request as draft February 18, 2025 04:39
@pengooseDev pengooseDev force-pushed the fix-normalize-inputValue branch from 3d91c38 to ec31aa5 Compare February 18, 2025 17:12

This comment has been minimized.

@pengooseDev
Copy link
Contributor Author

Thanks for nice feedback.

After adding the normalize flag to toHaveValue, I believe encouraging this usage pattern could effectively resolve the issue.

 await expect(locator).toHaveValue('Text and content');

However, third-party matchers—such as toContain (as seen in the provided example)—that do not offer a normalize option may still encounter this issue.

Additionally, none of the current matchers expose the normalizeWhiteSpace option externally. Following the team's existing approach, I have added the normalizeWhiteSpace flag accordingly. If the team later decides this flag should be exposed externally, I’d be happy to add it in a subsequent commit. :)

@pengooseDev pengooseDev changed the title fix(inputValue): convert non-breaking spaces to regular spaces feat: normalize whitespace to expect().toHaveValue() Feb 18, 2025
@pengooseDev pengooseDev marked this pull request as ready for review February 18, 2025 17:31
@pengooseDev pengooseDev requested a review from Skn0tt February 18, 2025 17:34

This comment has been minimized.

@pavelfeldman
Copy link
Member

I'm not even sure we want to accept the patch, the issue does not sound convincing to me. Value is a value, so we should compare it as such with no tolerance for error.

@pengooseDev
Copy link
Contributor Author

@pavelfeldman
I agree. It would be appreciated if the team could discuss whether to close the issue. :)

@pengooseDev pengooseDev force-pushed the fix-normalize-inputValue branch from 9bf7589 to 730ab5d Compare February 20, 2025 01:26

This comment has been minimized.

@pengooseDev pengooseDev force-pushed the fix-normalize-inputValue branch from 730ab5d to f9dd535 Compare February 21, 2025 12:53
Copy link
Contributor

Test results for "tests 1"

6 flaky ⚠️ [chromium-library] › tests/library/headful.spec.ts:233:3 › should click in OOPIF @chromium-ubuntu-22.04-node20
⚠️ [chromium-library] › tests/library/chromium/oopif.spec.ts:284:3 › should click @chromium-ubuntu-22.04-node22
⚠️ [firefox-page] › tests/page/page-evaluate.spec.ts:403:3 › should throw for too deep reference chain @firefox-ubuntu-22.04-node18
⚠️ [webkit-library] › tests/library/browsercontext-clearcookies.spec.ts:52:3 › should remove cookies by name @webkit-ubuntu-22.04-node18
⚠️ [webkit-library] › tests/library/browsercontext-device.spec.ts:45:5 › device › should scroll to click @webkit-ubuntu-22.04-node18
⚠️ [webkit-page] › tests/page/page-filechooser.spec.ts:24:5 › should upload multiple large files @webkit-ubuntu-22.04-node18

38678 passed, 794 skipped
✔️✔️✔️

Merge workflow run.

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.

[Feature] add option to normalize whitespace to expect().toHaveValue()
3 participants