fix(web): clears repeating bksp on keyboard reload ⬅️

[jahorton]

Fixes #6735.

May also fix #1930, but I have been unable to repro it on (emulated) Android using #6735's repro.

---

Turns out a very natural assumption for us to make actually isn't valid - for iOS, at least, it is possible for the OSK itself to be reloaded during a backspace event. (I haven't been able to succesfully reproduce this with emulated Android yet.) With how KMW is structured, a reload results in the OSK being rebuilt from the ground up.

When this occurs - a backspace at the same time as an OSK reload, with the old OSK's final event being a 'touchstart' on K_BKSP - we get a permanently-sticky backspace key, as the new OSK has no reference for the repeating K_BKSP's timer.

Note that we'll want a more comprehensive rework to prevent the event that's causing the full OSK reload, as that could easily interfere with a multitap, something we're considering adding in 16.0. That, or prevent the full reload itself. Lots of 15.0 work went into things that should facilitate the latter idea, fortunately... but it's still for another PR, as we need this fix for 15.0 stable.

User Testing

Testing environments

Just in case, be sure that your test device's settings do not suppress device rotation events. There is a chance that such a setting may interfere with the repros.

GROUP_IOS: Using the Keyman app on any real iOS device...
GROUP_ANDROID: Using the Keyman app on any real Android device...

Tests

Note that for both tests that follow, step 2 represents our best-known reproduction strategies for issues #1930 and #6735.

TEST_PLAIN_STICKY: Attempt to reproduce the sticky backspace issue and FAIL this test if successful.

1. Type enough text for word-wrapping effects to take effect at least once.
2. Then, mash the backspace key as quickly as you can.
   • If using emulation/simulation, try mashing both the left and right mouse buttons.
3. Before half the text is erased, stop mashing the backspace key.
4. If all text is erased, try typing text. If newly-typed text is immediately erased, FAIL this test.
5. Repeat steps 1-4 at least two additional times.

TEST_ROTATING_STICKY: Attempt to reproduce the sticky backspace issue and FAIL this test if successful.

1. Type enough text for word-wrapping effects to take effect at least once.
2. Then, mash the backspace key as quickly as you can while rotating the device back and forth.
3. Before half the text is erased, stop mashing the backspace key and stop rotating the device.
4. If all text is erased, try typing text. If newly-typed text is immediately erased, FAIL this test.
5. Repeat steps 1-4 at least two additional times.

[keymanapp-test-bot]

User Test Results

Test specification and instructions

• ✅ GROUP_IOS: Using the Keyman app on any real iOS device...

  2 tests PASSED

  • ✅ TEST_PLAIN_STICKY (PASSED): the delete doesn't get stuck and keep delete all the way
  • ✅ TEST_ROTATING_STICKY (PASSED): the delete doesn't get stuck and keep delete all the way while rotating

• ✅ GROUP_ANDROID: Using the Keyman app on any real Android device...

  2 tests PASSED

  • ✅ TEST_PLAIN_STICKY (PASSED): Tested in the Android 11.0 Mobile device and I did not see any issue (ie., newly typed text is immediately erased) during my testing. Seems to be working fine.
  • ✅ TEST_ROTATING_STICKY (PASSED): Tested in the Android 11.0 Mobile device and it seems that it is working as expected.

Test Artifacts

• Android
  • Keyman for Android apk
  • FirstVoices Keyboards for Android apk
  • KMSample1 apk
  • KMSample2 apk
• iOS
  • Keyman for iOS (simulator image)
  • FirstVoices Keyboards for iOS (simulator image)
  • TestFlight internal PR build version - 16.0.13 (0.6786.6427)
• Web
  • KeymanWeb Test Home
• Windows
  • Keyman for Windows
  • FirstVoices Keyboards for Windows

[mcdurdin]

Note that we'll want a more comprehensive rework to prevent the event that's causing the full OSK reload, as that could easily interfere with a multitap, something we're considering adding in 16.0. That, or prevent the full reload itself.

It's puzzling to me why we would ever need a reload except for switching keyboards?

[mcdurdin]

Changes LGTM -- root cause[s] aside, this is essential mopping up that should be done in any case on shutdown.

[mcdurdin]

Once this passes test, it should be cherry-picked to stable-15.0.

Fixing the underlying reload requirement may well address other glitches apart from the backspace stickiness, such as performance woes, sticky key previews, etc.

[jahorton]

Note that we'll want a more comprehensive rework to prevent the event that's causing the full OSK reload, as that could easily interfere with a multitap, something we're considering adding in 16.0. That, or prevent the full reload itself.

It's puzzling to me why we would ever need a reload except for switching keyboards?

As far as I know, KeymanWeb has always taken a rather lazy approach to mobile device rotation. Rotate => reload the OSK. That's where all the computation logic was, anyway. I did a lot of work in 15.0 to allow us to be less lazy and eventually use the newer .refreshLayout() functions instead, but I don't think I ever got them fully ready for the task.

[jahorton]

But, continuing from that chain of thought... I figured "why not?" and investigated how much effort it'd take to change this.

