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(ui): IME composition detection not working #17476

Merged
merged 1 commit into from
Sep 3, 2024

Conversation

NitroRCr
Copy link
Contributor

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Documentation
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change?

  • Yes
  • No

The PR fulfills these requirements:

  • It's submitted to the dev branch (or v[X] branch)
  • When resolving a specific issue, it's referenced in the PR's title (e.g. fix: #xxx[,#xxx], where "xxx" is the issue number)
  • It's been tested on a Cordova (iOS, Android) app
  • It's been tested on an Electron app
  • Any necessary documentation has been added or updated in the docs or explained in the PR's description.

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature (to avoid wasting your time, it's best to start a new feature discussion first and wait for approval before working on it)

Other information:

Fix the issue where QInput & QSelect updates model-value during the IME composition process.

Previously, the code detected e.data and set qComposing to true only when a CJK character was detected. This might work in some IMEs, but it at least doesn't work in my environment (Windows' default Microsoft Pinyin IME), as during composition, e.data contains the original key characters instead of CJK characters:
image

To fix this, simply use compositionstart and compositionend, just like the official Vue v-model implementation.

After making the modification, I tested it, and it works well.

@rstoenescu rstoenescu merged commit 39fe251 into quasarframework:dev Sep 3, 2024
@rstoenescu
Copy link
Member

Thank you for your awesome contribution!
This will go into Quasar v2.16.10

@jtilford
Copy link

I'm having a problem with v2.16.10 that I'm not having with v2.16.9, related to a QInput field.

On Android (8.1.0 tested) with the software on-screen keyboard, the v-model for a QInput doesn't update when characters (English) are entered, but DOES seem to update on enter, space, or backspace. After the first update, "enter" seems to not update the model anymore.

The characters all show in the input. It's the code watching the v-model that doesn't get the new characters until those noted keys (at least) are pressed.

Use-case: We have search on our app where users type the characters of a product they want to see, and until v2.16.10, every character input would update the search.

On web builds, this problem is not present -- we can search on web and every character input to the QInput updates the v-model value live.

@NitroRCr
Copy link
Contributor Author

It seems that this issue only occurs on a few on-screen keyboards. I was able to reproduce the problem on an AOSP on-screen keyboard on Android 7, but the same situation also appears in the official Vue v-model implementation (tested here).

I observed that although it's English characters that are being typed, the characters in the input field have an underline, indicating that it is in IME composition mode. At this point, the model-value does not update, and I don't think this is a problem.

Perhaps it should be considered to add input event to QInput in order to handle input events during the composition process. As long as there is composition detection, input and update:model-value are different events, and I believe both are necessary.

@jtilford
Copy link

I'm a little unclear on your suggestion's impact. Would I need to change my inputs across my app, after Quasar 2.16.9, to use something else instead of v-model? Or can we restore the previous functionality of the inputs, and make IME composition detection some kind of boolean parameter or a separate input type?

@NitroRCr
Copy link
Contributor Author

NitroRCr commented Oct 18, 2024

I'm a little unclear on your suggestion's impact. Would I need to change my inputs across my app, after Quasar 2.16.9, to use something else instead of v-model? Or can we restore the previous functionality of the inputs, and make IME composition detection some kind of boolean parameter or a separate input type?

I was suggesting the developers of Quasar to add input event to QInput. This way, it would be possible to handle input during the IME composition process using :model-value="model" and @input="model = $event". Later, I made these modifications myself and created PR #17543. Unfortunately, that PR has not received a response.

@jtilford
Copy link

jtilford commented Oct 18, 2024

I was suggesting the developers of Quasar to add input event to QInput. This way, it would be possible to handle input during the IME composition process using :model-value="model" and @input="model = $event". Later, I made these modifications myself and created PR #17543. Unfortunately, that PR has not received a response.

I appreciate your quick reply. Looking at your PR, I think I can answer my own question.

The PR would restore the prior (<=2.16.9) functionality to @update:model-value, while adding @input to allow IME composition detection (like @update:model-value has been working in 2.16.10 and up).

Is that correct?

@NitroRCr
Copy link
Contributor Author

I appreciate your quick reply. Looking at your PR, I think I can answer my own question.

The PR would restore the prior (<=2.16.9) functionality to @update:model-value, while adding @input to allow IME composition detection (like @update:model-value has been working in 2.16.10 and up).

Is that correct?

I'm afraid not. IME composition detection means model-value won't update during IME composition process until composition is complete. The PR wouldn't restore the functionality to @update:model-value, but would add @input, which is also emitted during the IME composition process. This behavior is consistent with using @update:model-value or @input dirctly on native input element. In your use case you need to use @input instead of v-model if that PR is merged.

rstoenescu added a commit that referenced this pull request Oct 19, 2024
@rstoenescu
Copy link
Member

Hi All,

I am temporarily reverting this PR in v2.17.1 because it causes even more trouble (see #17536). I am open to suggestions and a new PR.

@jtilford
Copy link

jtilford commented Oct 19, 2024

Hi All,

I am temporarily reverting this PR in v2.17.1 because it causes even more trouble (see #17536). I am open to suggestions and a new PR.

Thanks so much, @rstoenescu!

I super appreciate your time on this. I wrote out a whole reply and realized I may have misinterpreted your message. It looks like you're reverting #17476, but for a moment I thought you were discarding #17543, when referencing "this PR" in your message.

Do you think it makes sense to add some kind of parameter, like :ime-composition-detection="true", to switch on the change? At least, in that case, the feature would work, and we could document the known cases where it causes problems with the inputs, such as on Android 7 and 8.

Again, thank you for looking at this.

Edit to add: I'm still pretty green when it comes to discussions on GitHub and using PRs in general, so I appreciate your understanding. I work on a team that pretty much develops on trunk without PRs, and I haven't had many opportunities to join conversations on FOSS projects.

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.

3 participants