Skip to content

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

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 11 additions & 4 deletions packages/@react-aria/numberfield/src/useNumberField.ts
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ export function useNumberField(props: AriaNumberFieldProps, state: NumberFieldSt
decrement,
decrementToMin,
numberValue,
inputValue,
commit
} = state;

Expand All @@ -99,6 +100,14 @@ export function useNumberField(props: AriaNumberFieldProps, state: NumberFieldSt
}
});

let numberFormatter = useNumberFormatter(formatOptions);
let intlOptions = useMemo(() => numberFormatter.resolvedOptions(), [numberFormatter]);

// Replace negative textValue formatted using currencySign: 'accounting'
// with a textValue that can be announced using a minus sign.
let textValueFormatter = useNumberFormatter({...formatOptions, currencySign: undefined});
let textValue = useMemo(() => isNaN(numberValue) ? '' : textValueFormatter.format(numberValue), [textValueFormatter, numberValue]);

let {
spinButtonProps,
incrementButtonProps: incButtonProps,
Expand All @@ -115,7 +124,7 @@ export function useNumberField(props: AriaNumberFieldProps, state: NumberFieldSt
onDecrement: decrement,
onDecrementToMin: decrementToMin,
value: numberValue,
textValue: state.inputValue
textValue
}
);

Expand Down Expand Up @@ -144,8 +153,6 @@ export function useNumberField(props: AriaNumberFieldProps, state: NumberFieldSt
// Browsers and operating systems are quite inconsistent about what keys are available, however.
// We choose between numeric and decimal based on whether we allow negative and fractional numbers,
// and based on testing on various devices to determine what keys are available in each inputMode.
let numberFormatter = useNumberFormatter(formatOptions);
let intlOptions = useMemo(() => numberFormatter.resolvedOptions(), [numberFormatter]);
let hasDecimals = intlOptions.maximumFractionDigits > 0;
let hasNegative = isNaN(state.minValue) || state.minValue < 0;
let inputMode: TextInputDOMProps['inputMode'] = 'numeric';
Expand Down Expand Up @@ -183,7 +190,7 @@ export function useNumberField(props: AriaNumberFieldProps, state: NumberFieldSt
isReadOnly,
isRequired,
validationState,
value: state.inputValue,
value: inputValue,
defaultValue: undefined, // defaultValue already used to populate state.inputValue, unneeded here
autoComplete: 'off',
'aria-label': props['aria-label'] || null,
Expand Down
35 changes: 32 additions & 3 deletions packages/@react-spectrum/numberfield/test/NumberField.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,9 @@
* governing permissions and limitations under the License.
*/

jest.mock('@react-aria/live-announcer');
import {act, fireEvent, render, triggerPress, typeText, within} from '@react-spectrum/test-utils';
import {announce} from '@react-aria/live-announcer';
import {Button} from '@react-spectrum/button';
import {chain} from '@react-aria/utils';
import messages from '../../../@react-aria/numberfield/intl/*';
Expand Down Expand Up @@ -46,6 +48,7 @@ describe('NumberField', function () {
onBlurSpy.mockClear();
onKeyDownSpy.mockClear();
onKeyUpSpy.mockClear();
announce.mockClear();
// there's an announcer, make sure to run through it
// make sure only to run the pending timers, spin button can cause an infinite loop if we run all
act(() => {jest.runOnlyPendingTimers();});
Expand Down Expand Up @@ -183,8 +186,7 @@ describe('NumberField', function () {
expect(onChangeSpy).toHaveBeenCalledWith(52);
});

// TODO: this doesn't work in Node 12 but it does in 13, once we can move to that in circle ci this can be un-skipped
it.skip.each`
it.each`
Name
${'NumberField'}
`('$Name switches to numeric for percentages', () => {
Expand Down Expand Up @@ -873,33 +875,48 @@ describe('NumberField', function () {
expect(textField).toHaveAttribute('value', '($9.00)');
expect(onChangeSpy).toHaveBeenCalledTimes(1);
expect(onChangeSpy).toHaveBeenCalledWith(-9);
expect(announce).toHaveBeenCalledTimes(1);
expect(announce).toHaveBeenLastCalledWith('−$9.00', 'assertive');

act(() => {textField.focus();});
textField.setSelectionRange(2, 3);
userEvent.type(textField, '{backspace}');
expect(announce).toHaveBeenCalledTimes(2);
expect(announce).toHaveBeenLastCalledWith('−$.00', 'assertive');
textField.setSelectionRange(2, 2);
typeText(textField, '1');
expect(announce).toHaveBeenCalledTimes(3);
expect(announce).toHaveBeenLastCalledWith('−$1.00', 'assertive');
textField.setSelectionRange(3, 3);
typeText(textField, '8');
expect(textField).toHaveAttribute('value', '($18.00)');
act(() => {textField.blur();});
expect(textField).toHaveAttribute('value', '($18.00)');
expect(onChangeSpy).toHaveBeenCalledTimes(2);
expect(onChangeSpy).toHaveBeenLastCalledWith(-18);
expect(announce).toHaveBeenCalledTimes(4);
expect(announce).toHaveBeenLastCalledWith('−$18.00', 'assertive');

act(() => {textField.focus();});
textField.setSelectionRange(7, 8);
userEvent.type(textField, '{backspace}');
expect(textField).toHaveAttribute('value', '($18.00');
expect(announce).toHaveBeenCalledTimes(5);
expect(announce).toHaveBeenLastCalledWith('($18.00', 'assertive');
act(() => {textField.blur();});
expect(textField).toHaveAttribute('value', '$18.00');
expect(onChangeSpy).toHaveBeenCalledTimes(3);
expect(onChangeSpy).toHaveBeenLastCalledWith(18);

act(() => {textField.focus();});
userEvent.clear(textField);
expect(announce).toHaveBeenCalledTimes(6);
expect(announce).toHaveBeenLastCalledWith('Empty', 'assertive');
typeText(textField, '($32)');
expect(textField).toHaveAttribute('value', '($32)');
expect(announce).toHaveBeenCalledTimes(11);
console.log(announce.mock.calls[4]);
expect(announce).toHaveBeenLastCalledWith('−$32', 'assertive');
act(() => {textField.blur();});
expect(textField).toHaveAttribute('value', '($32.00)');
expect(onChangeSpy).toHaveBeenCalledTimes(4);
Expand All @@ -915,13 +932,17 @@ describe('NumberField', function () {
act(() => {textField.focus();});
userEvent.type(textField, '(10)');
expect(textField).toHaveAttribute('value', '(10)');
expect(announce).toHaveBeenCalledTimes(4);
expect(announce).toHaveBeenLastCalledWith('−10', 'assertive');
expect(announce).toHaveBeenCalledTimes(4);
expect(announce).toHaveBeenLastCalledWith('−10', 'assertive');
act(() => {textField.blur();});
expect(textField).toHaveAttribute('value', '(US$10.00)');
expect(onChangeSpy).toHaveBeenCalledTimes(1);
expect(onChangeSpy).toHaveBeenLastCalledWith(-10);
});

it.each`
it.only.each`
Name
${'NumberField'}
`('$Name can edit a currencySign accounting in a locale that does not use the parenthesis notation', () => {
Expand All @@ -936,17 +957,25 @@ describe('NumberField', function () {
expect(textField).toHaveAttribute('value', '-9,00 $');
expect(onChangeSpy).toHaveBeenCalledTimes(1);
expect(onChangeSpy).toHaveBeenCalledWith(-9);
expect(announce).toHaveBeenCalledTimes(1);
expect(announce).toHaveBeenLastCalledWith('−9,00 $', 'assertive');

act(() => {
textField.focus();
});
textField.setSelectionRange(1, 2);
userEvent.type(textField, '{backspace}');
expect(announce).toHaveBeenCalledTimes(2);
expect(announce).toHaveBeenLastCalledWith('−0,00 $', 'assertive');
textField.setSelectionRange(1, 1);
typeText(textField, '1');
expect(announce).toHaveBeenCalledTimes(3);
expect(announce).toHaveBeenLastCalledWith('−1,00 $', 'assertive');
textField.setSelectionRange(2, 2);
typeText(textField, '8');
expect(textField).toHaveAttribute('value', '-18,00 $');
expect(announce).toHaveBeenCalledTimes(4);
expect(announce).toHaveBeenLastCalledWith('−18,00 $', 'assertive');
act(() => {
textField.blur();
});
Expand Down