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

[browser] Fix HybridGlobalization, SoftHyphen is not ignored when it should be #105946

Merged
merged 8 commits into from
Aug 15, 2024

Conversation

ilonatommy
Copy link
Member

@ilonatommy ilonatommy commented Aug 5, 2024

Fixes #95473.

  • compare / endsWith / startsWith ignored only:
    \u200B (Zero Width Space)
    \u200C (Zero Width Non-Joiner)
    \u200D (Zero Width Joiner)
    \uFEFF (Zero Width No-Break Space)
    \0 (Null character)
    With this PR they start ignoring \u00AD as well.

  • indexOf did not ignore any of above because that would affect the index count. It turns out that \u00AD (SoftHyphen) does not matter in index count and it's correct to ignore it. Other characters ignored by comparison functions influence the index count.

  • Moving the turkish-i issue from blocking full test to blocking specific test cases that fail

  • Unblocking the test uncovered another, non related issue: String.Replace uses indexing function implementation with matchLength parameter. This implementation is not supported by HybridGlobalization on browser - matchLength should throw. This PR changes the expected value for tests that use String.Replace and adds comment in the docs.

  • We were not throwing PNSE for IndexOf implementation with matchLength parameter even though we do not calculate it and similar methods, IsPrefix and IsSuffix throw PNSE by design. Now we start throwing correctly.

    Web API does not expose locale-sensitive endsWith/startsWith function. As a workaround, both strings get normalized and weightless characters are removed. Resulting strings are cut to the same length and comparison is performed. This approach, beyond having the same compare option limitations as described under **String comparison**, has additional limitations connected with the workaround used. Because we are normalizing strings to be able to cut them, we cannot calculate the match length on the original strings. Methods that calculate this information throw PlatformNotSupported exception:

  • Documentation got updated to reflect the actual state - we throw PNSE when methods with matchLength are used.

ToDo:

  • iOS should also throw when IndexOf with matchLength is used. Make a follow-up PR with it.

@ilonatommy ilonatommy self-assigned this Aug 5, 2024
@ilonatommy ilonatommy marked this pull request as draft August 5, 2024 13:50
@ilonatommy ilonatommy marked this pull request as ready for review August 5, 2024 14:24
@ilonatommy
Copy link
Member Author

/azp run runtime-wasm

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ilonatommy
Copy link
Member Author

/azp run runtime-wasm

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ilonatommy
Copy link
Member Author

/azp run runtime-wasm

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ilonatommy
Copy link
Member Author

/azp run runtime-wasm

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@matouskozak matouskozak left a comment

Choose a reason for hiding this comment

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

A few small nits but overall LGTM!

docs/design/features/globalization-hybrid-mode.md Outdated Show resolved Hide resolved
docs/design/features/globalization-hybrid-mode.md Outdated Show resolved Hide resolved
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[browser] HybridGlobalization, SoftHypen is not ignored when it should be
3 participants