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

feat(web): &newLayer and &oldLayer #6366

Merged
merged 10 commits into from
Mar 23, 2022

Conversation

mcdurdin
Copy link
Member

@mcdurdin mcdurdin commented Mar 12, 2022

Fixes #6364.
Fixes #6112. (I think).

Removes &layerChanged and adds &newLayer and &oldLayer system stores with the additional nuances around values as described in #6364.

User Testing

Setup

Android/IOS: Run these tests with the obolo_chwerty_6351 keyboard available in the Test-14.0 (Keyman - Windows (Desktop/Developer)) artifacts.

Web: The obolo_chwerty_6351 keyboard for these tests is available under English.../obolo_chwerty_6351 in the globe menu.

  1. touch English... first:

    image

  2. Then obolo_chwerty_6351 should be available in the submenu that appears:

    image

Base Configurations

These tests should be run on Android, iOS and Web.

GROUP_ANDROID: Run the Keyman app, either in Android emulator or real device. Version of Android is not important. Test the System keyboard and In-App keyboard.
GROUP_IOS: Run the Keyman app, either in iPhone simulator or real device. Version of iOS is not important. Test the System keyboard and In-App keyboard.
GROUP_WEB: Run testing/unminified, either in Chrome desktop, with mobile emulation, or mobile device/emulator. The obolo_chwerty_6351 keyboard for this test is available under English.../obolo_chwerty_6351 in the globe menu.

Tests

  • TEST_STARTS_WITH_SHIFT: Make sure that the initial shift layer is working correctly.

    1. Verify that the initial layer when the keyboard is activated in an empty text area is Shift.
    2. Press an alphabet key on the shift layer to verify that the output is correctly shifted.
    3. Verify that the layer switches to the base layer.
    4. Press another alphabet key, verify that the output is lower case.
  • TEST_CAPS: Verify behaviour of Caps Lock

    1. Double-tap on the Shift key to activate Caps Lock.
    2. Verify that the next alphabet key pressed generates capitalised output.
    3. Verify that the layer remains the Caps Lock layer.
    4. Press the CH key and verify that it is also capitalised.
    5. Press Shift again to return to the base layer.
    6. Verify that the next alphabet key pressed generates lower case output.

SUITE_SOS_IOS

Fixes #6364.

Removes `&layerChanged` and adds `&newLayer` and `&oldLayer` system
stores with the additional nuances around values as described in #6364.
@mcdurdin mcdurdin added this to the B15S4 milestone Mar 12, 2022
@keymanapp-test-bot keymanapp-test-bot bot added has-user-test user-test-required User tests have not been completed labels Mar 12, 2022
@keymanapp-test-bot
Copy link

keymanapp-test-bot bot commented Mar 12, 2022

User Test Results

Test specification and instructions

  • ✅ GROUP_ANDROID: Run the Keyman app, either in Android emulator or real device. Version of Android is not important. Test the System keyboard and In-App keyboard.

    2 tests PASSED
    • TEST_STARTS_WITH_SHIFT (PASSED): Tested this on Android Emulator API 29 and it is working as expected. (notes)
    • TEST_CAPS (PASSED): Tested this on Android Emulator API 29 and it working as expected. (notes)
  • 🟥 GROUP_IOS: Run the Keyman app, either in iPhone simulator or real device. Version of iOS is not important. Test the System keyboard and In-App keyboard.

    • 🟥 TEST_STARTS_WITH_SHIFT (FAILED) (notes)
    • TEST_CAPS (PASSED): works as indicated in the instructions.
  • ✅ GROUP_WEB: Run testing/unminified, either in Chrome desktop, with mobile emulation, or mobile device/emulator. The obolo_chwerty_6351 keyboard for this test is available under English.../obolo_chwerty_6351 in the globe menu.

    2 tests PASSED
    • TEST_STARTS_WITH_SHIFT (PASSED): The initial layer on an empty text field is indeed Shift. (notes)
    • TEST_CAPS (PASSED): Double tap on the Shift key does enable Caps Lock. Tap on the Shift key again does disable the Caps Lock.

