Skip to content
This repository has been archived by the owner on Feb 6, 2023. It is now read-only.

Normalize input data application from onBeforeInput and compositionUpdate #1419

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

octatone
Copy link

@octatone octatone commented Oct 4, 2017

Before submitting a pull request, please make sure the following is done...

  1. ✔️ Fork the repo and create your branch from master.
  2. ✔️ If you've added code that should be tested, add tests!
  3. If you've changed APIs, update the documentation.
  4. Ensure that:
  • ✔️ The test suite passes (npm test)
  • ✔️ Your code lints (npm run lint) and passes Flow (npm run flow)
  • You have followed the testing guidelines
  1. If you haven't already, complete the CLA.
    I should be listed as an authorized contributor via Microsoft.

Summary

Should resolve: #1079 and #1128 there are likely more related issues, but I cannot remember them all.

This pull request attempts to address several issues with text composition on mobile Android, IME behaviors, and browser API inconsistencies. It does not solve all custom Android keyboard issues, but it at least makes the experience with GBoard magnitudes better.

It does so by tracking all input event data, and applying data via the following strategy:

When beforeInput data is present, it is only preferred if its length
is greater than that of the last compositionUpdate event data. This is
meant to resolve IME incosistencies where compositionUpdate may contain
only the last character or the entire composition depending on language
(e.g. Korean vs. Japanese).

When beforeInput data is not present, compositionUpdate data is preferred.
This resolves issues with some platforms where beforeInput is never fired
(e.g. Android with certain keyboard and browser combinations).

Lastly, if neither beforeInput nor compositionUpdate events are fired,
use the data in the compositionEnd event

This PR continues from work here: #1285 and here: #1152 Their commits are preserved in this PR - not sure of the implications for CLA.

Test Plan

I have included unit tests to assert the above strategy, however this PR needs more manual testing to verify cross-plat improvements and to find more cases where this strategy does not improve the experience for the user.

Additionally, more work needs to be done (as suggested by @flarnie) to curate order of events for composition modes for the many browser + keyboard + platform combinations.

package.json Outdated
@@ -26,8 +26,8 @@
"postbuild": "node node_modules/fbjs-scripts/node/check-lib-requires.js lib",
"lint": "eslint .",
"flow": "flow src",
"test": "NODE_ENV=test jest",
"test-ci": "NODE_ENV=test npm run lint && npm run flow && npm run test"
"test": "cross-env NODE_ENV=test jest",
Copy link
Author

Choose a reason for hiding this comment

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

Added to enable running tests cross-plat (Windows)

@octatone
Copy link
Author

octatone commented Oct 9, 2017

@njjewers @mathieumg @flarnie

@tomkelsey
Copy link

Had this running on a project for a while and our Android users are infinitely more happy 👍

@njjewers
Copy link

Hey, Octatone, I just wanted to thank you for the work you've done. I'm glad you found my patch set useful as a base to continue working from. Here's hoping that this most recent patch makes it into master.

@mitermayer
Copy link
Contributor

mitermayer commented Nov 19, 2017

Hi @octatone ,

Could you rebase master into here, and update tests:

  • we are moving away from 'describe' / 'it' to root tests written with 'test'
  • we are now using jest snapshots for testing (if its an immutable data structure assertion please use .toJS() to create the snapshot)

Please let us know if you are still interested in pursuing this PR

@octatone
Copy link
Author

@mitermayer Yes, I am still interested in pursuing this PR. I will work on rebasing this ASAP.

@njjewers
Copy link

I've got a little more free time around now, if you want/need any help @octatone I'd be glad to be of assistance.

@octatone
Copy link
Author

@mitermayer Rebased against master, tests converted to root tests + snapshots.

@facebook-github-bot
Copy link

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@pvieira91
Copy link

Can't this be merged? It would be really helpful for anyone who needs this to be working in android

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

Successfully merging this pull request may close these issues.

Reapiting user insert android
7 participants