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(android): Web-based popup key longpressses #9573

Closed
darcywong00 opened this issue Sep 18, 2023 · 15 comments · Fixed by #9696 or #9591
Closed

change(android): Web-based popup key longpressses #9573

darcywong00 opened this issue Sep 18, 2023 · 15 comments · Fixed by #9696 or #9591

Comments

@darcywong00
Copy link
Contributor

darcywong00 commented Sep 18, 2023

From the Sept 2023 team planning meetings, it was decided to have Android match iOS (see #2968) in using KeymanWeb for handling longpress keys.

Pros: maintainability of OSK, also avoids duplicating gestures
Cons: The longpresses would be limited to the top of the OSK / banner

Affects: Keyman Engine for Android consumers (FV, KAB, etc).

This might also end up resolving open issues specific to Android longpresses
e.g. #9499

@mcdurdin
Copy link
Member

Note: this means banner will always be visible on Android, no option to switch off banner any more.

@darcywong00 darcywong00 changed the title change(android): Move to use KeymanWeb longpresses change(android): Web-based popup key longpressses Sep 18, 2023
@darcywong00
Copy link
Contributor Author

Preliminary implementation
andorid kmw osk

shows some TODO styling to be consistent with current popup
image

Does iOS also limit popup to the banner within the Keyman app?

@darcywong00
Copy link
Contributor Author

Note: this means banner will always be visible on Android, no option to switch off banner any more.

UX question: Should Android follow iOS and display a Keyman image banner when predictions are disabled? It may take me some time to repurpose the iOS banners to the Android form factors.

@jahorton
Copy link
Contributor

I think the plan is to go there eventually, but that's not something iOS currently mandates. iOS has the "Show Banner" toggle - the banner only shows if predictions are available OR if that toggle is on.

In the screenshot below, the toggle is off and there is no lexical model.

File (3)

@mcdurdin
Copy link
Member

Note: this means banner will always be visible on Android, no option to switch off banner any more.

UX question: Should Android follow iOS and display a Keyman image banner when predictions are disabled? It may take me some time to repurpose the iOS banners to the Android form factors.

That needs to be done, yes. The UX for longpress when the banner is not visible is not very good.

@darcywong00
Copy link
Contributor Author

darcywong00 commented Sep 20, 2023

ok. so it sounds like I should have Keyman Engine for Android always display the banner (probably a blank black banner) that the apps can then theme with a custom image banner.

It's a little unfortunate "blank banner" in KeymanWeb currently means a banner height 0

/**
* Function BlankBanner
* Description A banner of height 0 that should not be shown
*/
export class BlankBanner extends Banner {
constructor() {
super(0);
}
}

The language settings toggle to disable predictions will stop suggestions from appearing, but the banner remains.

@mcdurdin
Copy link
Member

ok. so it sounds like I should have Keyman Engine for Android always display the banner (probably a blank black banner) that the apps can then theme with a custom image banner.

Yes, this should be the same in both Android and iOS, ideally. The banner image could be specified by Android (and iOS)'s keyboard.html CSS, and if that's possible, then I suggest that it could be implemented something like this:

  #keymanEmptyBanner {  /* I don't know what the appropriate selector is though */
    background-color: #ffffff;
    background-image: url('keyman-banner.png');
    background-size: auto 100%;
    background-repeat: no-repeat;
    background-position: top left;
  }

This would scale the image according to the height of the banner, and fill remaining space white, I think. That should handle all the various aspect ratios. One could use keyman-banner.svg instead, which would give a better result on all devicess.

That way we don't need to have a new API to define the image. But this depends on how the existing banner is implemented.

@jahorton thoughts? Does this sound sensible?

@jahorton
Copy link
Contributor

jahorton commented Sep 20, 2023

What the iOS app actually does is use the ImageBanner, passing along a dynamically-constructed data URL for the image's source.

For an interactive version of the idea: https://elmah.io/tools/base64-image-encoder/ can be used to construct a data URL; set the image banner's image-source to the URL spit out and you've got an image.

For a small black box that could be tiled as the background:

<img src="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAABoAAAARCAIAAABM7ytaAAAAAXNSR0IArs4c6QAAAARnQU1BAACxjwv8YQUAAAAJcEhZcwAAEnQAABJ0Ad5mH3gAAAAVSURBVDhPYxgFo2AUjIJRQDpgYAAABT8AAcEGbxwAAAAASUVORK5CYII=" />

For what iOS uses, https://github.com/keymanapp/keyman/blob/master/ios/keyman/Keyman/SWKeyboard/ImageBannerViewController.swift has the most compact, centralized version of the code. It actually renders a full iOS view to an image, then converts that to base64. See renderAsBase64 - there were built-in methods on the iOS image type we were able to utilize.

The result is then forwarded through this block to the WebView:

