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

chore(web,developer): Move keymanweb-osk.ttf to common/resources #9993

Merged
merged 11 commits into from
Nov 15, 2023

Conversation

darcywong00
Copy link
Contributor

@darcywong00 darcywong00 commented Nov 13, 2023

Follow-on to #9846

Since Developer and Web use the same keymanweb-osk.ttf resource, this PR moves it to /common/resources/fonts
and updates the configure step accordingly.

(edit to add @jahorton tests)

User Testing

  • TEST_WEB_OSK: Using the "Test unminified KeymanWeb" test page, verify that all OSK keys still show up properly.
  1. Load the page.
  2. Click in a text area so that the OSK becomes visible.
  3. Inspect all keys. If any has text beginning with and ending with **, FAIL this test.
  4. Such keys would normally have special OSK symbols on them.
  • TEST_DEVELOPER_WEB_OSK: Using the Developer test-host page for testing Web keyboards, verify that all OSK keys still show up properly. (updated for future reference not to use sil_euro_atlin)
  1. Load the keyboard project from the keyboards repo: release/basic/basic_kbdus.
  2. Build the keyboard.
  3. Test the keyboard using Web, starting the test server and using one of Developer's links to start the test page.
  4. Click in a text area so that the OSK becomes visible.
  5. Inspect all keys. If any has text beginning with and ending with **, FAIL this test.
    Such keys would normally have special OSK symbols on them.
  • TEST_ANDROID_OSK - Verifies special characters render on the OSK keycaps and longpress keys
    Setup - Install the PR build of KeyboardHarness on an Android device/emulator (newer than Android 5.0)
  1. Launch the KeyboardHarness app
  2. Use the Globe key to select "test9469" keyboard. (There's several test keyboards to cycle through
  3. Verify the keycaps on the default layer display and match the screenshot below (updated with ZWNJGeneric on the right of left-shift)

@keymanapp-test-bot
Copy link

keymanapp-test-bot bot commented Nov 13, 2023

User Test Results

Test specification and instructions

  • TEST_WEB_OSK (PASSED): I tested the 'Test unminified KeymanWeb' page in Chrome, and here are my observations: 1. Inspected all the keys on the On-Screen Keyboard and verified that none of them display text begins with or ends with **. (notes)
  • TEST_DEVELOPER_WEB_OSK (PASSED): Tested with the attached Keyman Developer 17.0.209-alpha-test-9993 and here are my observations: 1. Opened sil_euro_latin Keyboard project in Keyman Developer. 2. Compiled the keyboard. 3. Open it in the local host browser. 4. Inspected all the keys on the On-Screen Keyboard and verified that no display text begins with or ends with **. (notes)
  • TEST_ANDROID_OSK (PASSED): Installed the PR build of Keyboardharness on an Android device (Samsung Galaxy A23 5g - Android 13) and here is my observation: 1. Opened the KeyboardHarness app. 2 Switched to "test9469" Keyboard using globe key. 3. Verified that the keycaps on the default layer is displayed and matched with the given screenshot. (notes)

Test Artifacts

@@ -36,7 +36,12 @@ builder_parse "$@"

#### Build action definitions ####

builder_run_action configure verify_npm_setup
if builder_start_action configure; then
Copy link
Member

Choose a reason for hiding this comment

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

builder_describe_outputs (line 32) needs configure to point to the font, because /node_modules is already present, so this step is skipped.

Copy link
Contributor

@jahorton jahorton left a comment

Choose a reason for hiding this comment

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

This needs user tests. I'm requesting changes on the basis of the test-bot skip command. (There's a noticeable chance for breakage in how the font resource gets included in the build and/or linked for use on the test pages.)

TEST_WEB_OSK: Using the "Test unminified KeymanWeb" test page, verify that all OSK keys still show up properly.

  1. Load the page.
  2. Click in a text area so that the OSK becomes visible.
  3. Inspect all keys. If any has text beginning with and ending with **, FAIL this test.
    • Such keys would normally have special OSK symbols on them.

