keymanapp / keyman

Keyman cross platform input methods system running on Android, iOS, Linux, macOS, Windows and mobile and desktop web
https://keyman.com/
Other
372 stars 102 forks source link

bug(ios): Backspace is not predictable #10204

Closed chandrab699 closed 3 months ago

chandrab699 commented 5 months ago

Describe the bug

Backspace stops randomly at newline, white space, ?,

Remedy is to reposition the cursor to the same location, backspace work start functioning again

Reproduce the bug

No response

Expected behavior

No response

Related issues

No response

Keyman apps

Keyman version

17.0.227

Operating system

iOS 17.1.3

Device

iPhone 13 Pro max

Target application

Any

Browser

No response

Keyboard name

Sil_EuroLatin

Keyboard version

2.0.4

Language name

English

Additional context

https://github.com/keymanapp/keyman/assets/30884806/873e21cb-8ab0-4566-8595-bb24c6b0e585

MayuraVerma commented 5 months ago

It might be happening due to AutoCapitalization feature

You can see CAPS lock triggering while passing the white space with backspace

MayuraVerma commented 5 months ago

Backspace gets stuck when the cursor to the right for space or question mark

sgschantz commented 5 months ago

Reproduced with iPhone XS Max and iOS 17.2.1 and EuroLatin in both Notes and Mail Does not happen all the time, but when it is happening, it happens consistently for that editing session. When it is not happening, then backspace works fine.

I could not determine why backspace behaves as expected for some editing sessions and not for others.

Expected behavior: Holding down the backspace key should repeatedly delete characters to the left of the insertion point without stopping.

Actual behavior (sometimes): When the insertion point is inserted following some text and the backspace key is held down, it begins to delete characters, but when it reaches a newline Keyman stops deleting. Repeated presses on the delete key have no effect. To continue deleting again, the insertion point must be moved to another location.

Keyman also stops deleting when it encounters a question mark '?' followed by a space. The deleting is suspended before it deletes the space. The space character alone is not enough to stop the deletion but must be paired with a question mark. Again, the insertion point must be moved to continue deleting again.

Also note that when deleting is suspended, it is possible to type other characters. After doing so, these characters can immediately deleted using backspace, but will halt at the same point before the other characters were typed.

jahorton commented 4 months ago

This strongly reminds me of some odd things we've had to work around for context management. If memory serves, Apple likes to tell us that the context - the part of your text that the keyboard is allowed to see and edit - is empty when backspace is held and lands at the start-point of what was once a paragraph.

Actual behavior (sometimes): When the insertion point is inserted following some text and the backspace key is held down, it begins to delete characters, but when it reaches a newline Keyman stops deleting. Repeated presses on the delete key have no effect. To continue deleting again, the insertion point must be moved to another location.

Keyman also stops deleting when it encounters a question mark '?' followed by a space. The deleting is suspended before it deletes the space. The space character alone is not enough to stop the deletion but must be paired with a question mark. Again, the insertion point must be moved to continue deleting again.

Also note that when deleting is [suspended], it is possible to type other characters. After doing so, these characters can immediately deleted using backspace, but will halt at the same point before the other characters were typed.

This precisely describes the behavior that results under the condition I described above. At present, when we think we're done deleting, we do not send extra backspaces through to the app - this is (in part) because a keyboard's backspace rule may function differently than what Apple's default backspace would do. (Without seeing the text, there's no way for the keyboard to properly detect that issue should it arise.) We want to make sure such cases are handled correctly... even if Apple isn't making it easy.

Remedy is to reposition the cursor to the same location, backspace work start functioning again

This also matches what I remember: repositioning the cursor causes iOS to refresh the context it lets the keyboard see, rather than continuing to update the context's previous version.

mcdurdin commented 4 months ago

This precisely describes the behavior that results under the condition I described above. At present, when we think we're done deleting, we do not send extra backspaces through to the app - this is (in part) because a keyboard's backspace rule may function differently than what Apple's default backspace would do.

This does not match behaviour on other platforms. When we have no known context, we emit the backspace event to the app, so that the app can use that to, for example, move to a previous field.

jahorton commented 4 months ago

This does not match behaviour on other platforms. When we have no known context, we emit the backspace event to the app, so that the app can use that to, for example, move to a previous field.

I never argued that it did. I'm simply leaving a note about highly-related architectural issues that any solution will have to handle.

mcdurdin commented 4 months ago

This does not match behaviour on other platforms. When we have no known context, we emit the backspace event to the app, so that the app can use that to, for example, move to a previous field.

I never argued that it did. I'm simply leaving a note about highly-related architectural issues that any solution will have to handle.

I don't understand what you want out of this then?

We want to make sure such cases are handled correctly...

What does 'correctly' look like to you?

jahorton commented 3 months ago

