fix(android): fix loading of keyboards on Android#16146
Conversation
User Test ResultsTest specification and instructions ERROR: user tests have not yet been defined Test Artifacts
|
f317db4 to
b406759
Compare
This change moves to using `WebViewAssetLoader` for loading files from the device instead of using file:// URLs. This fixes the blank keyboard problem reported in #16096 for Android. Also make `KMKeyboard.getKeyboardRoot()` private, and rename public `Keyboard.getKeyboardPath()` to private `Keyboard.getKeyboardUrl()`. Part-of: #16096
b406759 to
64abd5b
Compare
Note that https://help.keyman.com/developer/engine/android/current-version/KMKeyboard/ |
jahorton
left a comment
There was a problem hiding this comment.
LGTM aside from the one nit.
| } | ||
|
|
||
| public String getKeyboardRoot() { | ||
| private String getKeyboardRoot() { |
There was a problem hiding this comment.
We should probably leave this unchanged; KMKeyboard is an engine API class.
There was a problem hiding this comment.
I asked the App Builder team about it, just in case; no effect for them at present.
There was a problem hiding this comment.
This was an internal public, not a published API endpoint, can safely make private.
If it's not documented, then it's not part of the API. It's a public method but an internal implementation detail. |
mcdurdin
left a comment
There was a problem hiding this comment.
This is looking good, some tidyup and consolidation requested. Also have noted future work on KMDefault_UndefinedPackageID (we can turn that into a follow-up issue).
| if (packageID.equals(KMManager.KMDefault_UndefinedPackageID)) { | ||
| return keyboardRoot + KMManager.KMDefault_UndefinedPackageID + File.separator; | ||
| keyboardRoot += KMManager.KMDefault_UndefinedPackageID + "/"; | ||
| } else { |
There was a problem hiding this comment.
I know this isn't new, I really wonder about the whole KMDefault_UndefinedPackageID (aka "cloud") pattern throughout this code. It seems like this is a fake "null" but I haven't fully audited its use. I would love to eliminate it altogether (28 references) because it seems like it is unnecessary complexity.
| private static String txtFont = ""; | ||
| private static String oskFont = null; | ||
| private static String keyboardRoot = ""; | ||
| private String keyboardRoot = ""; |
| if (packageID.equals(KMManager.KMDefault_UndefinedPackageID)) { | ||
| this.keyboardRoot = (context.getDir("data", Context.MODE_PRIVATE).toString() + | ||
| File.separator + KMManager.KMDefault_UndefinedPackageID + File.separator); | ||
| this.keyboardRoot = WebViewUtils.MAGIC_DEFAULT_DOMAIN + "/data/" + KMManager.KMDefault_UndefinedPackageID + "/"; |
There was a problem hiding this comment.
| this.keyboardRoot = WebViewUtils.MAGIC_DEFAULT_DOMAIN + "/data/" + KMManager.KMDefault_UndefinedPackageID + "/"; | |
| this.keyboardRoot = WebViewUtils.buildAssetUrl(KMManager.KMDefault_UndefinedPackageID + "/"); |
Can we make a method under WebViewUtils which builds these URLs so we don't repeat the WebViewUtils.MAGIC_DEFAULT_DOMAIN + "/data/" pattern?
public String buildAssetUrl(String assetPath) {
return WebViewUtils.MAGIC_DEFAULT_DOMAIN + "/data/" + assetPath;
}Or even:
| this.keyboardRoot = WebViewUtils.MAGIC_DEFAULT_DOMAIN + "/data/" + KMManager.KMDefault_UndefinedPackageID + "/"; | |
| this.keyboardRoot = WebViewUtils.buildAssetUrl([KMManager.KMDefault_UndefinedPackageID, ""]); |
public String buildAssetUrl(String[] assetPath) {
return WebViewUtils.MAGIC_DEFAULT_DOMAIN + "/data/" + String.join("/", assetPath);
}| } else { | ||
| this.keyboardRoot = (context.getDir("data", Context.MODE_PRIVATE).toString() + | ||
| File.separator + KMManager.KMDefault_AssetPackages + File.separator + packageID + File.separator); | ||
| this.keyboardRoot = WebViewUtils.MAGIC_DEFAULT_DOMAIN + "/data/" + KMManager.KMDefault_AssetPackages + "/" + packageID + "/"; |
There was a problem hiding this comment.
| this.keyboardRoot = WebViewUtils.MAGIC_DEFAULT_DOMAIN + "/data/" + KMManager.KMDefault_AssetPackages + "/" + packageID + "/"; | |
| this.keyboardRoot = WebViewUtils.buildAssetUrl(KMManager.KMDefault_AssetPackages + "/" + packageID + "/"); |
or
| this.keyboardRoot = WebViewUtils.MAGIC_DEFAULT_DOMAIN + "/data/" + KMManager.KMDefault_AssetPackages + "/" + packageID + "/"; | |
| this.keyboardRoot = WebViewUtils.buildAssetUrl([KMManager.KMDefault_AssetPackages, packageID]); |
| public static String getResourceUrl() { | ||
| return WebViewUtils.MAGIC_DEFAULT_DOMAIN +"/data/"; | ||
| } |
There was a problem hiding this comment.
Use shared method in WebViewUtils instead as proposed
| } | ||
|
|
||
| public static String getLexicalModelsUrl() { | ||
| return getResourceUrl() + KMDefault_LexicalModelPackages + "/"; |
There was a problem hiding this comment.
| return getResourceUrl() + KMDefault_LexicalModelPackages + "/"; | |
| return WebViewUtils.buildAssetUrl(KMDefault_LexicalModelPackages + "/"); |
or
| return getResourceUrl() + KMDefault_LexicalModelPackages + "/"; | |
| return WebViewUtils.buildAssetUrl([KMDefault_LexicalModelPackages, ""]); |
| String htmlPath = "file://" + getContext().getDir("data", Context.MODE_PRIVATE) + "/" + KMManager.KMFilename_KeyboardHtml; | ||
| // Use the reserved magic domain for loading the keyboard from the local device. | ||
| // See https://developer.android.com/reference/androidx/webkit/WebViewAssetLoader | ||
| String htmlPath = WebViewUtils.MAGIC_DEFAULT_DOMAIN + "/data/" + KMManager.KMFilename_KeyboardHtml; |
There was a problem hiding this comment.
| String htmlPath = WebViewUtils.MAGIC_DEFAULT_DOMAIN + "/data/" + KMManager.KMFilename_KeyboardHtml; | |
| String htmlPath = WebViewUtils.buildAssetUrl(KMManager.KMFilename_KeyboardHtml); |
or
| String htmlPath = WebViewUtils.MAGIC_DEFAULT_DOMAIN + "/data/" + KMManager.KMFilename_KeyboardHtml; | |
| String htmlPath = WebViewUtils.buildAssetUrl([KMManager.KMFilename_KeyboardHtml]); |
| // Use the reserved magic domain for loading the keyboard from the local device. | ||
| // See https://developer.android.com/reference/androidx/webkit/WebViewAssetLoader |
There was a problem hiding this comment.
This comment can just go in WebViewUtils.java, buildAssertUrl()
| * internal storage path. | ||
| * See https://developer.android.com/reference/androidx/webkit/WebViewAssetLoader | ||
| */ | ||
| public static final String MAGIC_DEFAULT_DOMAIN = "https://appassets.androidplatform.net"; |
There was a problem hiding this comment.
| public static final String MAGIC_DEFAULT_DOMAIN = "https://appassets.androidplatform.net"; | |
| private static final String MAGIC_DEFAULT_DOMAIN = "https://appassets.androidplatform.net"; | |
| /** | |
| * Path under the asset domain where all assets live | |
| */ | |
| public static final String ASSET_DATA_PATH = "/data/"; | |
| /** | |
| * Build a full URL to the provided asset | |
| */ | |
| public String buildAssetUrl(String assetPath) { | |
| return WebViewUtils.MAGIC_DEFAULT_DOMAIN + WebViewUtils.ASSET_DATA_PATH + assetPath; | |
| } |
or
| public static final String MAGIC_DEFAULT_DOMAIN = "https://appassets.androidplatform.net"; | |
| private static final String MAGIC_DEFAULT_DOMAIN = "https://appassets.androidplatform.net"; | |
| /** | |
| * Path under the asset domain where all assets live | |
| */ | |
| public static final String ASSET_DATA_PATH = "/data/"; | |
| /** | |
| * Build a full URL to the provided asset, joining all components with '/' | |
| */ | |
| public String buildAssetUrl(String[] assetPath) { | |
| return WebViewUtils.MAGIC_DEFAULT_DOMAIN + WebViewUtils.ASSET_DATA_PATH + String.join("/", assetPath); | |
| } |
This PR migrates the Android WebView from loading local files via file:// URLs to using
WebViewAssetLoader, which serves internal storage files through a magic HTTPS domain (https://appassets.androidplatform.net). This fixes the blank keyboard problem reported in #16096 for Android.Core pattern of the change:
The
WebViewAssetLoaderinKMKeyboardWebViewClientintercepts requests to this domain and serves files from internal storage. Since the domain is a constant,Contextis no longer needed to construct URLs, simplifying several method signatures.Also this PR makes
KMKeyboard.getKeyboardRoot()private, and renames publicKeyboard.getKeyboardPath()to privateKeyboard.getKeyboardUrl().Part-of: #16096
Replaces: #16132