refactor(web): inactive banner management 🎏 #9988

jahorton · 2023-11-13T06:37:49Z

Accomplishes two goals:

Adds support for a new HTMLBanner type, which allows specifying banner contents to be injected via innerHTML property. For devices that support it, this will take place within an attached "shadow" for the banner in order to facilitate styling isolation.
Given the new banner type, I dropped the old 'select type of banner' approach in favor of 'set a Banner for use when inactive' approach.
- This allows direct calls to banner constructors, and thus to use of constructor parameters instead of messy, semi-dissociated "banner options". Thus, it becomes clear that only data for the requested 'inactive banner' type needs to be specified.

To set the new banner type in motion...

Once KeymanWeb has initialized, make a call like this:

keyman.osk.bannerController.inactiveBanner = new keyman.osk.bannerController.HTMLBanner(`
<img style="display:inline-block; height: 100%" src="../../../../../build/publish/debug/ui/button/kmw_button.gif"></div>
<div style="display:inline-block; color: white; text-align: center">Hello World</div>
`)

(I used that in the Developer console of the "Predictive Text: robust testing" test page.)

Results:

The same banner will be continuously reused for the lifetime of the page unless replaced with a different assignment to keyman.osk.bannerController.inactiveBanner. (Except when predictive-text is active, of course.)

User Testing

TEST_IOS_ALWAYS_SHOW: Using the iOS-based Keyman app, verify that the image banner still works as expected.

Install the iOS test artifact through TestFlight.
Follow the "Get Started" instructions fully.
- Be sure to install a keyboard without an associated predictive-text model... such as the IPA keyboard.
- Search: IPA
- Shows up as IPA (SIL) in the keyboard search.
In the Settings menu, toggle "Show Banner" on - the toggle should gain a green background.
Exit the Keyman app and open Safari.
Go to google.com.
Click within the main search field. When the system keyboard shows up, swap keyboards to reach the Keyman system keyboard.
Swap to the default "EuroLatin (SIL)" keyboard if it is not already active.
Swap to the keyboard without an associated predictive-text model.
Verify that the image banner is displayed.

Note that step 7, then 8 is necessary due to #9990. That bug is not addressed by this PR; I just happened to discover it when writing this user test.

…tiveBanner" prop

keymanapp-test-bot · 2023-11-13T06:37:54Z

User Test Results

Test specification and instructions

✅ TEST_IOS_ALWAYS_SHOW (PASSED): 1. Installed the iOS test artifact through TestFlight. 2. Opened the Keyman app. 3. Installed the IPA (SIL) keyboard using the Get Started menu. 4. Enabled "Show Banner" from the Settings menu. 5. Enabled Keyman as the System-wide keyboard. Closed the Keyman app. 6. In the Safari browser, opened the google.com page. Clicked the main search field. 7. Swapped Keyboards to Keyman System keyboard. 8. Swapped to the "IPA (SIL)" keyboard. Verified that the image banner is displayed on the top of the keyboard. (notes)

Test Artifacts

Android
Developer
iOS
- Keyman for iOS (simulator image)
- FirstVoices Keyboards for iOS (simulator image)
- TestFlight internal PR build version - 17.0.209 (0.9988.9738)
Keyboards
- Test Keyboards
Web
- KeymanWeb Test Home
Windows

web/src/engine/osk/src/banner/imageBanner.ts

web/src/engine/osk/src/banner/banner.ts

mcdurdin · 2023-11-13T08:45:28Z

web/src/engine/osk/src/banner/bannerView.ts

-    alwaysShow: false,
-    imagePath: ""
-  }
+  private _inactiveBanner: Banner;


Is this the opposite of activeBanner? The terms active and inactive here seem really ambiguous

... yeah, I bumped up against that for a bit. Kind of makes me want to change the .activeBanner to .currentBanner.

It might be clearer if I also split BannerView and BannerController into separate files in the predecessor PR; .activeBanner is only in the former, while .inactiveBanner is only in the latter.

Of course, there's also the timeless classic question of "... why not both?"

As of the newly-pushed commits, I've implemented both thoughts from my comment above.

web/src/engine/osk/src/banner/bannerView.ts

…eb/inactive-banner-management

darcywong00 · 2023-11-14T01:57:46Z

web/src/engine/osk/src/banner/bannerController.ts