One thing worth looking into: I don't think we refresh context at present after a number of backspaces... enough so that the context window could have easily shifted to include information that was once "too far back" in the view of the TextDocumentProxy provided by iOS. Essentially... a kind of "context desync" that we never thought to handle. Admittedly, most of the times I ran into this sort of issue... I'd deleted a very significant amount of text from the caret's paragraph before reaching the preceding newline - something that would easily be suspectible to this hypothesized oversight.

If my new hunch here is correct, perhaps what we need is some sort of "update context window" operation - one that doesn't reset deadkeys but allows us to extend the context when intermediate text is deleted.


Alternatively, but less ideally: we could use the existing special-case char \uFFFE (our nul-rule sentinel) as a "context extension" within Web, adding a few as backspace-deletion targets. This strategy could still work even if the iOS window into the context doesn't update in accordance with the hunch above.

Alternatively from that, there's always the potential to add a special-case for key event logic as such:

jahorton commented 3 months ago

Investigation of that hypothesis:

Ref on the "not if we caused it" part:

After inserting a logging statement in our implementation... yep, there's no context-update call during standard keystrokes. We also prevent a context reset when we do trigger the method by reaching a paragraph start.

If we did an update as part of the standard insertText handling (where the Swift code updates to what KMW has processed), that does trigger after every context-affecting keystroke.

jahorton commented 3 months ago

Of course, for what we're talking about here... to update context as the TextDocumentProxy window slides... we'll run into something very close to parts of #5258. The same code we'd need to "realign" before/after suggestion/reversion use is likely very similar code - if not the same code - we'll need to realign "before" and "after" context in order to preserve deadkeys as the context-window slides.

The trickiest part, looking at this in isolation, is that we need to avoid triggering an outright context-reset - as that would obliterate any deadkeys. The context could potentially slide after each keystroke, and any time this occurs, incorrect behavior will happen whenever/wherever deadkeys are expected.

Unfortunately, #5258 is (currently) not even on our roadmap, with a note about "design work needed." So... we'll likely want to bump up its time-table... or we'll want an intermediate stopgap.

jahorton commented 3 months ago

In an in-person discussion, it was mentioned that Core has a setContextIfNeeded method that had a few related PRs... and also potential unit tests and user tests that could be useful for testing likely solutions. That method serves a fairly similar purpose - post-keystroke context-resynchronization without removal of deadkeys.

@mcdurdin, any good spots / tests I should make sure to reference when the time comes? Links to the PRs that introduced them would be sufficient, as would permalinks to current implementations of the function and its related tests. (PRs are most useful re: user tests, of course.) I figure a couple of links for either type should be enough of a lead; I should be able to take it from there via git blame and similar tooling.

Then again, I can probably chase it outright, now that I've documented this note on the issue. Just needed to make sure I didn't forget the "lead" that arose during that in-person discussion.

jahorton commented 3 months ago

Simulator reproduction!

Useful text block:

Testing testing one two three one two three

Even more random text typed here. Lots and lots and lots of text. Like, way too much text. I want to trigger a context window, and we're going to make certain of it.

And yeah, another one to be safe. Gonna start within this one, so I can fall back on the last one and see if it triggers across THOSE boundaries. That could totally happen, right?
  1. On a Mac with Xcode and Simulator available...
  2. Run $KEYMAN_ROOT/ios/build.sh --debug
  3. Open Xcode, then run the Keyman target within the Simulator.
  4. Once it fully loads, ensure that Keyman is enabled as a system keyboard.
  5. Terminate the Keyman app from Xcode.
  6. Within Simulator, open the Messages app and pick the first available contact.
  7. Copy the "Useful text block" above, then paste it as a message within the app.
  8. In Xcode, change the build target to SWKeyboard and launch it. Select the Messages app.
  9. Within Simulator, open the keyboard by selecting the pasted message.
  10. Move the caret to the end of the text block.
  11. If not currently active, swap to the Keyman keyboard.

With custom logging added... then typing a single backspace:

Screen Shot 2024-01-31 at 2 21 24 PM

Note that the most recently-deleted character still appears within the log message - the message is logged before we apply the changes from KMW.

After a number of extra backspaces...

Screen Shot 2024-01-31 at 2 22 27 PM

And, surprisingly...

Screen Shot 2024-01-31 at 2 23 53 PM

Wait, the window iOS gives us doesn't slide!? I can confirm that even when we reach an empty view, the context window still refuses to slide (by placing the logging after the update, rather than before).

Accordingly, all further backspaces after this point have no effect... unless we change the caret's position.

jahorton commented 3 months ago

Attempting to use this at the time of logging:

textDocumentProxy.adjustTextPosition(byCharacterOffset: -1)
textDocumentProxy.adjustTextPosition(byCharacterOffset:  1)

does not force an update of the textDocumentProxy context window. In fact...

