Skip to content

fix(ios): load host page, keyboards through a consistent WKURLSchemeHandler#16136

Draft
jahorton wants to merge 1 commit into
fix/ios/bundle-globe-hintfrom
fix/ios/load-via-scheme-handler
Draft

fix(ios): load host page, keyboards through a consistent WKURLSchemeHandler#16136
jahorton wants to merge 1 commit into
fix/ios/bundle-globe-hintfrom
fix/ios/load-via-scheme-handler

Conversation

@jahorton

@jahorton jahorton commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

These changes bypass any need to modify KMW keyboard loading (say, via .fetch), instead forcing all engine files and resources to load via the new scheme/protocol in order to accomplish its goal.

Alternate approaches considered:

  • monkeypatching fetches on file:// protocol calls to ping Swift for a file load, then pass the results back via JS call that resolves the associated fetch Promise.
    • Would require fewer changes, but would require that one point of monkeypatching.

Build-bot: skip release:ios,web

User Testing

TEST_IOS:

  • Install Keyman for iPhone and iPad from this PR
  • Verify that a keyboard properly shows
  • Verify that the predictive-text banner shows suggestions (use a keyboard/language with an associated lexical model)

TEST_WEB:

  • Open KeymanWeb test page, navigate to any test
  • Select a keyboard and verify that it displays the OSK and that the console window doesn't show errors

TEST_WEB_KBD_DOCS:

  • Open KeymanWeb test page titled "Tests keyboard documentation rendering"
  • Verify that all keyboard renders load correctly and lack any obvious visual errors.

@github-project-automation github-project-automation Bot moved this to Todo in Keyman Jun 24, 2026
@keymanapp-test-bot keymanapp-test-bot Bot added the user-test-missing User tests have not yet been defined for the PR label Jun 24, 2026
@keymanapp-test-bot

keymanapp-test-bot Bot commented Jun 24, 2026

Copy link
Copy Markdown

User Test Results

Test specification and instructions

  • TEST_IOS (OPEN)
  • TEST_WEB (OPEN)
  • TEST_WEB_KBD_DOCS (OPEN)
Results Template
# Test Results

* **TEST_IOS (OPEN):** notes
* **TEST_WEB (OPEN):** notes
* **TEST_WEB_KBD_DOCS (OPEN):** notes

Test Artifacts

@mcdurdin mcdurdin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I like where this is going, some feedback and thoughts on how to tighten it up.

sourcePath = addDelimiter(sourcePath);
this.sourcePath = sourcePath;
this.protocol = sourcePath.replace(/(.{3,5}:)(.*)/,'$1');
this.protocol = sourceURL.protocol;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sometimes a little bit of cleanup just makes things so much prettier

Comment thread web/src/engine/src/interfaces/pathConfiguration.ts Outdated
Comment thread web/src/engine/src/interfaces/pathConfiguration.ts Outdated
}

updateFromOptions(pathSpec: Required<PathOptionSpec>) {
const _rootPath = this.sourcePath.replace(/(https?:\/\/)([^\/]*)(.*)/,'$1$2/');

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Will this also need fixup?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not exactly sure what it's doing, and I didn't need to change it, so I decided to leave this be.


// Local function to convert relative to absolute URLs
// with respect to the source path, server root and protocol
fixPath(p: string) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could this whole function be replaced with new URL(p, this.sourcePath)?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'd want to test that out more thoroughly as its own PR, rather than rolling it in here.

Comment thread ios/engine/KMEI/KeymanEngine/Classes/Keyboard/WebViewKeyboardLoader.swift Outdated
Comment on lines 24 to 37
@@ -36,12 +36,14 @@
```
*/

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
*/

Legacy comment!

Comment thread ios/engine/KMEI/KeymanEngine/Classes/Keyboard/WebViewKeyboardLoader.swift Outdated
Comment thread ios/engine/KMEI/KeymanEngine/Classes/Keyboard/WebViewKeyboardLoader.swift Outdated
// +1: include the ':' that likely exists
pathComponent = String(components.path.dropFirst(KeymanWebViewController.SCHEME.count + 1))
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm confused about this piece of code. Surely pathComponent should never include keyman-engine:? That would be components.scheme?

Also, I think this would be a good place to collapse repeated slashes, both at start of string and beyond, because repeated slashes when building paths is a common bug and I think we can be resilient to it without real penalty.

That also removes the need to do that work in pathConfiguration.ts.

pathComponent = components.path.replacingOccurrences(of: "/+", with: "/", options: .regularExpression)

if pathComponent.hasPrefix("/") {
  pathComponent = pathComponent.dropFirst()
}

Something like that?

@jahorton jahorton Jun 25, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This was an earlier attempt to work around issues in which URLs like the following appeared: keyman-engine:/keyman-engine:/kmwosk.css

It didn't work, which led to resolving things in Web engine code - the changes to pathConfiguration.ts, oskView.ts, and visualKeyboard.ts.

Will clean up now.

That also removes the need to do that work in pathConfiguration.ts.

pathComponent = components.path.replacingOccurrences(of: "/+", with: "/", options: .regularExpression)

if pathComponent.hasPrefix("/") {
  pathComponent = pathComponent.dropFirst()
}

... about that. If there are repeated leading slashes, URLComponents will consider the 'path' as the host, not a path! For most of our resources, this then results in a nil path, which is very much something we don't want. Some of the changes seen in pathConfiguration.ts were made b/c they're the easiest workaround... or were at the time.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

... and fortunately, the fixes in OSK code resolved the cases that were causing this, so we can still knock the pathConfiguration.ts double-slash handling out!

@mcdurdin

Copy link
Copy Markdown
Member

One more thought; can we base this on #16132 so that we are looking at a consolidated fix for both platforms?

@jahorton

Copy link
Copy Markdown
Contributor Author

One more thought; can we base this on #16132 so that we are looking at a consolidated fix for both platforms?

Work is still ongoing there, and there's been at least one force-push to that branch since the quoted comment was posted. I'd have to re-force-push this branch every time a force-push happens there.

@jahorton jahorton force-pushed the fix/ios/load-via-scheme-handler branch from 2e9568e to b9a3c11 Compare June 25, 2026 15:47
@keymanapp-test-bot 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 Jun 25, 2026
@jahorton jahorton force-pushed the fix/ios/load-via-scheme-handler branch from b9a3c11 to 1f99b2d Compare June 25, 2026 16:07
…andler

This bypasses any need to modify KMW keyboard loading (say, via .fetch), though it does require a number of collateral changes to be made in order for CORS, etc to be satisfied.

Build-bot: skip build:ios
jahorton added a commit that referenced this pull request Jun 25, 2026
Pretty much just what the title says; it's been silently missing this whole time.

This doesn't fix iOS keyboard's display by itself, but it is a prerequisite for the full solution offered by #16136.

Build-bot: skip build:ios
Test-bot: skip
jahorton added a commit that referenced this pull request Jun 25, 2026
Pretty much just what the title says; it's been silently missing this whole time.

This doesn't fix iOS keyboard's display by itself, but it is a prerequisite for the full solution offered by #16136.

Build-bot: skip build:ios
Test-bot: skip
@jahorton jahorton changed the base branch from master to fix/ios/bundle-globe-hint June 25, 2026 18:23
@jahorton jahorton force-pushed the fix/ios/load-via-scheme-handler branch from 1f99b2d to 08c2f25 Compare June 25, 2026 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Todo

Development

Successfully merging this pull request may close these issues.

2 participants