func setBannerImage(to path: String) {
bannerImgPath = path // Save the path in case delayed initializaiton is needed.
var logString: String
if path.contains("base64") || path.count > 256 {
logString = "<base64 image>"
} else {
logString = path
}
log.debug("Banner image path: '\(logString).'")
webView?.evaluateJavaScript("setBannerImage(\"\(path)\");", completionHandler: nil)
}

which in turn references this:

function setBannerImage(path) {
var kmw=window['keyman'];
if(kmw.osk) {
kmw.osk.bannerController.setOptions({"imagePath": path});
}
}

This approach was taken partly due to iOS dark-mode support.

Seems like Java has a Base64 utility class that might make a good lead. Here's a StackOverflow post regarding it; not the most clear, but it should still be helpful. https://stackoverflow.com/a/36492265


Ah right, the other part of the reason we took this approach: it's quite difficult to construct a sufficient set of banner images for all the different aspect ratios supported by iOS; by constructing it at runtime, we can ensure the banner's layout always looks good and appropriate. But yeah, there is an API for this in the iOS engine.

@mcdurdin
Copy link
Member

That seems overly complicated. One could just use the dark mode CSS property. And then we can keep it purely web-side.

if (window.matchMedia && window.matchMedia('(prefers-color-scheme: dark)').matches) {
    // dark mode
}

https://stackoverflow.com/a/57795495/1836776 also shows how to detect change in color scheme.

In CSS:

@media (prefers-color-scheme: dark) {

@jahorton
Copy link
Contributor

We already use that for the OSK CSS.

@mcdurdin
Copy link
Member

Ah right, the other part of the reason we took this approach: it's quite difficult to construct a sufficient set of banner images for all the different aspect ratios supported by iOS; by constructing it at runtime, we can ensure the banner's layout always looks good and appropriate. But yeah, there is an API for this in the iOS engine.

Hence my suggestion of the background CSS properties -- that way AFAICT we only need 1 image and aspect ratio issues basically go away.

@jahorton
Copy link
Contributor

jahorton commented Sep 20, 2023

As a reminder of the app's history, here's how we used to specify the banner's image before predictive text:

https://github.com/keymanapp/keyman/tree/stable-11.0/ios/keyman/Keyman/SWKeyboard/Keyman%20Banner

There were quite a few image files for the various aspect ratios and resolution levels involved.

#1703 seems to be when the change was made.

Part of this was also due to our desire to allow OEM theming for the banner - FirstVoices had its own banner style, after all. Still does, actually, and it uses the old, pre-12.0 approach with separate defined files:

func getTopBarImage(isPortrait: Bool) -> String? {
if isPortrait {
return Bundle.main.path(forResource: "banner-Portrait@2x", ofType: "png")
}
// iPad
if UIDevice.current.userInterfaceIdiom != UIUserInterfaceIdiom.phone {
return Bundle.main.path(forResource: "banner-Landscape@2x", ofType: "png")
}
// iPhone
let screenRect = UIScreen.main.bounds
if CGFloat.maximum(screenRect.height, screenRect.width) >= 568.0 {
return Bundle.main.path(forResource: "banner-Landscape-568h@2x", ofType: "png")
} else {
return Bundle.main.path(forResource: "banner-Landscape@2x", ofType: "png")
}
}

The main issue with the pure Web-side suggestion is that, so far as I can see, we lose the OEM theming.

@darcywong00
Copy link
Contributor Author

public static readonly DEFAULT_OPTIONS: BannerOptions = {
alwaysShow: false,
imagePath: ""
}
constructor(bannerView: BannerView, hostDevice: DeviceSpec, predictionContext?: PredictionContext) {
// Step 1 - establish the container element. Must come before this.setOptions.
this.hostDevice = hostDevice;
this.container = bannerView;
this.predictionContext = predictionContext;
// Initialize with the default options - any 'manually set' options come post-construction.
// This will also automatically set the default banner in place.
this.setOptions(BannerController.DEFAULT_OPTIONS);
}

afaict, even though I have Keyman for Android change the banner to image, KeymanWeb already initialized with a constructor for new BlankRanner().

@jahorton
Copy link
Contributor

jahorton commented Sep 25, 2023

That would be its default value until a different form of banner is specified.

From iOS:

function showBanner(flag) {
console.log("Setting banner display for dictionaryless keyboards to " + flag);
keyman.osk.bannerController.setOptions({'alwaysShow': flag});
}

The alwaysShow field there will ensure that the OSK defaults to an image banner when predictive-text is unavailable, rather than blank.

Alternatively, keyman.osk.banner.selectBanner('image') should work, but it won't persist as a default.

@darcywong00 darcywong00 linked a pull request Nov 17, 2023 that will close this issue
@darcywong00
Copy link
Contributor Author

Fixed by #9591 and #9696

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