🟥 SUITE_SOS_IOS:

Test Artifacts

@mcdurdin mcdurdin marked this pull request as ready for review March 13, 2022 19:40
Copy link
Contributor

@darcywong00 darcywong00 left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@ermshiperete ermshiperete left a comment

Choose a reason for hiding this comment

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

It's a shame that we have to re-define TSS_NEWLAYER and TSS_OLDLAYER over and over again...

@mcdurdin
Copy link
Member Author

It's a shame that we have to re-define TSS_NEWLAYER and TSS_OLDLAYER over and over again...

Definitely. I think in the future the list in Compiler.h should be able to instead use kmx_file.h, and the KMBinaryFileFormat.h header should disappear completely.

Base automatically changed from fix/web/caps-state-tracking-for-touch to beta March 14, 2022 23:04
@bharanidharanj
Copy link

  • TEST_CAPS (FAILED): Tested this on iPhone 13 Pro simulator and it is failed in (In-App keyboard) the 3rd Step (Verify that the layer remains the Caps Lock layer) as it mentioned in the Test Steps.

Good catch! I have addressed this now in the latest update. What was happening was that the host app was resetting the edit context with each keystroke, returning us to a default state.

Given this change, I think we should re-run both the iOS tests. Thanks 😁

@keymanapp-test-bot retest GROUP_IOS TEST_STARTS_WITH_SHIFT TEST_CAPS

@mcdurdin Okay, Thanks.

@mcdurdin mcdurdin modified the milestones: B15S4, B15S5 Mar 21, 2022
@bharanidharanj
Copy link

bharanidharanj commented Mar 21, 2022

GROUP_IOS: Run the Keyman app, either in iPhone simulator or real device. Version of iOS is not important. Test the System keyboard and In-App keyboard.

  • TEST_STARTS_WITH_SHIFT (FAILED): Re-tested this on iPhone 13 Pro Simulator and noticed that clicking on the Backspace key would enable the Shift Key changes to Caps.

In-App Keyboard

System Keyboard

  • TEST_CAPS (FAILED): Re-tested this on iPhone 13 Pro Simulator and it is passed. I found an issue at Step v (Press Shift again to return to the base layer.). Here, the Shift Key Color changes from light blue to dark blue after clicking the Shifty key. (In-App 1.png) However, clicking on any letter key would show the lower case letter on the Screen. (In-App2.png)

In-App 1

In-App2

System Keyboard

@bharanidharanj
Copy link

GROUP_WEB: Run testing/unminified, either in Chrome desktop, with mobile emulation, or mobile device/emulator.

  • TEST_STARTS_WITH_SHIFT (FAILED): Tested this on mobile emulator in Chrome desktop and Shift key is not enabled (for the first time) when I was in the text area.

  • TEST_CAPS (FAILED): Tested this on mobile emulator in Chrome desktop and double clicking on the Shift key is not working as expected.

@bharanidharanj
Copy link

bharanidharanj commented Mar 21, 2022

SUITE_SOS_IOS:

  • TEST_6112 (FAILED): Tested this on iOS 15.2 (iPhone 13 Pro Simulator) and the Shift Key is not activated after I clicking on the text area. ie., it is still showing the default layer.

@mcdurdin
Copy link
Member Author

GROUP_WEB: Run testing/unminified, either in Chrome desktop, with mobile emulation, or mobile device/emulator.
TEST_STARTS_WITH_SHIFT (FAILED)

Sorry, my mistake. The instructions should have been clearer on this -- the keyboard to test is already available under English.../obolo_chwerty_6351 in the globe menu for KeymanWeb. I have updated the instructions!

@keymanapp-test-bot retest GROUP_WEB TEST_STARTS_WITH_SHIFT TEST_CAPS

@mcdurdin
Copy link
Member Author

mcdurdin commented Mar 21, 2022

GROUP_IOS:

  • TEST_STARTS_WITH_SHIFT (FAILED): Re-tested this on iPhone 13 Pro Simulator and noticed that clicking on the Backspace key would enable the Shift Key changes to Caps.

