Hitting suppr causes DeltaTextInputClient to send incorrect TextEditingDeletion on Web platform

[amantoux]

Steps to Reproduce

1. Run https://flutter.github.io/samples/web/simplistic_editor/#/ in a browser
2. Set cursor on right after first character (after 'T')
3. Hit suppron keyboard

Expected results: Deletion Delta Offset = (1,2)

Actual results: Deletion Delta Offset = (0,1)

Code sample: https://github.com/flutter/samples/blob/main/simplistic_editor

[darshankawar]

Transferring this to samples github repo.

[domesticmouse]

PTAL @Renzo-Olivares

[amantoux]

Transferring this to samples github repo.

@darshankawar

Nothing is wrong with the sample code

The issue is with DeltaTextInputClient when run on web platform

It's complicated to setup working example to demonstrate the problem but it can be easily reproduced with the sample

[amantoux]

Can you please have a look? This is blocking our project

[Renzo-Olivares]

@amantoux Thank you, I'll take a look. I'm able to reproduce this on my end.

[cgestes]

btw backspace doesn't work correctly either on the web, in the example deleting multiples chars in a row with backspace make the text alternates between two differents values.

deleting "brown", I get:
The quick rw fox jumps over the lazy dog.

if I click anywhere on the text it alternates with:
The quick bo fox jumps over the lazy dog.

If I select some text, the alternation stops.

[bleroux]

While learning the inner-workings of Flutter text editing, I have investigated this issue.

It is reproducible on Desktop too (Linux on my side, I have yet to test it on other platforms). To repro on Linux, I run the updated version of simplistic_editor from #1447.
(@cgestes Thank you for reporting #1424 (comment). Can you filed a new issue in flutter/samples for it? This way we can investigate it too and determine if it is a flutter/samples or flutter/flutter issue).

From my understanding, the sample code doesn’t make a distinction between Delete and Backspace which leads to this issue. One way to fix this in the sample code could be to rely on the DeleteCharacterIntent.forward parameter to determine if the deletion is backward or forward.
It would lead to something like this :

    DeleteCharacterIntent: CallbackAction<DeleteCharacterIntent>(
      onInvoke: (intent) => _delete(intent.forward),
    ),
   // And update _delete method to use this parameter to remove the right character

@Renzo-Olivares As you have a deep understanding of this, is relying on DeleteCharacterIntent.forward the expected fix? Or is this issue related to a wrong delta offset sent by the engine? (I can file a PR in both cases).

[Renzo-Olivares]

@cgestes I think that issue might be isolated to this demo.

@bleroux On the web side, the engine is sending the wrong delta offset (this sample ignores shortcuts for web). On the desktop side of things, the fix you described will work since LogicalKeyboardKey.delete is handled in DefaultTextEditingShortcuts https://github.com/flutter/flutter/blob/18a827f3933c19f51862dde3fa472197683249d6/packages/flutter/lib/src/widgets/default_text_editing_shortcuts.dart#L266.

[bleroux]

@Renzo-Olivares Many thanks for your insights, it helps me go deeper into this investigation.

I found that even for Web the delete key is handled by a Shortcut. This Shortcut is registered by WidgetApp which uses DefaultTextEditingShortcuts. The Shortcuts widget added by the sample app (in BasicTextInputClientState.build) does not override the default shortcuts so those shortcuts are enabled (I think the Shortcuts documentation could be improved to better states this, I will consider filing a PR).

So, for both Web and desktop, BasicTextInputClientState._delete is called and the text delta displayed in this sample app is not the one sent by the engine but the one built inside the _delete method.

Do you think that DefaultTextEditingShortcuts should be overridden in this sample app? It really depends on what we want to demonstrate in this sample.

If not, this leads to a PR where BasicTextInputClientState._delete has to be correctly implemented to use the forward parameter.
If yes, the sample app will rely on the engine to send a correct text delta. I think this would be better because it’s a way to find possible bugs in the engine text deltas computation.

For instance, while running the sample app with all default shortcuts overridden, it’s way easier to determine that the engine is sending a wrong TextEditingDeltaDeletion when using the delete key. I’m working on an engine PR to fix this on the Web (and will probably work on other PRs for desktop platforms). Do you think it is worth it to work on those PR?

[Renzo-Olivares]

@bleroux Oh thats interesting, I can confirm that the delete key is handled by the shortcut. The reason for that is that we don't handle DoNothingAndStopPropagationTextIntent. It should be mapped to DoNothingAction(consumesKey: false), in our <Intent, Action<Intent>> mapping in BasicTextInputClientState. When there is no mapping, the shortcuts system searches the outer DefaultTextEditingShortcuts widget (the inner one being the disabled shortcuts) for a new handler. It will find SingleActivator(LogicalKeyboardKey.delete, shift: pressShift): const DeleteCharacterIntent(forward: true),, which is why we are seeing the _delete being called instead of receiving a delta from the platform.

When I add this mapping, I can confirm that the web is sending the wrong delta.

I think the way forward is

1. Add a mapping for DoNothingAndStopPropagationTextIntent to BasicTextInputClientState.
2. Add logic to _delete to take into account forward deletion.
3. Land a PR to fix wrong delta for delete key on web.

What do you mean by DefaultTextEditingShortcuts should be overridden in the sample app?

I think those PRs are definitely worth it to work on, and I'd be happy to review them. I think the delete key is working as expected on desktop platforms at the moment (requires an <Intent, Actions<Intent>> mapping to handle DefaultTextEditingShortcuts <ShortcutActivator, Intent>). This is how it is also handled with non-delta text input. The text input plugin does not handle certain keyboard input, which is then handled by Shortcuts instead.

[bleroux]

What do you mean by DefaultTextEditingShortcuts should be overridden in the sample app?

@Renzo-Olivares I was not aware of DoNothingAndStopPropagationTextIntent so I used the following code to “override” all shortcuts added by WidgetApp:

    // in BasicTextInputClientState.build
    return Shortcuts.manager(
      manager: ShortcutManager(
        modal: true,
        shortcuts: shortcuts,
      ),
      child: Actions(
        actions: _actions,

Your solution looks better. 😄

I think the way forward is

1. Add a mapping for DoNothingAndStopPropagationTextIntent to BasicTextInputClientState.
2. Add logic to _delete to take into account forward deletion.
3. Land a PR to fix wrong delta for delete key on web.

I think those PRs are definitely worth it to work on, and I'd be happy to review them.

Great! I will work on this as soon as possible.

[Renzo-Olivares]

This should be fixed with flutter/engine#36616 and #1458