textDocumentProxy.adjustTextPosition(byCharacterOffset: -1)

// Actual logging calls here

textDocumentProxy.adjustTextPosition(byCharacterOffset:  1)

still doesn't force a context refresh, even with the caret theoretically moved beyond the context window's boundaries.


What does trigger a context refresh: actually deleting a character on the boundary.

if preCaretContext == "" {
  textDocumentProxy.deleteBackward()
}

But at that point... we've lost knowledge about that one pre-boundary character we had to delete. The method's return type is void, so we can't get the character that way. The best we can do is hope Apple has a decent default backspace in these scenarios.

... And so it turns out, the textDocumentProxy context window doesn't slide - it jumps, only moving when an actual edit moves past a boundary.

jahorton commented 3 months ago

Since this wasn't covered yet, what happens as we add text?

Screen Shot 2024-01-31 at 3 12 56 PM

Any added text appears to remain within the window, growing it rather than rotating out. Perhaps if I kept at it, I'd reach a point where it eventually transitions, but note how the context window is significantly larger than the initial one we started with - and this stays the case even after several more extra lines.

jahorton commented 3 months ago

So, based on these observations... the best we can likely do is to rely on Apple's default backspace handling when we would want to backspace through a context window boundary. After that backspace, we'd get context that has no associated deadkeys (as it was too far back to have any), so a standard resetContext should be sufficient at that point.

Then, receiving a backspace within Web that gets processed to a 'default' + "no manipulations" - essentially, null transform - should in some way signal to trigger a default Apple backspace.

This also raises concerns should a keyboard have special backspace handling rules structured in a manner that would trigger backspaces to cross the context window boundary, as we rely on the context window remaining stationary in our character-deletion portion of the update. Granted, I don't believe it's actually possible to trigger such a case from a Developer-compiled web keyboard, since to delete something from context, it has to be matched first - and that "match" would be impossible to fulfill if across the context-window boundary.

jahorton commented 3 months ago

Trying this out on my personal iPhone SE 2, the repro does trigger... just at a different position:

Testing testing one two three one two three

Even more random text typed here. Lots and lots and lots of text. Like, way too much text. I want to trigger a context window, and we're going to make certain of it.

And yeah, another one to be safe. |

With | as the caret position where further backspaces stop working.

jahorton commented 3 months ago

Attempting this with our Android system keyboard, within its Messages app via Android Studio emulator, I was able to delete the full block of text in one go.

Just to confirm that we don't have this issue on Android, rather than it being something that [somehow] never got reported.

jahorton commented 3 months ago

... one thing I haven't tried yet:

  1. Move position back one or two chars from the context-window's leading boundary.
  2. [!] Emit a sentinel value: perhaps \uFFFE or \uFFFF
  3. Now read the context.
  4. [!] Find and remove the sentinel value
  5. Restore original caret position.

The idea: perhaps we can emit a character outside the boundary to force an early context-window update?

mcdurdin commented 3 months ago

@mcdurdin, any good spots / tests I should make sure to reference when the time comes?

mcdurdin commented 3 months ago

What does trigger a context refresh: actually deleting a character on the boundary.

if preCaretContext == "" {
  textDocumentProxy.deleteBackward()
}

But at that point... we've lost knowledge about that one pre-boundary character we had to delete. The method's return type is void, so we can't get the character that way. The best we can do is hope Apple has a decent default backspace in these scenarios.

... And so it turns out, the textDocumentProxy context window doesn't slide - it jumps, only moving when an actual edit moves past a boundary.

and

So, based on these observations... the best we can likely do is to rely on Apple's default backspace handling when we would want to backspace through a context window boundary. After that backspace, we'd get context that has no associated deadkeys (as it was too far back to have any), so a standard resetContext should be sufficient at that point.

Then, receiving a backspace within Web that gets processed to a 'default' + "no manipulations" - essentially, null transform - should in some way signal to trigger a default Apple backspace.

This also raises concerns should a keyboard have special backspace handling rules structured in a manner that would trigger backspaces to cross the context window boundary, as we rely on the context window remaining stationary in our character-deletion portion of the update. Granted, I don't believe it's actually possible to trigger such a case from a Developer-compiled web keyboard, since to delete something from context, it has to be matched first - and that "match" would be impossible to fulfill if across the context-window boundary.

Well, that's good news, great work on diagnostics.

Emitting a backspace to the app/OS is precisely what we should be doing when we have a delete-back on an empty context, to give the app the opportunity to e.g. backspace through multiple text cells. So implementing this would mean that we would match desktop platform behaviour when backspacing on 'unknown' or empty context.

We won't have any cached markers/deadkeys beyond that boundary so we won't be losing any state.