+   * Builds a banner for use when predictions are not active, supporting a more generalized
+   * content pattern than ImageBanner via `innerHTML` specifications.
+   */
+  public readonly HTMLBanner = HTMLBanner;

  constructor(bannerView: BannerView, hostDevice: DeviceSpec, predictionContext?: PredictionContext) {
    // Step 1 - establish the container element.  Must come before this.setOptions.


this.setOptions is now gone?

Yeah. That paradigm started to feel really messy once I started adding in the HTML banner, especially since it obfuscates what properties are needed for which banner. Direct use of banner constructors is far, far clearer in that regard.

jahorton · 2023-11-14T03:06:50Z

Note: once the Android "image banner" work is done, we should absolutely test it against Android 5.0 just to be sure everything does work as intended. That work isn't available here, hence the lack of test for it.

web/src/engine/osk/src/banner/bannerController.ts

bharanidharanj · 2023-11-15T11:40:33Z

Test Results

TEST_IOS_ALWAYS_SHOW (PASSED): 1. Installed the iOS test artifact through TestFlight. 2. Opened the Keyman app. 3. Installed the IPA (SIL) keyboard using the Get Started menu. 4. Enabled "Show Banner" from the Settings menu. 5. Enabled Keyman as the System-wide keyboard. Closed the Keyman app. 6. In the Safari browser, opened the google.com page. Clicked the main search field. 7. Swapped Keyboards to Keyman System keyboard. 8. Swapped to the "IPA (SIL)" keyboard. Verified that the image banner is displayed on the top of the keyboard.

keyman-server · 2023-11-16T18:01:58Z

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

jahorton added 2 commits November 13, 2023 13:31

refactor(web): drops banner options paradigm in favor of simple "inac…

109920e

…tiveBanner" prop

feat(web): new banner type - HTMLBanner

18b2903

keymanapp-test-bot bot added the user-test-missing User tests have not yet been defined for the PR label Nov 13, 2023

keymanapp-test-bot bot added this to the A17S26 milestone Nov 13, 2023

github-actions bot added web/ ios/ ios/engine/ refactor labels Nov 13, 2023

jahorton mentioned this pull request Nov 13, 2023

bug(ios): image-banner disappears after rotating device orientation #9990

Closed

8 tasks

keymanapp-test-bot bot added has-user-test user-test-required User tests have not been completed and removed user-test-missing User tests have not yet been defined for the PR labels Nov 13, 2023

jahorton marked this pull request as ready for review November 13, 2023 08:39

jahorton requested review from mcdurdin and sgschantz as code owners November 13, 2023 08:39

mcdurdin approved these changes Nov 13, 2023

View reviewed changes

jahorton added 3 commits November 14, 2023 08:20

Merge branch 'chore/web/separate-banner-source-files' into refactor/w…

a02dad0

…eb/inactive-banner-management

chore(web): reverts removal of width-styling from ImageBanner

cf36490

chore(web): activeBanner -> currentBanner, some doc-comments

a88367d

darcywong00 reviewed Nov 14, 2023

View reviewed changes

jahorton commented Nov 14, 2023

View reviewed changes

web/src/engine/osk/src/banner/bannerController.ts Outdated Show resolved Hide resolved

fix(web): bug from applying suggestion

db67f04

keymanapp-test-bot bot removed the user-test-required User tests have not been completed label Nov 15, 2023

Base automatically changed from chore/web/separate-banner-source-files to master November 16, 2023 01:06

jahorton requested a review from ermshiperete as a code owner November 16, 2023 01:06

jahorton merged commit 8ebb725 into master Nov 16, 2023
15 checks passed

jahorton deleted the refactor/web/inactive-banner-management branch November 16, 2023 01:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(web): inactive banner management 🎏 #9988

refactor(web): inactive banner management 🎏 #9988

jahorton commented Nov 13, 2023 •

edited

Loading

keymanapp-test-bot bot commented Nov 13, 2023 •

edited

Loading

mcdurdin Nov 13, 2023

jahorton Nov 13, 2023

jahorton Nov 14, 2023

darcywong00 Nov 14, 2023

jahorton Nov 14, 2023 •

edited

Loading

jahorton commented Nov 14, 2023

bharanidharanj commented Nov 15, 2023

keyman-server commented Nov 16, 2023

refactor(web): inactive banner management 🎏 #9988

refactor(web): inactive banner management 🎏 #9988

Conversation

jahorton commented Nov 13, 2023 • edited Loading

User Testing

keymanapp-test-bot bot commented Nov 13, 2023 • edited Loading

User Test Results

Test Artifacts

mcdurdin Nov 13, 2023

Choose a reason for hiding this comment

jahorton Nov 13, 2023

Choose a reason for hiding this comment

jahorton Nov 14, 2023

Choose a reason for hiding this comment

darcywong00 Nov 14, 2023

Choose a reason for hiding this comment

jahorton Nov 14, 2023 • edited Loading

Choose a reason for hiding this comment

jahorton commented Nov 14, 2023

bharanidharanj commented Nov 15, 2023

Test Results

keyman-server commented Nov 16, 2023

jahorton commented Nov 13, 2023 •

edited

Loading

keymanapp-test-bot bot commented Nov 13, 2023 •

edited

Loading

jahorton Nov 14, 2023 •

edited

Loading