Probably a bit less stable than desired for stable-15.0, but... #6787 would suggest it may not take much effort.

[mcdurdin]

Probably a bit less stable than desired for stable-15.0, but... #6787 would suggest it may not take much effort.

Can you explain for me why it would be a bit less stable than desired? I'm not following ;-)

[jahorton]

Probably a bit less stable than desired for stable-15.0, but... #6787 would suggest it may not take much effort.

Can you explain for me why it would be a bit less stable than desired? I'm not following ;-)

... because it involves somewhat significant internal/structural changes, compated to this one?

We know our keyboard construction is pretty much rock-solid, so using that to construct a rotated keyboard is very stable - KMW's been doing this since, at minimum, version 10. There were a couple of times a little targeted maintenance was needed, but that's it.

This approach throws that approach out in favor of something that's only now possible due to all the big OSK refactoring work during 15.0. It's handled some small-scale resizing while live, but nothing as pronounced or significant as full-on rotation handling before that PR. It'll be a fairly bad look if this ends up breaking keyboard rotation in certain use cases we haven't foreseen. Sure, it should handle it fine, but we haven't regression-tested with its changes in place... and we still have a few other regression test rounds in the pipeline. We're a bit bottlenecked there at the moment, aren't we?

[mcdurdin]

... because it involves somewhat significant internal/structural changes, compated to this one?

OK 😄 Gotcha!

One of the process changes you might have missed when you were away is that we are trying to raise the bar on alpha PRs -- we want to be confident that they are solid when merged. So if you are not 100% confident that this is not going to break things, then we need to be doing testing before merge to catch the issues. That's the job of the test team, and unit tests.

I'll try and articulate, hopefully somewhat clearly.

During release retrospective, we identified that we had a lot of issues in the beta cycle that we should really have picked up earlier in the release cycle. This led me to conclude:

1. Time-separation between when we make the changes and when we thoroughly test (during beta) makes it much harder to trace which PR is the root cause.
2. Fixes take longer, because we've all context-switched in the mean time.
3. If original PR author is not available during beta, then it's much harder to fix!
4. We end up with more noise to triage - more crash reports, more bug reports.

All of these hamper us in our chase for a shorter release cycle. So, as the team has grown (yes, we are in a pinch this week ... but not generally), and we have more capacity to raise the quality bar, I think we can afford to slow down a little on our PRs and make sure they are really good.

This is all subjective to a degree of course. For example, I won't merge the 🎢 PR chain until all the end-user-impacting issues are tested and sorted. But I may merge it before 100% of the code layout impurities are addressed -- because this is a net improvement without additional regressions.

In terms of prioritisation in PRs, for end-user facing UX:

1. Build breaking should be prioritised over everything else (it blocks the entire team).
2. Regressions should always be fixed before merge to 'release branches' (master/beta/stable-x.y): we never want a PR that regresses existing behaviour.
3. New features should be complete before merge to release branches: we develop them in a PR chain if they are big. AKA 'feature branches'. We can even protect the topmost branch in the feature branch in GitHub to help remind us that child PRs should be clean before merge.
4. Fixes for regressions identified post-merge should be prioritised over new development or new bug fixes (somewhat subject to additional; triage).
5. Long-lived branches should be rebased or have their base merged in prior to final testing, to ensure we haven't introduced any conflicts.

Now that we have solid test and deploy environments for all platforms in all PR builds, I think this is achievable. We no longer need to merge to alpha in order to actually use Keyman.

As always, these priorities are subject to triage and negotiation -- these are guidelines, but they are solid ones that we will follow as far as we can. Flexibility still needs to stay a part of our process!

And yeah, I don't think any of this is rocket science, but more a reflection of the fact that as the Keyman codebase is maturing and the team is growing, we will continue to identify places where we can improve our processes, and it's exciting to be able to do so.

Finally, if you see places where my code changes don't meet this bar, please do let me know (kindly!). I'm sure I'm not always doing a great job of keeping up with this (or even articulating it clearly), particularly in my current situation.

(Update: copied this text to our wiki at https://github.com/keymanapp/keyman/wiki/Principles-of-Keyman-Code-Changes)

[bharanidharanj]

GROUP_ANDROID: Using the Keyman app on any real Android device...

• TEST_PLAIN_STICKY (PASSED): Tested in the Android 11.0 Mobile device and I did not see any issue (ie., newly typed text is immediately erased) during my testing. Seems to be working fine.
• TEST_ROTATING_STICKY (PASSED): Tested in the Android 11.0 Mobile device and it seems that it is working as expected.

[MakaraSok]

Test Results

Tested on iPod touch 7th gen running iOS 15.4.1.

GROUP_IOS: Using the Keyman app on any real iOS device...

• TEST_PLAIN_STICKY (PASSED): the delete doesn't get stuck and keep delete all the way
• TEST_ROTATING_STICKY (PASSED): the delete doesn't get stuck and keep delete all the way while rotating

[keyman-server]

Changes in this pull request will be available for download in Keyman version 16.0.23-alpha