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): correct composition entry of multi-byte numbers #3610

Merged
merged 1 commit into from
Aug 30, 2023

Conversation

hunterloftis
Copy link
Contributor

@hunterloftis hunterloftis commented Aug 29, 2023

Description

Current implementation: Intercepts IME-based keystrokes and replaces them with their single-byte cousin.

This change: Accepts all keystrokes until the component is blurred (loses focus), then converts all IME-based characters into their single-byte cousins.

Related issue(s)

Motivation and context

Fix Number Field issues with certain Input Method Editor (IME) input types.

How has this been tested?

  • All existing tests pass
  • Manual testing of both IME and non-IME cases

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Chore (minor updates related to the tooling or maintenance of the repository, does not impact compiled assets)

Checklist

  • I have signed the Adobe Open Source CLA.
  • My code follows the code style of this project.
  • If my change required a change to the documentation, I have updated the documentation in this pull request.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have reviewed at the Accessibility Practices for this feature, see: Aria Practices

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.

@hunterloftis hunterloftis force-pushed the number-field-composition-merge branch from 33126f5 to f407dd9 Compare August 29, 2023 22:11
@github-actions
Copy link

github-actions bot commented Aug 29, 2023

Tachometer results

Chrome

number-field permalink

Version Bytes Avg Time vs remote vs branch
npm latest 634 kB 337.41ms - 343.33ms - unsure 🔍
-0% - +2%
-1.33ms - +7.90ms
branch 629 kB 333.55ms - 340.62ms unsure 🔍
-2% - +0%
-7.90ms - +1.33ms
-

search permalink

Version Bytes Avg Time vs remote vs branch
npm latest 475 kB 174.29ms - 177.94ms - unsure 🔍
-2% - +1%
-3.05ms - +2.31ms
branch 470 kB 174.52ms - 178.46ms unsure 🔍
-1% - +2%
-2.31ms - +3.05ms
-

slider permalink

Version Bytes Avg Time vs remote vs branch
npm latest 461 kB 243.64ms - 250.20ms - faster ✔
7% - 10%
17.54ms - 25.91ms
branch 456 kB 266.04ms - 271.25ms slower ❌
7% - 11%
17.54ms - 25.91ms
-

textfield permalink

Version Bytes Avg Time vs remote vs branch
npm latest 423 kB 103.98ms - 108.53ms - unsure 🔍
-1% - +4%
-1.51ms - +3.90ms
branch 418 kB 103.59ms - 106.52ms unsure 🔍
-4% - +1%
-3.90ms - +1.51ms
-
Firefox

number-field permalink

Version Bytes Avg Time vs remote vs branch
npm latest 634 kB 675.95ms - 711.13ms - unsure 🔍
-3% - +4%
-19.55ms - +27.51ms
branch 629 kB 673.94ms - 705.18ms unsure 🔍
-4% - +3%
-27.51ms - +19.55ms
-

search permalink

Version Bytes Avg Time vs remote vs branch
npm latest 475 kB 306.86ms - 326.42ms - unsure 🔍
-5% - +3%
-16.89ms - +10.45ms
branch 470 kB 310.30ms - 329.42ms unsure 🔍
-3% - +5%
-10.45ms - +16.89ms
-

slider permalink

Version Bytes Avg Time vs remote vs branch
npm latest 461 kB 449.18ms - 469.02ms - faster ✔
18% - 24%
103.87ms - 140.21ms
branch 456 kB 565.92ms - 596.36ms slower ❌
22% - 31%
103.87ms - 140.21ms
-

textfield permalink

Version Bytes Avg Time vs remote vs branch
npm latest 423 kB 212.27ms - 234.85ms - unsure 🔍
-10% - +5%
-23.19ms - +10.91ms
branch 418 kB 216.92ms - 242.48ms unsure 🔍
-5% - +10%
-10.91ms - +23.19ms
-

@Westbrook Westbrook force-pushed the number-field-composition-merge branch from f407dd9 to 2875fca Compare August 30, 2023 14:34
Copy link
Collaborator

@najikahalsema najikahalsema left a comment

Choose a reason for hiding this comment

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

いいね!お疲れ様でした 🙇 This seems like a good way to handle this issue.

protected handleCompositionEnd(): void {
this.isComposing = false;
requestAnimationFrame(() => {
this.inputElement.dispatchEvent(
Copy link
Contributor

Choose a reason for hiding this comment

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

I wish I remembered why I did this in the rAF(). It's the only part that seems clearly brittle/directly working to not step on the OS's toes, which means easy to break.

@Westbrook Westbrook changed the title fix: correct composition entry of multi-byte numbers fix(textfield): correct composition entry of multi-byte numbers Aug 30, 2023
@Westbrook Westbrook changed the title fix(textfield): correct composition entry of multi-byte numbers fix(number-field): correct composition entry of multi-byte numbers Aug 30, 2023
@Westbrook Westbrook merged commit 5e11934 into main Aug 30, 2023
@Westbrook Westbrook deleted the number-field-composition-merge branch August 30, 2023 20:23
blunteshwar pushed a commit that referenced this pull request Sep 12, 2023
@kahol
Copy link

kahol commented May 18, 2024

@hunterloftis @Westbrook @najikahalsema Can you please review the latest tests using this fix in https://jira.corp.adobe.com/browse/CCEX-47632 ? It sounds like the behavior is better (for the most part, there's no more double entry) but there is still incorrect behavior depending on browser, OS, and language.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants