Skip to content

BL-15962 Fix empty input and backspace handling in SmallNumberPicker #7734

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.

Sign up for GitHub

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
andrew-polk merged 1 commit into from
Mar 16, 2026
Merged

BL-15962 Fix empty input and backspace handling in SmallNumberPicker #7734

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion AGENTS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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`}>`
Expand Down
13 changes: 7 additions & 6 deletions src/BloomBrowserUI/pageChooser/selectedTemplatePageControls.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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(
Expand Down Expand Up @@ -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};
}
`}
Comment thread
andrew-polk marked this conversation as resolved.
l10nKey={buttonKey}
hasText={true}
Expand Down Expand Up @@ -295,6 +295,7 @@ export const SelectedTemplatePageControls: React.FunctionComponent<
minLimit={minimumPagesToAdd}
maxLimit={maximumPagesToAdd}
handleChange={setNumberToAdd}
onValidityChange={setIsNumberToAddValid}
tooltip={numberOfPagesTooltip}
/>
</div>
Expand Down
5 changes: 4 additions & 1 deletion src/BloomBrowserUI/react_components/numberChooserDialog.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}>
Expand All @@ -56,6 +58,7 @@ export const NumberChooserDialog: React.FunctionComponent<
minLimit={props.min}
maxLimit={props.max}
handleChange={setNumberChosen}
onValidityChange={setIsNumberPickerValid}
/>
</div>
</div>
Expand All @@ -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);
Expand Down
10 changes: 0 additions & 10 deletions src/BloomBrowserUI/react_components/smallNumberPicker.less
View file
Open in desktop

This file was deleted.

82 changes: 66 additions & 16 deletions src/BloomBrowserUI/react_components/smallNumberPicker.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
}
Comment on lines 40 to +46
Copy link
Copy Markdown

@devin-ai-integration devin-ai-integration Bot Mar 13, 2026

Choose a reason for hiding this comment

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

🔴 Keystroke-level validity.valid check blocks typing valid multi-digit numbers when minLimit > 1

The handleNumberChange handler rejects input whenever event.target.validity.valid is false (line 28). The HTML constraint validation API evaluates rangeUnderflow against min on every keystroke, not just on the final value. So when minLimit=2 (as in DuplicateManyDialog at src/BloomBrowserUI/bookEdit/duplicateManyDialog.tsx:23), typing "1" as the first digit produces validity.rangeUnderflow=true → validity.valid=false, causing the handler to return early and React to snap the input back. This makes it impossible for the user to type any number whose first digit is less than minLimit: with min=2, max=999, users cannot type 10–19 or 100–199. The storybook example (dialogs.stories.tsx:101) also uses min: 2, max: 777 and is similarly affected.

Suggested approach

Replace the validity.valid check with a more targeted check that only rejects truly bad input (e.g. validity.badInput for non-numeric characters), and defer the min/max range validation to the handleBlur handler. This way intermediate typing states like "1" (on the way to "15") are allowed, and out-of-range values are corrected when the user finishes editing.

Prompt for agents 
In src/BloomBrowserUI/react_components/smallNumberPicker.tsx, the handleNumberChange function (lines 22-42) uses event.target.validity.valid to reject input on every keystroke. This causes rangeUnderflow/rangeOverflow to fire on intermediate typing states (e.g. typing '1' when min=2, on the way to typing '15'). Fix this by:

1. In handleNumberChange (around line 27-32), replace the !event.target.validity.valid check with event.target.validity.badInput, which only fires for truly non-numeric input. Keep the 'e' check. Remove the reliance on min/max validity at the keystroke level.

2. In handleBlur (around line 46-50), add range validation: if the input value is non-empty, parse it and check if it's within [minLimit, maxLimit]. If it's out of range or NaN, restore lastValidValue and do NOT call props.handleChange. This ensures the final committed value is always in range.

3. Also in handleNumberChange (around line 40-41), before calling setLastValidValue and props.handleChange, add a range check: only update if newNum >= (props.minLimit ?? 1) && newNum <= props.maxLimit. This prevents out-of-range values from being propagated to the parent while the user is still typing, but doesn't block the keystrokes from appearing in the display.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.


Comment on lines +45 to +47
Copy link
Copy Markdown

@devin-ai-integration devin-ai-integration Bot Mar 13, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

📝 Info: The newString === "e" guard is dead code for type="number" inputs

On line 32, the check newString === "e" will never be true. For type="number" inputs in Chromium-based browsers (WebView2 112), when the user types just "e", event.target.value is always "" (empty string), not "e". The input is actually handled correctly by the subsequent if (newString === "") check on line 37-39, which returns early. So there's no functional bug, but this guard is misleading about what it protects against.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

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 (
Comment thread
nabalone marked this conversation as resolved.
<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>
Expand Down
4 changes: 4 additions & 0 deletions src/BloomBrowserUI/react_components/stories/misc.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -78,6 +81,7 @@ export const SmallNumberPickerStory: Story = {
minLimit={min}
maxLimit={max}
handleChange={onHandleChange}
onValidityChange={onValidityChange}
tooltip={numberOfPagesTooltip}
/>
</div>
Expand Down