-
-
Notifications
You must be signed in to change notification settings - Fork 111
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
fix(ios): multitap consistency after new-lines #10728
Conversation
User Test ResultsTest specification and instructions
Test Artifacts
|
Test Results
|
@bharanidharanj I'm going to have to ask for video on this one. I'm not currently able to reproduce it, and the applied suggestion is highlighted in your screenshot above. There's an admittedly small chance of this, but the possibility exists that I'd need to replicate what you did quite precisely to get the same effects, and video would be, by far, the most convenient way to provide that. As for the other bits (sticky highlighting, period subkey menu), I've gone ahead and merged in recent changes that may be related, so... on that basis... @keymanapp-test-bot retest TEST_SHIFTED_CONTEXT_MULTITAP_ANDROID TEST_NEW_PARAGRAPH_PREDICTIONS See my previous comment re: the Android-oriented redo request. Also note that the sticky-highlighting issue is being handled by a separate PR that is not yet merged - #10728. Do not report sticky-highlighting errors for keys or fail these tests on that basis. Do report sticky highlighting for suggestions if that occurs, though. |
@jahorton Oops! Sorry for the confusion. Yes. I have tested this in my Android Mobile (ver 13) device. Sure. I will add a video file for reference. Thanks. |
Test Results
Multitapfn.mp4 |
Test Results
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think LGTM. I don't really understand all the tendrils here -- there's a whole lot of complexity both in the original code and in the changes, and I don't think I have a full handle on all the interactions.
Upon request, I suppose I could create a highly-focused test keyboard for validating such a case. That said, it does seem pretty niche - I don't think we currently have a keyboard with such rules. (As a result, I haven't prioritized testing this scenario.) Part of me wonders if the best course of action would be to spin this concern off into a separate issue so that it doesn't hold this PR up from fixing a lot of cases that are far more commonly seen.
Yes, sounds sensible, probably turn it into an issue.
var end = pos + length; | ||
// undefined + <number> => NaN. | ||
end = isNaN(end) ? undefined : end; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
var end = pos + length; | |
// undefined + <number> => NaN. | |
end = isNaN(end) ? undefined : end; | |
const end = start === undefined ? undefined : start + length; |
We probably shouldn't be using var
any longer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will fix the var
bit.
The comment isn't explicit about it, but both pos
and length
can be undefined
. The same property holds if both are undefined at the same time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, that's pretty ugly and side-effecty. Let's make sure we add a comment to that effect.
keyman.resetContext(); | ||
} | ||
|
||
return keyman.context.getText(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this never used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Here's the only thing calling it:
keyman/ios/engine/KMEI/KeymanEngine/Classes/Keyboard/KeymanWebViewController.swift
Line 249 in 66142be
webView!.evaluateJavaScript("setKeymanVal(\"\(jsonText)\");", completionHandler: nil) |
Note how nothing is checking for a result. To be extra clear here, that would be done by the second argument if one were provided.
https://developer.apple.com/documentation/webkit/wkwebview/1415017-evaluatejavascript
No callback means no need for a return value.
ios/engine/KMEI/KeymanEngine/resources/Keyman.bundle/Contents/Resources/ios-host.js
Outdated
Show resolved
Hide resolved
ios/engine/KMEI/KeymanEngine/resources/Keyman.bundle/Contents/Resources/ios-host.js
Outdated
Show resolved
Hide resolved
…e-into-multitap' into fix/ios/context-slide-into-multitap
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
I've made a basic issue for this at #10761. |
Changes in this pull request will be available for download in Keyman version 17.0.271-beta |
Fixes #10696.
Continues from #10584 and #10633.
Now that we're synchronizing app context with the internal web engine's context more proactively - which gives us more consistent backspace behavior - we need to ensure that this doesn't break preservation of context going forward, as new text is added.
Commit 1
Simplified:
Internal details:
Fixes context synchronization for all cases except a multitap immediately following a
textDocumentProxy
context-window slide.Ensures that all context manipulations for each tap of a multitap requires only a single call when updating host-app context.
Commit 2
Simplified
Internal details:
textDocumentProxy
context window jumps that result as new text is added (without other changes) do not trigger a context reset within the internal web engine.textDocumentProxy
quirks.textDocumentProxy
context window returns to text that was once "in window" but had since slid out of it.There is a chance that an extreme edge case might still have issues:
a\n
+b
=>b\n
.)textDocumentProxy
perform a context-window jump as a result... and also for the tap after it, which would need to make counter-edits to revert such a rule's change, as such a tap would likely retrigger atextDocumentProxy
update.Upon request, I suppose I could create a highly-focused test keyboard for validating such a case. That said, it does seem pretty niche - I don't think we currently have a keyboard with such rules. (As a result, I haven't prioritized testing this scenario.) Part of me wonders if the best course of action would be to spin this concern off into a separate issue so that it doesn't hold this PR up from fixing a lot of cases that are far more commonly seen.
User Testing
TEST_LONG_DELETION: Using the iOS Keyman app on an iPhone 13 / iPhone 13 Pro, delete a lot of pre-existing text from within a third-party application. (Simulator is fine, but stick to iPhone 13 targets.)
Testing testing
...")This test can be done on Simulator by using the Messages app instead and selecting one of the sample contacts. The Notes app is typically not available for Simulator-based iOS devices. (Notes doesn't come with the risk of butt-texting, though.)
TEST_SHIFTED_CONTEXT_MULTITAP_IOS: Using the iOS Keyman app on an iPhone 13 / iPhone 13 Pro, verify that multitaps at the start of new paragraphs do not have unwanted side effects. (Simulator is fine, but stick to iPhone 13 targets.)
8
key.a
a
.9
key.aê
ê
.8
key.aềa
à
's accent-mark on top the originalê
.Note: this PR does not include the fix from #10589; things will break here if a diacritic is the first entry on a line. This is known and expected.
TEST_SHIFTED_CONTEXT_MULTITAP_ANDROID: Using the Keyman for Android app, verify that multitaps at the start of new paragraphs do not have unwanted side effects.
8
key.a
a
.9
key.aê
ê
.8
key.aềa
à
's accent-mark on top the originalê
.TEST_NEW_PARAGRAPH_PREDICTIONS: Using the iOS Keyman app on an iPhone 13 / iPhone 13 Pro, verify that predictive text works properly at the start of new paragraphs.
Note: this PR does not include the fix from #10589; things will break here if a diacritic is the first entry on a line. This is known and expected.