fix(number-field): allow only numeric characters for Japanese/Chinese IME #4817
Conversation
Branch previewVisual regression test resultsWhen a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:
|
Lighthouse scores
What is this?Lighthouse scores comparing the documentation site built from the PR ("Branch") to that of the production documentation site ("Latest") and the build currently on Transfer Size
Request Count
|
Pull Request Test Coverage Report for Build 11728155483Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
Tachometer resultsChromeaction-menu permalinktest-basic
test-directive permalink
test-lazy permalink
test-open-close-directive permalink
test-open-close permalink
breadcrumbs permalinkbasic-test
combobox permalinkbasic-test
light-dom-test permalink
menu permalinktest-basic
number-field permalinkbasic-test
picker permalinkbasic-test
slider permalinktest-basic
Firefoxnumber-field permalinkbasic-test
slider permalinktest-basic
|
| @@ -496,6 +496,18 @@ export class NumberField extends TextfieldBase { | |||
|
|
|||
| protected override handleInput(event: InputEvent): void { | |||
| if (this.isComposing) { | |||
| // If user actually types a new character. | |||
| if (event.data) { | |||
There was a problem hiding this comment.
If event.data is null, it could lead to unexpected behavior. For example, pressing backspace or delete can cause event.data to be empty.An early return would be more tactical!
if (event.data === null || event.data === '') {
return;
}
There was a problem hiding this comment.
if (event.data) also guards for null and empty string, and after this this block is a return at line 511.
Wouldn't it work the same?
| // Don't allow non-numeric characters even in composing mode. | ||
| const partialValue = this.convertValueToNumber(event.data); | ||
|
|
||
| if (Number.isNaN(partialValue)) { |
There was a problem hiding this comment.
Does this allow pasting inputs?
| if (Number.isNaN(partialValue)) { | ||
| this.inputElement.value = this.indeterminate | ||
| ? indeterminatePlaceholder | ||
| : this._trackingValue; |
There was a problem hiding this comment.
Resetting the value to this._trackingValue can be confusing! Can we show some validation feedback instead of a reset?
There was a problem hiding this comment.
I followed the same logic as on english keyboard (line 547), we are resetting here because you should not be able to type non-numeric numbers, so we're removing what you typed.
There was a problem hiding this comment.
Makes Sense!
But in the storybook when the number-field has an initial value ='100', and user removes one zero with a backspace such that new value is now '10'. And now if user tries to type some non-numeric character in japanese, the values resets to '100' which should not happen.
There was a problem hiding this comment.
good catch, fixed 👍 can you try again?
|
@Rajdeepc @blunteshwar @rubencarvalho any more feedback here? |
… IME (#4817) * fix(number-field): allow only numeric characters for Japanese, Simplified/Traditional Chinese IME * chore: remove forgotten console log * chore: implement code review * chore: con't emit input event on invalid characters * chore: implement code review --------- Co-authored-by: rmanole <rmanole@adobe.com> Co-authored-by: Rúben Carvalho <rubcar@sapo.pt>
Description
Text candidates appear for characters by inputting them with Japanese/Simplified Chinese/Traditional Chinese IME for numbers in respective fields where only digits are allowed.
Related issue(s)
sp-number-fieldwhen using Japanese/Simplified Chinese/Traditional Chinese IME #4805Motivation and context
I think users should not be allowed to type "love" for example in an
sp-number-field, when using Japanese/Chinese IME.We do this check for english keyboard, but it seems it was missed to internationalised keyboards.
Internal tracking: CCEX-145560
How has this been tested?
Test case 1 Repeat for Safari/Chrome/Firefox
Did it pass in Desktop?
Did it pass in Mobile?
Did it pass in iPad?
Screenshots (if appropriate)
Types of changes
Checklist
Best practices
This repository uses conventional commit syntax for each commit message; note that the GitHub UI does not use this by default so be cautious when accepting suggested changes. Avoid the "Update branch" button on the pull request and opt instead for rebasing your branch against
main.