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

change(ios): Web-based popup key longpresses #2968

Merged
merged 6 commits into from
Apr 15, 2020

Conversation

jahorton
Copy link
Contributor

@jahorton jahorton commented Apr 8, 2020

Fixes #2716. (Well, works around the iOS bug causing it, but still.)
May also address #2835.

Sorry about the sporadic labels... this one's both a workaround for a current bug (b/c Apple iOS 13.4 bug) and an enhancement, as it implements a feature we've been talking about doing as the workaround.

This PR moves a lot of kmwnative.ts's definitions into visualKeyboard.ts proper, relying on JS method 'extension' patterns to override them for the Android case (so that it is unchanged). This allows iOS to use KMW's popup key code. As it turns out, the only real issue from that change was that popup keys weren't properly constrained within keyboard space; a bit of logic and alternate calculations later, and it's working. It was also the same for key previews.

For review purposes, it's probably better to review the first two commits separately from the commits that follow.


Note that this is based upon #2959, though its changes will supersede nearly all of that PR's changes - the JS-side functions that trigger Swift subkey displays are no longer used in this PR. We do need the globe-key component, though. Should we be able to rely upon our original implementation via Apple bugfix in 13.4.1 or 13.4.5, we'll be able to reconnect those code endpoints again, so we're not removing the no-longer-accessed codepaths at this time.

@jahorton jahorton marked this pull request as ready for review April 8, 2020 04:35
@mcdurdin
Copy link
Member

mcdurdin commented Apr 8, 2020

User Testing

Because this impacts the implementation of the on screen keyboard on three platforms -- iOS, Android and web -- we need to test on all three.

iOS in-app keyboard

  • Do key previews appear correctly?
  • Do key popups appear and interact correctly?
  • Does longpress on globe key work?
  • Sniff test: Is everything else the same on OSK (i.e. no other regressions)?

iOS system keyboard

  • Do key previews appear correctly?
  • Do key popups appear and interact correctly?
  • Does longpress on globe key work?
  • Sniff test: Is everything else the same on OSK (i.e. no other regressions)?

Android in-app keyboard

  • Do key previews appear correctly?
  • Do key popups appear and interact correctly?
  • Does longpress on globe key work?
  • Sniff test: Is everything else the same on OSK (i.e. no other regressions)?

Android system keyboard

  • Do key previews appear correctly?
  • Do key popups appear and interact correctly?
  • Does longpress on globe key work?
  • Sniff test: Is everything else the same on OSK (i.e. no other regressions)?

web keyboard

  • Do key previews appear correctly?
  • Do key popups appear and interact correctly?
  • Does longpress on globe key work?
  • Sniff test: Is everything else the same on OSK (i.e. no other regressions)?

@mcdurdin
Copy link
Member

mcdurdin commented Apr 8, 2020

iOS testing notes

Device: iPhone 7, Version: 12.4.1

  1. Khmer Angkor keyboard now shows two dotted circles on many long press options. This is mostly a display artefact that we don't necessarily need to resolve for this to go out.
  2. Long press on spacebar on Khmer Angkor, selecting 2nd option (which itself is a little weird) leaves the spacebar in purple 'pressed' state.
  3. The Globe icon flickers briefly on first touch but never shows a 'depressed' state which is depressing.
  4. I managed to hang Keyman once by pressing the globe icon in-app but never reproduced this.
  5. The look of the popups is not quite the same as the native popups.
    • preview letter is a little too high in the box:
      image

    • first option in the popup is narrower than the others:
      image

Apart from number 4, which I cannot reproduce, I don't think any of these are blockers. Given I can't repro, let's call it a glitch and ignore it for now.

Device: iPhone 11 Simulator, Version: 13.4

No differences to above.

Unable to test 13.4.5 because Apple do not provide a Simulator image for this and not willing to install on my iPhone 7.

@mcdurdin
Copy link
Member

mcdurdin commented Apr 8, 2020

@darcywong00 can you test Android please (I don't have my Android phone handy).

@darcywong00
Copy link
Contributor

darcywong00 commented Apr 9, 2020

@darcywong00 can you test Android please (I don't have my Android phone handy).

Android testing notes

So I ran the emulator on our minimum SDK 19, and saw the following in-app longpress issue

SDK 19 ios popup

It happens on several keys. And it's not isolated to this PR, because I get the same blanks on the current master. (I believe the longpress on the blank keys also output blank characters)

Longpress is fine on stable-13.0 though
sdk 19 stable-13

System keyboard behaved same as in-app.
Also, on Android longpress on the globe is the same has a normal press on globe.

Finally, Khmer angkor had a blank OSK. Retesting to see if it's the same on master
This also occurs on `master.
Screenshot_1586410875

@mcdurdin
Copy link
Member

mcdurdin commented Apr 9, 2020

@darcywong00 can you test Android please (I don't have my Android phone handy).

Android testing notes

So I ran the emulator on our minimum SDK 19, and saw the following in-app longpress issue

It happens on several keys. And it's not isolated to this PR, because I get the same blanks on the current master. (I believe the longpress on the blank keys also output blank characters)

Longpress is fine on stable-13.0 though

Haven't gotten to other scenarios yet...

This is all on min SDK 19 (which Android version is that)? This looks like it could be a font issue -- they are two different fonts -- perhaps the 14.0 version is not applying the correct font and so these rarer characters are missing?

@darcywong00
Copy link
Contributor

darcywong00 commented Apr 9, 2020

This is all on min SDK 19 (which Android version is that)?

That's Android 4.4 (KitKat). The OSK issues are addressed in issue #2977

Copy link
Member

@mcdurdin mcdurdin left a comment

Choose a reason for hiding this comment

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

LGTM. @jahorton there were a few minor issues with iOS that I noted - not necessarily related to this PR but I thought worth checking out. Can you assess those and determine if you want to open issues for them?

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

Successfully merging this pull request may close these issues.

[iOS] Eurolatin (SIL) long press menu does not appear
3 participants