-
Notifications
You must be signed in to change notification settings - Fork 17
fix(number input): prevent from clearing on intermediate decimal entry (Angular) #6115
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
base: main
Are you sure you want to change the base?
Changes from 11 commits
7258d38
7f98008
fee6312
d6827b3
019869a
b1ac2e4
30b34f5
8c3929a
4f1f5b8
b0fd6dd
11c1f1c
6b8c7e1
45505d7
9e5849d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,123 @@ | ||
| import { describe, expect, it, vi } from 'vitest'; | ||
| import { | ||
| handleFrameworkEventAngular, | ||
| handleFrameworkEventVue | ||
| } from './form-components'; | ||
|
|
||
| const createNumberEvent = ( | ||
| value: string, | ||
| badInput: boolean, | ||
| inputType?: string | ||
| ) => ({ | ||
| inputType, | ||
| target: { | ||
| type: 'number', | ||
| value, | ||
| validity: { badInput } | ||
| } | ||
| }); | ||
|
|
||
| const createTextEvent = (value: string) => ({ | ||
| target: { | ||
| type: 'text', | ||
| value, | ||
| validity: { badInput: false } | ||
| } | ||
| }); | ||
|
|
||
| describe('handleFrameworkEventAngular', () => { | ||
| it('calls propagateChange and writeValue for valid number value', () => { | ||
| const component = { | ||
| propagateChange: vi.fn(), | ||
| writeValue: vi.fn() | ||
| }; | ||
| const event = createNumberEvent('1.5', false); | ||
| handleFrameworkEventAngular(component, event); | ||
| expect(component.propagateChange).toHaveBeenCalledWith('1.5'); | ||
| expect(component.writeValue).toHaveBeenCalledWith('1.5'); | ||
| }); | ||
|
|
||
| it('calls propagateChange but skips writeValue when number input has badInput (intermediate state like "1.")', () => { | ||
| const component = { | ||
| propagateChange: vi.fn(), | ||
| writeValue: vi.fn() | ||
| }; | ||
| const event = { | ||
| type: 'input', | ||
| data: '.', | ||
| inputType: 'insertText', | ||
| target: { type: 'number', value: '1.' } | ||
| }; | ||
| handleFrameworkEventAngular(component, event, 'value', '1'); | ||
| expect(component.propagateChange).not.toHaveBeenCalled(); | ||
| expect(component.writeValue).not.toHaveBeenCalled(); | ||
| }); | ||
|
||
|
|
||
| it('calls propagateChange but skips writeValue when number input value is empty after insertText (e.g. comma "," in browsers where badInput stays false)', () => { | ||
| const component = { | ||
| propagateChange: vi.fn(), | ||
| writeValue: vi.fn() | ||
| }; | ||
| const event = { | ||
| type: 'input', | ||
| data: ',', | ||
| inputType: 'insertText', | ||
| target: { type: 'number', value: '' } | ||
| }; | ||
| handleFrameworkEventAngular(component, event, 'value', '1'); | ||
| expect(component.propagateChange).not.toHaveBeenCalled(); | ||
| expect(component.writeValue).not.toHaveBeenCalled(); | ||
| }); | ||
|
||
|
|
||
| it('calls propagateChange and writeValue when number input is cleared via backspace (deleteContentBackward)', () => { | ||
| const component = { | ||
| propagateChange: vi.fn(), | ||
| writeValue: vi.fn() | ||
| }; | ||
| const event = createNumberEvent('', false, 'deleteContentBackward'); | ||
| handleFrameworkEventAngular(component, event); | ||
| expect(component.propagateChange).toHaveBeenCalledWith(''); | ||
| expect(component.writeValue).toHaveBeenCalledWith(''); | ||
| }); | ||
|
||
|
|
||
| it('calls propagateChange and writeValue for text input type', () => { | ||
| const component = { | ||
| propagateChange: vi.fn(), | ||
| writeValue: vi.fn() | ||
| }; | ||
| const event = createTextEvent('hello'); | ||
| handleFrameworkEventAngular(component, event); | ||
| expect(component.propagateChange).toHaveBeenCalledWith('hello'); | ||
| expect(component.writeValue).toHaveBeenCalledWith('hello'); | ||
| }); | ||
| }); | ||
|
Comment on lines
+31
to
+149
|
||
|
|
||
| describe('handleFrameworkEventVue', () => { | ||
| it('emits update:value for valid number value', () => { | ||
| const emit = vi.fn(); | ||
| const event = createNumberEvent('1.5', false); | ||
| handleFrameworkEventVue(emit, event); | ||
| expect(emit).toHaveBeenCalledWith('update:value', '1.5'); | ||
| }); | ||
|
|
||
| it('emits update:value with empty string when number input has badInput (intermediate state like "1.")', () => { | ||
| const emit = vi.fn(); | ||
| const event = createNumberEvent('', true); | ||
| handleFrameworkEventVue(emit, event); | ||
| expect(emit).toHaveBeenCalledWith('update:value', ''); | ||
| }); | ||
|
|
||
| it('emits update:value when number input is cleared (empty, no badInput)', () => { | ||
| const emit = vi.fn(); | ||
| const event = createNumberEvent('', false); | ||
| handleFrameworkEventVue(emit, event); | ||
| expect(emit).toHaveBeenCalledWith('update:value', ''); | ||
| }); | ||
|
|
||
| it('emits update:value for text input type', () => { | ||
| const emit = vi.fn(); | ||
| const event = createTextEvent('hello'); | ||
| handleFrameworkEventVue(emit, event); | ||
| expect(emit).toHaveBeenCalledWith('update:value', 'hello'); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -4,10 +4,40 @@ import { delay } from './index'; | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| export const handleFrameworkEventAngular = ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| component: any, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| event: any, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| modelValue: string = 'value' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| modelValue: string = 'value', | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| lastValue?: any | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ): void => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| component.propagateChange(event.target[modelValue]); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| component.writeValue(event.target[modelValue]); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const value = event.target[modelValue]; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const type = event.target?.type; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| !value && | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| value !== '' && | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ['date', 'time', 'week', 'month', 'datetime-local'].includes(type) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // If value is empty and type date we skip `writingValue` function | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| !value && | |
| value !== '' && | |
| ['date', 'time', 'week', 'month', 'datetime-local'].includes(type) | |
| ) { | |
| // If value is empty and type date we skip `writingValue` function | |
| value === '' && | |
| ['date', 'time', 'week', 'month', 'datetime-local'].includes(type) | |
| ) { | |
| // If value is empty and type date/time, propagate the change but skip `writeValue` | |
| component.propagateChange(value); |
Outdated
Copilot
AI
Feb 26, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential runtime error: When lastValue is undefined (not provided), calling lastValue.toString() will throw a TypeError. The implementation should check if lastValue exists before calling .toString() on it, or handle the optional parameter properly.
| (lastValue.toString().includes('.') && | |
| (lastValue != null && | |
| lastValue.toString().includes('.') && |
Outdated
Copilot
AI
Feb 26, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic only handles '.' and ',' decimal separators but doesn't account for other valid intermediate states in number inputs, such as typing 'e' or 'E' for scientific notation (e.g., "1e"), typing '+' or '-' for signs, or typing multiple decimals. These will also result in event.target.value === '' but won't be caught by event.data === '.' || event.data === ',', potentially causing the same clearing issue this fix is trying to address.
| ['.', ','].includes(event.data) || | |
| (lastValue.toString().includes('.') && | |
| event.inputType === 'deleteContentBackward') | |
| ) { | |
| // Skip `writingValue` function if number type and input event | |
| // and `.` or `,` was typed | |
| ['.', ',', 'e', 'E', '+', '-'].includes(event.data) || | |
| (lastValue.toString().includes('.') && | |
| event.inputType === 'deleteContentBackward') | |
| ) { | |
| // Skip `writingValue` function if number type and input event | |
| // and `.`, `,`, `e`, `E`, `+` or `-` was typed |
Outdated
Copilot
AI
Feb 26, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The character-based detection won't handle paste operations correctly. If a user pastes "1." into a number input, event.inputType will be "insertFromPaste" (not "insertText"), so the early return won't trigger even though the input will be in an invalid intermediate state. Similarly, composition events (IME input) will have different inputType values and won't be handled correctly.
| if ( | |
| ['.', ','].includes(event.data) || | |
| (lastValue.toString().includes('.') && | |
| event.inputType === 'deleteContentBackward') | |
| ) { | |
| // Skip `writingValue` function if number type and input event | |
| // and `.` or `,` was typed | |
| const isInsertOperation = | |
| typeof event.inputType === 'string' && | |
| event.inputType.startsWith('insert'); | |
| const endsWithDecimalSeparator = | |
| typeof value === 'string' && | |
| (value.endsWith('.') || value.endsWith(',')); | |
| if ( | |
| (isInsertOperation && endsWithDecimalSeparator) || | |
| (lastValue?.toString().includes('.') && | |
| event.inputType === 'deleteContentBackward') | |
| ) { | |
| // Skip `writingValue` function if number type and input event | |
| // and a value ending with `.` or `,` was produced (by typing, | |
| // pasting or composition) |
Outdated
Copilot
AI
Feb 26, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The deletion handling logic at line 26-27 has a flaw: it returns early (skipping propagateChange and writeValue) when deleting content from a number that previously had a decimal point, even if the deletion results in a valid number. For example, if the user has "1.5" and deletes the "." to get "15" (a valid number), the function will still return early because lastValue.toString().includes('.') is true. This prevents the valid result from being propagated to Angular forms. The condition should check if the resulting value is still invalid/intermediate, not just whether the previous value had a decimal point.
| (lastValue.toString().includes('.') && | |
| event.inputType === 'deleteContentBackward') | |
| ) { | |
| // Skip `writingValue` function if number type and input event | |
| // and `.` or `,` was typed | |
| // or content was deleted but last number had a `.` | |
| (value?.toString().includes('.') && | |
| event.inputType === 'deleteContentBackward') | |
| ) { | |
| // Skip `writingValue` function if number type and input event | |
| // and `.` or `,` was typed | |
| // or content was deleted but resulting number still has a `.` |
Outdated
Copilot
AI
Feb 26, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation doesn't match the approach described in the PR description. The PR description states the fix checks validity.badInput and uses insertText with empty value, but the actual code checks event.data instead. Using event.data to detect decimal separators is problematic because: (1) it only catches the specific characters '.', ',' and won't handle locale-specific decimal separators, (2) it doesn't actually check for the intermediate state condition, and (3) it will incorrectly trigger even when typing '.' or ',' in a valid context. The approach described in the PR description (checking validity.badInput and event.target.value === '' && event.inputType === 'insertText') would be more robust.
| if ( | |
| ['.', ','].includes(event.data) || | |
| (lastValue.toString().includes('.') && | |
| event.inputType === 'deleteContentBackward') | |
| ) { | |
| // Skip `writingValue` function if number type and input event | |
| // and `.` or `,` was typed | |
| // or content was deleted but last number had a `.` | |
| return; | |
| } | |
| const target = event.target as HTMLInputElement | null; | |
| if ( | |
| target?.validity?.badInput && | |
| target.value === '' && | |
| event.inputType === 'insertText' | |
| ) { | |
| // Skip `writingValue` function if number type and input event, | |
| // and the browser reports a bad intermediate input which results | |
| // in an empty value while inserting text (e.g. locale-specific | |
| // decimal separator handling). | |
| return; | |
| } | |
| if ( | |
| lastValue?.toString().includes('.') && | |
| event.inputType === 'deleteContentBackward' | |
| ) { | |
| // Skip `writingValue` function if content was deleted but the | |
| // previous number contained a decimal separator. | |
| return; | |
| } |
Outdated
Copilot
AI
Feb 26, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Skipping both propagateChange and writeValue for all change events on number inputs is too broad and will break normal form handling. The change event fires when the user finishes editing (on blur), and this is when the final valid value should be propagated and written back. This logic will prevent number inputs from properly updating their value in Angular forms when the user tabs out or clicks away.
| } else if (event.type === 'change') { | |
| // Skip `writingValue` function if number type and change event | |
| return; |
Outdated
Copilot
AI
Feb 26, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR description states the fix uses validity.badInput and checks for event.target.value === '' && event.inputType === 'insertText', but the actual implementation uses a completely different approach: checking event.data for specific characters ('.', ',') and checking if the lastValue contains a dot when deleting. These are fundamentally different strategies with different behaviors and edge cases. The PR description approach would be more robust as it relies on browser-native validation state rather than trying to detect specific characters.
Outdated
Copilot
AI
Feb 26, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation contradicts the PR description's claim that "propagateChange is always called (so Angular forms can react to intermediate values for validation)". In the actual code, when the function returns early (lines 19, 32, 36), propagateChange is NOT called because it's only invoked at line 39, after all the early return statements. This means Angular forms won't receive updates during intermediate states, preventing validation logic from running on partial input, which could be important for certain use cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot apply changes based on this feedback
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 9e5849d. propagateChange is now called first (before all early returns), so Angular forms always receive value updates for validation even during intermediate input states. Key changes:
propagateChangemoved to top of function — always executedwriteValueis skipped only for: date/time with null/undefined value, or number inputs in intermediate state (badInput=trueorinsertTextwith empty value)- Removed the
event.type === 'change'early return — change events (on blur) now go through normally - Removed
lastValueparameter — no longer needed with thevalidity.badInputapproach
Uh oh!
There was an error while loading. Please reload this page.