TEST_DEVELOPER_WEB_OSK: Using the Developer test-host page for testing Web keyboards, verify that all OSK keys still show up properly.

  1. Load a keyboard project. (Say, release/sil/sil_euro_latin from the keymanapp/keyboards repo.
  2. Build the keyboard.
  3. Test the keyboard using Web, starting the test server and using one of Developer's links to start the test page.
  4. Click in a text area so that the OSK becomes visible.
  5. Inspect all keys. If any has text beginning with and ending with **, FAIL this test.
    • Such keys would normally have special OSK symbols on them.

A similar test for one of the mobile apps (Android, iOS) may also be wise. And feel free to change the directions for the two tests I did spec above to be clearer and/or more specific about the steps if needed.

@keymanapp-test-bot keymanapp-test-bot bot added has-user-test user-test-required User tests have not been completed labels Nov 14, 2023
@@ -30,13 +30,19 @@ builder_describe "Builds the Keyman Engine for Web's On-Screen Keyboard package

builder_describe_outputs \
configure /node_modules \
configure /web/src/resources/osk/keymanweb-osk.ttf \
Copy link
Contributor

Choose a reason for hiding this comment

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

Wait... can the builder-script setup actually support two separate lines for the same action:target without losing one of them? If so... wish I'd known that; there's a lot of Web scripts that could use multiple output-checks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm hoping that syntax works. builder.inc failed when I originally had

  configure  /node_modules \
             /web/src/resources/osk/keymanweb-osk.ttf \
  build ...

Copy link
Member

@mcdurdin mcdurdin Nov 14, 2023

Choose a reason for hiding this comment

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

No, currently it's only one output per target. Anything more is a future builder feature.

@jahorton jahorton dismissed their stale review November 14, 2023 01:36

Requested user tests have now been spec'd.

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.

Sorry to say that the Developer build scripts are still Makefile-based, so we'll have to refactor that 😭

android/KMEA/build.sh Outdated Show resolved Hide resolved
developer/build.sh Show resolved Hide resolved
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.

looks good to me

@bharanidharanj
Copy link

Test Results

  • TEST_WEB_OSK (PASSED): I tested the 'Test unminified KeymanWeb' page in Chrome, and here are my observations: 1. Inspected all the keys on the On-Screen Keyboard and verified that none of them display text begins with or ends with **.

@bharanidharanj
Copy link

Test Results

  • TEST_DEVELOPER_WEB_OSK (PASSED): Tested with the attached Keyman Developer 17.0.209-alpha-test-9993 and here are my observations: 1. Opened sil_euro_latin Keyboard project in Keyman Developer. 2. Compiled the keyboard. 3. Open it in the local host browser. 4. Inspected all the keys on the On-Screen Keyboard and verified that no display text begins with or ends with **.

@bharanidharanj
Copy link

Test Results

  • TEST_ANDROID_OSK (PASSED): Installed the PR build of Keyboardharness on an Android device (Samsung Galaxy A23 5g - Android 13) and here is my observation: 1. Opened the KeyboardHarness app. 2 Switched to "test9469" Keyboard using globe key. 3. Verified that the keycaps on the default layer is displayed and matched with the given screenshot.

@keymanapp-test-bot keymanapp-test-bot bot removed the user-test-required User tests have not been completed label Nov 15, 2023
@darcywong00 darcywong00 merged commit 284ec09 into master Nov 15, 2023
15 checks passed
@darcywong00 darcywong00 deleted the chore/common/keymanweb-osk-ttf branch November 15, 2023 12:10
@keyman-server
Copy link
Collaborator

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

@mcdurdin
Copy link
Member

  • TEST_DEVELOPER_WEB_OSK: Using the Developer test-host page for testing Web keyboards, verify that all OSK keys still show up properly.
  1. Load a keyboard project. (Say, release/sil/sil_euro_latin from the keymanapp/keyboards repo.

Unfortunately sil_euro_latin doesn't use the standard OSK and none of the special characters would be visible, so this test result is invalid.

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.

5 participants