BL-15962 Fix empty input and backspace handling in SmallNumberPicker

Author: nabalone

AGENTS.md

@@ -8,7 +8,7 @@ The front-end uses yarn 1.22.22. Never ever use npm.
 - do not define a props data type unless it is huge
 - example: export const SomeComponent: React.FunctionComponent<{initiallySelectedGroupIndex: number;> = (props) => {...}
 
-- Avoid removing existing comments.
+- Avoid removing existing comments unless your changes make them inaccurate/obsolete
 - Avoid adding a comment like "// add this line".
 
 - Style elements using the css macro from @emotion/react directly on the element being styled, using the css prop. E.g. `<div css={css`color:red`}>`

src/BloomBrowserUI/pageChooser/selectedTemplatePageControls.tsx

@@ -57,6 +57,8 @@ export const SelectedTemplatePageControls: React.FunctionComponent<
     const minimumPagesToAdd = 1;
     const maximumPagesToAdd = 99;
     const [numberToAdd, setNumberToAdd] = useState<number>(minimumPagesToAdd);
+    // Number picker snaps back to a valid value on blur if the user clicks away from the input with an invalid value, so isNumberToAddValid will usually be true
+    const [isNumberToAddValid, setIsNumberToAddValid] = useState(true);
 
     const captionKey = "TemplateBooks.PageLabel." + props.caption;
     const descriptionKey = "TemplateBooks.PageDescription." + props.caption;
@@ -73,7 +75,10 @@ export const SelectedTemplatePageControls: React.FunctionComponent<
     };
 
     const isAddOrChoosePageButtonEnabled = (): boolean => {
-        return !props.forChangeLayout || !props.willLoseData || continueChecked;
+        const canProceedWithLayoutChoice =
+            !props.forChangeLayout || !props.willLoseData || continueChecked;
+        const hasValidAddCount = !!props.forChangeLayout || isNumberToAddValid;
+        return canProceedWithLayoutChoice && hasValidAddCount;
     };
 
     const numberOfPagesTooltip = useL10n(
@@ -254,11 +259,6 @@ export const SelectedTemplatePageControls: React.FunctionComponent<
                                 width: 200px;
                                 font-weight: bold !important;
                                 font-size: 10pt !important;
-
-                                &:disabled {
-                                    color: ${kBloomBuff};
-                                    border: medium solid ${kBloomBuff};
-                                }
                             `}
                             l10nKey={buttonKey}
                             hasText={true}
@@ -295,6 +295,7 @@ export const SelectedTemplatePageControls: React.FunctionComponent<
                                     minLimit={minimumPagesToAdd}
                                     maxLimit={maximumPagesToAdd}
                                     handleChange={setNumberToAdd}
+                                    onValidityChange={setIsNumberToAddValid}
                                     tooltip={numberOfPagesTooltip}
                                 />
                             </div>

src/BloomBrowserUI/react_components/numberChooserDialog.tsx

@@ -33,6 +33,8 @@ export const NumberChooserDialog: React.FunctionComponent<
         useSetupBloomDialog(props.dialogEnvironment);
 
     const [numberChosen, setNumberChosen] = useState(props.min);
+    // The number picker snaps back to a valid value on blur if the user clicks away from the input with an invalid value, so isNumberPickerValid will usually be true
+    const [isNumberPickerValid, setIsNumberPickerValid] = useState(true);
 
     return (
         <BloomDialog {...propsForBloomDialog}>
@@ -56,6 +58,7 @@ export const NumberChooserDialog: React.FunctionComponent<
                             minLimit={props.min}
                             maxLimit={props.max}
                             handleChange={setNumberChosen}
+                            onValidityChange={setIsNumberPickerValid}
                         />
                     </div>
                 </div>
@@ -64,7 +67,7 @@ export const NumberChooserDialog: React.FunctionComponent<
                 <BloomButton
                     l10nKey="Common.OK"
                     hasText={true}
-                    enabled={true}
+                    enabled={isNumberPickerValid}
                     variant={"contained"}
                     onClick={() => {
                         props.onClick(numberChosen);

src/BloomBrowserUI/react_components/smallNumberPicker.less

@@ -1,48 +1,98 @@
+import { css } from "@emotion/react";
 import * as React from "react";
 import { useState } from "react";
 import TextField from "@mui/material/TextField";
 import ReactToolTip from "react-tooltip";
-import "./smallNumberPicker.less";
 
 export interface INumberChooserProps {
     maxLimit: number; // a valid result cannot be greater than this
     minLimit?: number; // a valid result cannot be less than this
     handleChange: (newNumber: number) => void;
+    onValidityChange?: (isValid: boolean) => void; // Notifies parent about validity changes
     tooltip?: string; // caller should localize
 }
 
+/**
+ * A React component for selecting nonnegative integers within a specified range.
+ * Ensures that the input adheres to the constraints and provides immediate feedback for invalid input.
+ */
 export const SmallNumberPicker: React.FunctionComponent<INumberChooserProps> = (
     props: INumberChooserProps,
 ) => {
-    const initialValue = props.minLimit === undefined ? 1 : props.minLimit;
-    const [chosenNumber, setChosenNumber] = useState(initialValue);
+    const minimumValue = props.minLimit ?? 0;
+    const initialValue = minimumValue;
+    const [displayValue, setDisplayValue] = useState(initialValue.toString());
+    const [lastValidValue, setLastValidValue] = useState(initialValue);
+
+    // We have the input allow empty string so that the user can clear the input before entering a new number
+    // but don't persist or submit it
+    function isValid(input: HTMLInputElement): boolean {
+        return input.validity.valid && input.value !== "";
+    }
 
     const handleNumberChange = (event: React.ChangeEvent<HTMLInputElement>) => {
         const newString = event.target.value;
-        const newNum = parseInt(newString);
+        const newNum = event.target.valueAsNumber;
+
+        // Don't allow typing in invalid characters; immediately snap back
+        // Except we don't prevent underflow immediately, so e.g. users can type digits "10" when the minimum is 2.
+        // Number inputs allow e for exponential notation but for a small number picker it only makes behavior more confusing
         if (
-            !newNum ||
-            newNum > props.maxLimit ||
-            (props.minLimit && newNum < props.minLimit)
+            event.target.validity.badInput ||
+            event.target.validity.rangeOverflow ||
+            newString.toLowerCase().includes("e")
         ) {
-            setChosenNumber(initialValue);
-            props.handleChange(initialValue);
-        } else {
-            setChosenNumber(newNum);
+            return;
+        }
+
+        setDisplayValue(newString);
+
+        const valid = isValid(event.target);
+        props.onValidityChange?.(valid);
+
+        if (valid) {
+            setLastValidValue(newNum);
             props.handleChange(newNum);
         }
     };
 
-    // We would love to set the TextField "type" to "number", but this introduces up/down arrows that we
-    // can't get rid of in Firefox and have the input still perform as a number input. This means we have to
-    // use a "text" style input and handle max and min and letter input in code. Any invalid input sets the
-    // input value back to the 'minLimit'.
+    // If the user clicks away with the input empty or invalid, restore the last valid value
+    const handleBlur = (event: React.FocusEvent<HTMLInputElement>) => {
+        const input = event.target;
+        if (!isValid(input)) {
+            setDisplayValue(lastValidValue.toString());
+            props.onValidityChange?.(true);
+        }
+    };
+
     return (
         <div className="smallNumberPicker">
             <div data-tip={props.tooltip}>
                 <TextField
+                    css={css`
+                        /* Don't display the little up/down arrows for number input */
+                        input[type="number"] {
+                            -moz-appearance: textfield;
+                        }
+                        input[type="number"]::-webkit-outer-spin-button,
+                        input[type="number"]::-webkit-inner-spin-button {
+                            -webkit-appearance: none;
+                            margin: 0;
+                        }
+
+                        input[type="number"] {
+                            text-align: right;
+                        }
+                    `}
+                    onBlur={handleBlur}
                     onChange={handleNumberChange}
-                    value={chosenNumber}
+                    value={displayValue}
+                    type="number"
+                    inputProps={{
+                        min: minimumValue,
+                        max: props.maxLimit,
+                        step: 1,
+                    }}
                     variant="standard"
                 />
             </div>

src/BloomBrowserUI/react_components/smallNumberPicker.tsx

@@ -56,6 +56,9 @@ export const SmallNumberPickerStory: Story = {
             console.log("We handled change!");
             console.log(`  result was ${newNumber}`);
         };
+        const onValidityChange = (isValid: boolean) => {
+            console.log(`Input validity changed: ${isValid}`);
+        };
         const min = 1;
         const max = 15;
 
@@ -78,6 +81,7 @@ export const SmallNumberPickerStory: Story = {
                                 minLimit={min}
                                 maxLimit={max}
                                 handleChange={onHandleChange}
+                                onValidityChange={onValidityChange}
                                 tooltip={numberOfPagesTooltip}
                             />
                         </div>