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

Add unicode minus symbol #10636

Merged
merged 1 commit into from
Jan 28, 2020
Merged

Conversation

JamesCatt
Copy link
Contributor

@JamesCatt JamesCatt commented Dec 18, 2019

Adds support for the unicode minus symbol, including when used for a negative number.

Link to issue number:

Fixes #10633

Summary of the issue:

NVDA does not identify negative numbers (including currencies) that use the unicode minus symbol, or announce unicode minus symbol in general.
Information on the minus symbol (U+2212)

Description of how this pull request fixes the issue:

  • Updates negative number regex to work with unicode minus in addition to existing dash
  • Adds minus to symbol list

Testing performed:

  • Existing unit tests
  • Confirmed working with:
    • Chrome 79
    • Firefox 72.0b7
    • IE11
    • MS Edge 44 (EdgeHTML)
    • MS Edge 80 (Chromium)
  • Also confirmed negative numbers still work with dash in addition to minus

Known issues with pull request:

None

Change log entry:

Bug Fixes
Fixed not announcing Unicode minus symbol (U+2212) (#10633)

@LeonarddeR
Copy link
Collaborator

Note that this isn't an utf-8 symbol but an Unicode symbol. Utf-8 is an encoded representation of Unicode.

@JamesCatt JamesCatt changed the title Add utf8 minus symbol Add unicode minus symbol Dec 19, 2019
@JamesCatt
Copy link
Contributor Author

Note that this isn't an utf-8 symbol but an Unicode symbol. Utf-8 is an encoded representation of Unicode.

Roger that, my mistake. Updated the PR to correct that. Thanks for pointing it out.

Adds support for the unicode minus symbol, including when used for a negative number.
@JamesCatt
Copy link
Contributor Author

@LeonarddeR should I be requesting a review from someone? If so, who would you suggest? Tx

@Adriani90
Copy link
Collaborator

@JamesCatt you can request review from @michaelDCurran or from @feerrenrut in this case since they are from NV Access and the only people who can merge pull requests into master, beta or any other branch.

@JamesCatt
Copy link
Contributor Author

@JamesCatt you can request review from @michaelDCurran or from @feerrenrut in this case since they are from NV Access and the only people who can merge pull requests into master, beta or any other branch.

Will do, thanks!

@StommePoes
Copy link

Does this allow minus to be read out if it's not directly prefacing a financial symbol? For example, a math sentence: 3 − 2 ? (with a space after it?)

I'm rusty on my regexen, but I think that's what I'm seeing: just as a dash on its own is generally silent but sounds "minus" when directly next to a number, does this mean we still have zero recourse to get the − character itself to be read at some punctuation level?

@JamesCatt
Copy link
Contributor Author

JamesCatt commented Jan 25, 2020

Does this allow minus to be read out if it's not directly prefacing a financial symbol? For example, a math sentence: 3 − 2 ? (with a space after it?)

I'm rusty on my regexen, but I think that's what I'm seeing: just as a dash on its own is generally silent but sounds "minus" when directly next to a number, does this mean we still have zero recourse to get the − character itself to be read at some punctuation level?

Nope, I added a standalone entry for the character as well (see line 59 in the diff), so it will be announced regardless of the context it appears in.

Copy link
Contributor

@feerrenrut feerrenrut left a comment

Choose a reason for hiding this comment

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

This looks good, I added some information to the PR description about the exact Unicode symbol (U+2212).

I have tested this (very quickly) with espeak, I assume @JamesCatt you did the same. Please could you test with several other synthesizers, in particular OneCore.

I'll merge this now, but please comment on here to let me know which synthesizers have been tested. Thanks for the contribution!

@feerrenrut feerrenrut merged commit dee3b4d into nvaccess:master Jan 28, 2020
@nvaccessAuto nvaccessAuto added this to the 2019.3 milestone Jan 28, 2020
feerrenrut added a commit that referenced this pull request Jan 28, 2020
@feerrenrut feerrenrut modified the milestones: 2019.3, 2020.1 Jan 28, 2020
@JamesCatt
Copy link
Contributor Author

I'll merge this now, but please comment on here to let me know which synthesizers have been tested. Thanks for the contribution!

Will do, and thanks!

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.

Negative dollar amounts not announced with minus symbol
6 participants