-
Notifications
You must be signed in to change notification settings - Fork 1.3k
fix(#4160): NumberField: screen reader fails to announce negative values with currencySign: 'accounting' #4161
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
Conversation
…ues with currencySign: 'accounting'
Build successful! 🎉 |
Build successful! 🎉 |
…ing' into useNumberField
Build successful! 🎉 |
Build successful! 🎉 |
…unce-negative-values-with-currencysign-accounting
Build successful! 🎉 |
// Replace negative value formatted using currencySign: 'accounting' | ||
// with a value that can be announced using a minus sign. | ||
.replace(/^\((.+)\)$/, '\u2212$1'); | ||
textValue = textValue === '' ? stringFormatter.format('Empty') : (textValue || `${value}`).replace('-', '\u2212'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should also be moved up to useNumberField, because then we can grab the minusSign character and out of formatToParts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the problem was that it is coming from the number formatter, but the screen reader doesn't announce that character right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right, but there may be other minus sign symbols, if we moved it up, we could always replace whatever symbol with the one that we know the screen reader can announce?
if I recall correctly, there's like 3 of them at least
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't useSpinButton
handle announcement of the minus sign and "Empty" at this lower level? I can understand moving the currency formatting up, the useNumberField, because that's where we accept formatOptions
, but it seems to make sense to keep the logic to announce 'minus' here. Are you saying there are three Unicode symbols for the minus sign that screen readers can announce, or three locales that use different symbols for the minus sign, that might have to be replaced with a more robust regex?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are around 3 different locale specific minus signs, if I recall correctly.
But now I feel like I'm expanding the scope of this.
So let's keep useSpinButton as it was, and handle the currency negative up in useNumberField as you were doing.
…unce-negative-values-with-currencysign-accounting
Build successful! 🎉 |
## API Changes
unknown top level export { type: 'identifier', name: 'Column' } |
@majornista in testing we noticed a couple issues:
I noticed in the number field code we override the spin button role and all of the associated attributes to work around a VoiceOver issue (according to the comment): react-spectrum/packages/@react-aria/numberfield/src/useNumberField.ts Lines 216 to 223 in 574a27d
Maybe we should make this happen only on Apple platforms so that NVDA treats this as a spin button and announces the |
Closes #4160
✅ Pull Request Checklist:
📝 Test Instructions:
🧢 Your Project:
Adobe/Accessibility