I think this is intended behaviour? When deleting text to return to an empty text area, the keyboard should return to Shift state. You can tell it is Shift rather than Caps because the top left key is Sh, not SH!

GROUP_IOS
TEST_STARTS_WITH_SHIFT PASSED

@mcdurdin
Copy link
Member Author

mcdurdin commented Mar 22, 2022

SUITE_SOS_IOS:

  • TEST_6112 (FAILED): Tested this on iOS 15.2 (iPhone 13 Pro Simulator) and the Shift Key is not activated after I clicking on the text area. ie., it is still showing the default layer.

Okay, I believe I've fixed this now -- I've retested locally and it appears to be correct.

@keymanapp-test-bot retest SUITE_SOS_IOS TEST_6112

@mcdurdin
Copy link
Member Author

mcdurdin commented Mar 22, 2022

  • GROUP_IOS ... TEST_CAPS (FAILED): Re-tested this on iPhone 13 Pro Simulator and it is passed. I found an issue at Step v (Press Shift again to return to the base layer.). Here, the Shift Key Color changes from light blue to dark blue after clicking the Shifty key. (In-App 1.png) However, clicking on any letter key would show the lower case letter on the Screen. (In-App2.png)

I haven't been able to reproduce this. The dark blue colour normally appears as the 'down' touch action and then is replaced with a lighter blue to indicate that Shift is selected (or grey to indicate that Shift is not selected) once the user releases the touch. Can you try this again? If this continues to go wrong, we might list it as a separate issue to resolve.

@keymanapp-test-bot retest GROUP_IOS TEST_CAPS

@MakaraSok
Copy link
Collaborator

Test Results

GROUP_IOS: Run the Keyman app, either in iPhone simulator or real device. Version of iOS is not important. Test the System keyboard and In-App keyboard.

  • TEST_STARTS_WITH_SHIFT (FAILED): The keyboard doesn't show Shift layer when the keyboard is activated, i.e. switch from a different keyboard to bolo_chwerty; unless tapping in the text area where the cursor is blinking. See the screen recording below.
not.shown.Shift.right.off.unless.touch.text.field.mov
  • TEST_CAPS (PASSED): works as indicated in the instructions.

@MakaraSok
Copy link
Collaborator

MakaraSok commented Mar 22, 2022

Test Results

GROUP_WEB: Run testing/unminified, either in Chrome desktop, with mobile emulation, or mobile device/emulator. The obolo_chwerty_6351 keyboard for this test is available under English.../obolo_chwerty_6351 in the globe menu.

  • TEST_STARTS_WITH_SHIFT (BLOCKED): The obolo_chwerty_6351 cannot be found per instructions.
  • TEST_CAPS (BLOCKED): The obolo_chwerty_6351 cannot be found per instructions.

Link used: id: 312043

Chrome desktop with mobile emulation:
image

Mobile device:
image

With Darcy's help, the keyboard, however, is found under View Keymanweb manual test pages.

image

image

Can you please clarify which link to go to to test this PR?
(1) View Keymanweb use samples.
(2) View Keymanweb manual test pages.

My go to is (1) when not specified.

This test is still considered blocked until clarification needed is made. Thanks.

@MakaraSok
Copy link
Collaborator

MakaraSok commented Mar 22, 2022

Test Results

SUITE_SOS_IOS:

  • TEST_6112 (FAILED): In-app, the keyboard doesn't show right off with the Shift layer in-app (while system does), however the basic function of having a letter capitalized after a ., ! or ? and a space is working well.

One main problem is that if adding more than two spaces after any of the three punctuations above, the keyboard shows the lower case layer instead.

In-app:

in-app.more.than.two.space.after.the.3.puncts.mov

System:

system.-.more.than.two.spaces.after.the.3.puncts.mov

Note: A good to have feature - the keyboard should stay on Shift layer when pressing the return key after the ".!? + spaces". Currently, it switches to small caps in both in-app and system keyboard.

