Skip to content
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

fix(number-field): update IME change detection #4672

Merged
merged 4 commits into from
Aug 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
49 changes: 36 additions & 13 deletions packages/number-field/src/NumberField.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,17 @@ export const remapMultiByteCharacters: Record<string, string> = {
'%': '%',
'+': '+',
ー: '-',
一: '1',
二: '2',
三: '3',
四: '4',
五: '5',
六: '6',
七: '7',
八: '8',
九: '9',
零: '0',
};

const chevronIcon: Record<string, (dir: 'Down' | 'Up') => TemplateResult> = {
s: (dir) => html`
<sp-icon-chevron50
Expand Down Expand Up @@ -175,20 +184,25 @@ export class NumberField extends TextfieldBase {
private _trackingValue = '';
private lastCommitedValue?: number;

private setValue(value: number = this.value): void {
this.value = value;
private setValue(newValue: number = this.value): void {
// Capture previous value for accurate IME change detection
Copy link
Collaborator Author

@rubencarvalho rubencarvalho Aug 19, 2024

Choose a reason for hiding this comment

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

For IME input in Safari, the value conversion process would cause this.value and this.lastCommitedValue to be identical at the time of the comparison, even though a change had happened. So I added previousValue to hold the value of this.lastCommitedValue before the update to this.value, which fixed it! :)

const previousValue = this.lastCommitedValue;

this.value = newValue;

if (
typeof this.lastCommitedValue === 'undefined' ||
this.lastCommitedValue === this.value
typeof previousValue === 'undefined' ||
previousValue === this.value
) {
// Do not announce when the value is unchanged.
return;
}

this.lastCommitedValue = this.value;

this.dispatchEvent(
new Event('change', { bubbles: true, composed: true })
);
this.lastCommitedValue = this.value;
}

/**
Expand All @@ -214,7 +228,13 @@ export class NumberField extends TextfieldBase {
private valueBeforeFocus: string = '';
private isIntentDecimal: boolean = false;

private convertValueToNumber(value: string): number {
private convertValueToNumber(inputValue: string): number {
// Normalize full-width characters to their ASCII equivalents
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Without normalizing this value (remapping the multi-byte characters) before the input manipulation, we would get NaN.

let normalizedValue = inputValue
.split('')
.map((char) => remapMultiByteCharacters[char] || char)
.join('');

const separators = this.valueBeforeFocus
.split('')
.filter((char) => this.decimalsChars.has(char));
Expand All @@ -223,7 +243,7 @@ export class NumberField extends TextfieldBase {
if (
isIPhone() &&
this.inputElement.inputMode === 'decimal' &&
value !== this.valueBeforeFocus
normalizedValue !== this.valueBeforeFocus
) {
const parts = this.numberFormatter.formatToParts(1000.1);

Expand All @@ -234,12 +254,15 @@ export class NumberField extends TextfieldBase {
for (const separator of uniqueSeparators) {
const isDecimalSeparator = separator === replacementDecimal;
if (!isDecimalSeparator && !this.isIntentDecimal) {
value = value.replace(new RegExp(separator, 'g'), '');
normalizedValue = normalizedValue.replace(
new RegExp(separator, 'g'),
''
);
}
}

let hasReplacedDecimal = false;
const valueChars = value.split('');
const valueChars = normalizedValue.split('');
for (let index = valueChars.length - 1; index >= 0; index--) {
const char = valueChars[index];
if (this.decimalsChars.has(char)) {
Expand All @@ -249,11 +272,10 @@ export class NumberField extends TextfieldBase {
} else valueChars[index] = '';
}
}
value = valueChars.join('');
normalizedValue = valueChars.join('');
}
return this.numberParser.parse(value);
return this.numberParser.parse(normalizedValue);
}

private get _step(): number {
if (typeof this.step !== 'undefined') {
return this.step;
Expand Down Expand Up @@ -492,6 +514,7 @@ export class NumberField extends TextfieldBase {
.split('')
.map((char) => remapMultiByteCharacters[char] || char)
.join('');

if (this.numberParser.isValidPartialNumber(value)) {
// Use starting value as this.value is the `input` value.
this.lastCommitedValue = this.lastCommitedValue ?? this.value;
Expand Down
12 changes: 12 additions & 0 deletions packages/number-field/test/number-field.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -502,6 +502,18 @@ describe('NumberField', () => {
el.value = 52;
expect(changeSpy.callCount).to.equal(0);
});
it('handles IME input correctly and dispatches change event', async () => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This test covers both the NaN case and the change event.

el.focus();
el.dispatchEvent(new CompositionEvent('compositionstart'));
// input multibyte characters
await sendKeys({ type: '123' });
await elementUpdated(el);
el.dispatchEvent(new CompositionEvent('compositionend'));
await elementUpdated(el);
await sendKeys({ press: 'Enter' });
expect(el.value).to.equal(50123);
expect(changeSpy.callCount).to.equal(1);
});
it('via scroll', async () => {
el.focus();
await elementUpdated(el);
Expand Down
Loading