From my perspective, I am not very concerned about not triggering backspace rules across the boundary. Any difference in behaviour is likely to be imperceptible to the end user, particularly since this really only comes up when deleting large blocks of text. I don't know how Apple calculates these boundaries, but there's a reasonable chance that they are on grapheme cluster boundaries or similar anyway, which would further mitigate any possible concerns.

The best we can do is hope Apple has a decent default backspace in these scenarios.

Noting that 99% of our keyboards have no special backspace handling anyway so Apple's default is more than adequate (and better than our default for emoji...)

jahorton commented 3 months ago

And, surprisingly...

Screen Shot 2024-01-31 at 2 23 53 PM

Note that there is something subtle here that's an extra complication. Because our "app context" appears to have been emptied at this point, the keyboard's rules have decided that this scenario calls for "start of sentence" behavior - it has displayed the 'shift' layer, rather than staying on the 'default' layer.

Of course, this scenario is "less bad" than outright-refusal to output backspaces, so getting a fix for the latter takes priority over waiting for perfect polish here.

mcdurdin commented 3 months ago

Of course, this scenario is "less bad" than outright-refusal to output backspaces, so getting a fix for the latter takes priority over waiting for perfect polish here.

Agree, not ideal. And yes, agree, let's get the backspace refusal sorted and then come back to this.

It's weird that apple's API isn't giving us good data for context, and perhaps there is a way to poke it that isn't too finicky. Worth opening a bug report for Apple (or are we missing something)?

jahorton commented 3 months ago

Well, I found "a way" to poke it, but it might be too finicky:

  // Pre-condition:  no text is selected.  As this is currently only called by `insertText`
  // below, this condition is met.
  func sendContextUpdate() {
    let preCaretContext = textDocumentProxy.documentContextBeforeInput ?? ""
    let postCaretContext = textDocumentProxy.documentContextAfterInput ?? ""

    if preCaretContext == "" {
      textDocumentProxy.adjustTextPosition(byCharacterOffset: -1)
      // The following call MUST NOT occur synchronously with the prior call.
      // If it does, the `textDocumentProxy` will not update its context window.
      DispatchQueue.main.async {
        // Note: if context is the TRUE context and is a|bc before a triggering deletion, 
        // thus is |bc now, this WILL result in b|c after, even if we don't have visible
        // context at the moment.
        self.textDocumentProxy.adjustTextPosition(byCharacterOffset: 1)

        // Our context window hasn't yet updated.  `TextDocumentProxy`, as used
        // by the system keyboard, involves Inter-Process Commmunication - an
        // inherently async process... and it appears that there's no internal
        // forced-sync waiting imposed by the API in regard to context-window
        // management.
      }

      // 33ms seemed sufficient.  Can we go lower? (~30 Hz rate)
      // Success with 20ms. (50 Hz rate)
      // 1ms is not sufficient, nor is 10ms.   :(
      // Note:  these notes were taken via Simulator, not on a physical device;
      // there's no guarantee (yet) that the times will be the same.
      // But something refresh-rate related is a fairly reasonable assumption.
      DispatchQueue.main.asyncAfter(deadline: .now() + 0.033) {  // contrast:  held backspace - every 0.1.
        let currentPostContext = self.textDocumentProxy.documentContextAfterInput ?? ""

        if currentPostContext != postCaretContext {
          // We probably reached the TRUE beginning of context, meaning the initial
          // relocate by -1 failed.  (The b|c situation.) So, we need to undo the +1 that DID succeed.
          //
          // Side-effect:  the caret _will_ visibly "jitter" due to our caret-manipulation shenanigans here.
          // We'd probably want a way to note "context just checked" to prevent the jitter from happening
          // more than once at the TRUE start of context.
          self.textDocumentProxy.adjustTextPosition(byCharacterOffset: -1)
        }
        // Does NOT update after half a second if there's no context manipulation.
        let preCaretAsyncContext = self.textDocumentProxy.documentContextBeforeInput ?? ""
        let postCaretAsyncContext = self.textDocumentProxy.documentContextAfterInput ?? ""

        os_log("Async context after 33ms delay: %@|%@", log: KeymanEngineLogger.engine, type: .debug, preCaretAsyncContext, postCaretAsyncContext)
      }
    }

    os_log("Context (sync): %@|%@", log: KeymanEngineLogger.engine, type: .debug, preCaretContext, postCaretContext)
  }

With the code as-is above, the "async context" log returns correct context.

jahorton commented 3 months ago

Just double-checked, as I added the async aspects later in investigation - the adjustTextPosition calls are still necessary. Removing them and keeping the rest the same will not cause the textDocumentProxy to refresh its context. Neither will inserting custom text that would fall within the window.

Finally, https://developer.apple.com/documentation/uikit/uikeyinput/1614457-hastext does not track whether the source object has existing, out-of-window context. It's completely dependent on what's within the context window, unfortunately - it'll say "no text" whenever this error triggers, despite there being plenty of (out-of-window) text available.