fix: TimeInput not triggering onChange on incomplete values (#1711)

Update TimeInput to trigger onChange on incomplete input values, missing
chars filled in with zeros. Update internal/displayed value on blur to
match the last onChange.

fixes #1710

Author: vbabich

packages/components/src/TimeInput.test.tsx

@@ -114,7 +114,7 @@ describe('selection', () => {
 
     input.focus();
     input.setSelectionRange(selectionStart, selectionEnd, selectionDirection);
-    user.type(input, '{Shift}', {
+    await user.type(input, '{Shift}', {
       skipClick: true,
       initialSelectionStart: selectionStart,
       initialSelectionEnd: selectionEnd,
@@ -150,14 +150,14 @@ describe('selection', () => {
 });
 
 describe('select and type', () => {
-  async function testSelectAndType(
+  async function selectAndType(
     user: ReturnType<typeof userEvent.setup>,
     cursorPosition: number,
-    str: string,
-    expectedResult: string
+    str: string
   ) {
     const elementRef = React.createRef<TimeInputElement>();
-    const { unmount } = makeTimeInput({ ref: elementRef });
+    const onChange = jest.fn();
+    const { unmount } = makeTimeInput({ ref: elementRef, onChange });
     const input: HTMLInputElement = screen.getByRole('textbox');
 
     input.focus();
@@ -169,10 +169,36 @@ describe('select and type', () => {
       initialSelectionEnd: cursorPosition,
     });
     await user.keyboard(str);
+    return { input, onChange, unmount };
+  }
+  // Test internal/displayed value, but not the onChange callback
+  async function testSelectAndType(
+    user: ReturnType<typeof userEvent.setup>,
+    cursorPosition: number,
+    str: string,
+    expectedResult: string
+  ) {
+    const { input, unmount } = await selectAndType(user, cursorPosition, str);
 
     expect(input.value).toEqual(expectedResult);
     unmount();
   }
+  // Test the value in onChange callback
+  async function testSelectAndTypeOnChange(
+    user: ReturnType<typeof userEvent.setup>,
+    cursorPosition: number,
+    str: string,
+    expectedResult: string
+  ) {
+    const { onChange, unmount } = await selectAndType(
+      user,
+      cursorPosition,
+      str
+    );
+    expect(onChange).lastCalledWith(TimeUtils.parseTime(expectedResult));
+    unmount();
+  }
+
   it('handles typing after autoselecting a segment', async () => {
     const user = userEvent.setup();
     await testSelectAndType(user, 0, '0', '02:34:56');
@@ -247,6 +273,58 @@ describe('select and type', () => {
     unmount();
   });
 
+  it('fills in missing chars and triggers onChange', async () => {
+    const user = userEvent.setup();
+    await testSelectAndTypeOnChange(user, 1, '{backspace}', '00:34:56');
+    await testSelectAndTypeOnChange(user, 3, '{backspace}', '12:00:56');
+    await testSelectAndTypeOnChange(user, 6, '{backspace}', '12:34:00');
+    await testSelectAndTypeOnChange(
+      user,
+      8,
+      // First backspace clears the whole section
+      '{backspace}{backspace}{backspace}{backspace}',
+      '10:00:00'
+    );
+  });
+
+  it('updates the displayed value on blur', async () => {
+    const user = userEvent.setup();
+    const onChange = jest.fn();
+    const { unmount } = makeTimeInput({ onChange });
+    const input: HTMLInputElement = screen.getByRole('textbox');
+    input.focus();
+    await user.type(input, '{shift}{backspace}{backspace}', {
+      initialSelectionStart: 6,
+      initialSelectionEnd: 6,
+    });
+    expect(onChange).toBeCalledTimes(2);
+    expect(onChange).lastCalledWith(TimeUtils.parseTime('12:30:00'));
+    expect(input.value).toEqual('12:3');
+
+    input.blur();
+
+    // Blur should update the internal value to match the last onChange
+    // but not trigger another onChange
+    expect(onChange).toBeCalledTimes(2);
+
+    expect(input.value).toEqual('12:30:00');
+
+    // Fill in missing chars in the middle
+    input.focus();
+    await user.type(input, '{shift}{backspace}', {
+      skipClick: true,
+      initialSelectionStart: 3,
+      initialSelectionEnd: 3,
+    });
+    expect(input.value).toEqual(
+      `12:${FIXED_WIDTH_SPACE}${FIXED_WIDTH_SPACE}:00`
+    );
+    input.blur();
+    expect(input.value).toEqual('12:00:00');
+
+    unmount();
+  });
+
   it('existing edge cases', async () => {
     const user = userEvent.setup();
     // Ideally it should change the first section to 20, i.e. '20:34:56'
@@ -440,3 +518,37 @@ it('updates properly when the value prop is updated', () => {
 
   expect(textbox.value).toEqual('00:00:00');
 });
+
+it('ignores value prop changes matching displayed value', async () => {
+  const user = userEvent.setup();
+  const onChange = jest.fn();
+  const { rerender } = makeTimeInput({ value: 1, onChange });
+
+  const textbox: HTMLInputElement = screen.getByRole('textbox');
+  expect(textbox.value).toEqual('00:00:01');
+
+  textbox.focus();
+  await user.type(textbox, '{backspace}', {
+    skipClick: true,
+    initialSelectionStart: 8,
+    initialSelectionEnd: 8,
+  });
+
+  expect(textbox.value).toEqual('00:00:0');
+  expect(onChange).toBeCalledWith(0);
+
+  // Ignore prop update matching internal state
+  rerender(<TimeInput value={0} onChange={onChange} />);
+  expect(textbox.value).toEqual('00:00:0');
+  expect(onChange).toBeCalledTimes(1);
+
+  // Update internal value
+  rerender(<TimeInput value={1} onChange={onChange} />);
+  expect(textbox.value).toEqual('00:00:01');
+  expect(onChange).toBeCalledTimes(1);
+
+  // Update internal value
+  rerender(<TimeInput value={0} onChange={onChange} />);
+  expect(textbox.value).toEqual('00:00:00');
+  expect(onChange).toBeCalledTimes(1);
+});

packages/components/src/TimeInput.tsx

@@ -33,6 +33,16 @@ export type TimeInputElement = {
   setSelection: (newSelection: SelectionSegment) => void;
 };
 
+function fixIncompleteValue(value: string): string {
+  // If value is not a complete HH:mm:ss time, fill missing parts with 0
+  if (value != null) {
+    return `${value
+      .substring(0, 8)
+      .replace(/\u2007/g, '0')}${`00:00:00`.substring(value.length)}`;
+  }
+  return value;
+}
+
 // Forward ref causes a false positive for display-name in eslint:
 // https://github.com/yannickcr/eslint-plugin-react/issues/2269
 // eslint-disable-next-line react/display-name
@@ -49,6 +59,7 @@ const TimeInput = React.forwardRef<TimeInputElement, TimeInputProps>(
       'data-testid': dataTestId,
     } = props;
     const [value, setValue] = useState(TimeUtils.formatTime(propsValue));
+    const parsedValueRef = useRef<number>(propsValue);
     const [selection, setSelection] = useState<SelectionSegment>();
     const inputRef = useRef<HTMLInputElement>(null);
 
@@ -68,9 +79,14 @@ const TimeInput = React.forwardRef<TimeInputElement, TimeInputProps>(
 
     useEffect(
       function setFormattedTime() {
-        setValue(TimeUtils.formatTime(propsValue));
+        // Ignore value prop update if it matches the displayed value
+        // to preserve the displayed value while typing
+        if (parsedValueRef.current !== propsValue) {
+          setValue(TimeUtils.formatTime(propsValue));
+          parsedValueRef.current = propsValue;
+        }
       },
-      [propsValue]
+      [parsedValueRef, propsValue]
     );
 
     function getNextSegmentValue(
@@ -115,15 +131,27 @@ const TimeInput = React.forwardRef<TimeInputElement, TimeInputProps>(
       );
     }
 
-    function handleChange(newValue: string): void {
-      log.debug('handleChange', newValue);
-      setValue(newValue);
+    const handleChange = useCallback(
+      (newValue: string): void => {
+        log.debug('handleChange', newValue);
+        setValue(newValue);
+        parsedValueRef.current = TimeUtils.parseTime(
+          fixIncompleteValue(newValue)
+        );
+        onChange(parsedValueRef.current);
+      },
+      [onChange]
+    );
 
-      // Only send a change if the value is actually valid
-      if (TimeUtils.isTimeString(newValue)) {
-        onChange(TimeUtils.parseTime(newValue));
+    const handleBlur = useCallback((): void => {
+      const fixedValue = fixIncompleteValue(value);
+      // Update the value displayed in the input
+      // onChange with the fixed value already triggered in handleChange
+      if (fixedValue !== value) {
+        setValue(fixedValue);
       }
-    }
+      onBlur();
+    }, [value, onBlur]);
 
     const handleSelect = useCallback(
       (newSelection: SelectionSegment) => {
@@ -146,7 +174,7 @@ const TimeInput = React.forwardRef<TimeInputElement, TimeInputProps>(
         selection={selection}
         value={value}
         onFocus={onFocus}
-        onBlur={onBlur}
+        onBlur={handleBlur}
         data-testid={dataTestId}
       />
     );