Skip to content

Commit

Permalink
fix(#4160): NumberField: screen reader fails to announce negative val…
Browse files Browse the repository at this point in the history
…ues with currencySign: 'accounting' (#4161)

* fix(#4160): NumberField: screen reader fails to announce negative values with currencySign: 'accounting'
  • Loading branch information
majornista authored Mar 8, 2023
1 parent b64e64f commit d6adfeb
Show file tree
Hide file tree
Showing 2 changed files with 43 additions and 7 deletions.
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

1 comment on commit d6adfeb

@rspbot
Copy link

@rspbot rspbot commented on d6adfeb Mar 8, 2023

Choose a reason for hiding this comment

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

Please sign in to comment.