in-app (same for system):

return.after.+space.mov

@keymanapp-test-bot keymanapp-test-bot bot removed the user-test-required User tests have not been completed label Mar 22, 2022
@MakaraSok
Copy link
Collaborator

Test Results

GROUP_WEB: Run testing/unminified, either in Chrome desktop, with mobile emulation, or mobile device/emulator. The obolo_chwerty_6351 keyboard for this test is available under English.../obolo_chwerty_6351 in the globe menu.

  • TEST_STARTS_WITH_SHIFT (PASSED): The initial layer on an empty text field is indeed Shift.

  • TEST_CAPS (PASSED): Double tap on the Shift key does enable Caps Lock. Tap on the Shift key again does disable the Caps Lock.

The iOS code was making a direct call to `keyman.core.resetContext()`
rather than to `keyman.resetContext()`. This meant that the
`OutputTarget` was not set and the `newContext` event would never be
called (as well as triggering a background warning in the language
processor "OutputTarget missing during an invalidateContext call").

Refactored to include a wrapper for `resetContext()` in ios-host.js, as
I think it's better to proxy all calls to KMW consistently through the
host javascript.

(There are a couple of other direct calls which could be cleaned up;
search for "evaluateJavaScript" to find all calls; I think we could
consider build a wrapper for all these calls?)
@mcdurdin
Copy link
Member Author

mcdurdin commented Mar 22, 2022

I've revisited the context reset code and found a couple of inconsistencies. I think this addresses the starting shift state in iOS more cleanly. @MakaraSok can you retest with the new build?

@keymanapp-test-bot retest GROUP_IOS TEST_STARTS_WITH_SHIFT

@keymanapp-test-bot keymanapp-test-bot bot added the user-test-required User tests have not been completed label Mar 22, 2022
@mcdurdin
Copy link
Member Author

@keymanapp-test-bot retest SUITE_SOS_IOS TEST_6112

@MakaraSok
Copy link
Collaborator

Test Results

GROUP_IOS: Run the Keyman app, either in iPhone simulator or real device. Version of iOS is not important. Test the System keyboard and In-App keyboard.

  • TEST_STARTS_WITH_SHIFT (FAILED):

in-app: 👎

The keyboard still doesn't show the Shift layer right off. Plus the predictive banner in particular is pushed up and cropped off. The keyboard itself seems to be pushed up leaving a huge empty gab at the bottom of the keyboard.

system: 👍

The keyboard shows Shift layer right off after switching to the keyboard. The banner looks good, no cropped off like that of the in-app.

image

Clicking in the text area doesn't enable the Shift layer either. Note also

no.Shift.right.off.mov

SUITE_SOS_IOS:

  • TEST_6112 (FAILED): notes

in-app: 👎

The keyboard doesn't switch to the Shift layer at the start of a text field; however, it does after full stop (.), exclamation (!) or question mark (?) and one or two spaces (e.g. . ). Three spaces and a return key after one of the three punctuations doesn't keep the keyboard on the Shifter layer either, it actually is expected though to have the keyboard stay on the Shift layer.

system: 👍
The keyboard does switch to the Shift layer at the start of a text field; however, it does after full stop (.), exclamation (!) or question mark (?) and one or two spaces (e.g. . ). Three spaces and a return key after one of the three punctuations doesn't keep the keyboard on the Shifter layer either, it actually is expected though to have the keyboard stay on the Shift layer.

@keymanapp-test-bot keymanapp-test-bot bot removed the user-test-required User tests have not been completed label Mar 23, 2022
@mcdurdin
Copy link
Member Author

I have isolated the primary remaining issue (which is minor) into #6416 as we want to get this in front of beta users. Merging.

@mcdurdin mcdurdin merged commit 5c388d1 into beta Mar 23, 2022
@mcdurdin mcdurdin deleted the feat/web/6364-new-layer-old-layer-replaces-layerchanged branch March 23, 2022 05:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

feat(web): tweak meaning of &layerChanged store to reflect name of new layer
5 participants