superlistapp / super_editor

A Flutter toolkit for building document editors and readers
https://superlist.com/SuperEditor/
MIT License
1.59k stars 230 forks source link

Android - Creating a non-text node and selecting it after the text with OS IME suggestions on the keyboard causes an exception. #2070

Open rafalplonka opened 4 weeks ago

rafalplonka commented 4 weeks ago

Package Version super_editor, GitHub, stable branch

To Reproduce

  1. Place a caret on the text with keyboard suggestions on Android.
  2. Programmatically create a new node that is not a text node.
  3. Programmatically add a selection on the added node.

Minimal Reproduction Code

This issue only occurs on Android when there is already text that has suggestions (see the video for details). Adding the node doesn't cause a crash, but selecting it afterwards does.

The issue cannot be reproduced in the demo since the demo doesn’t allow for adding a node, such as a divider, while having the caret on the text element.


    editor.execute([
      InsertNodeAfterNodeRequest(existingNodeId: afterNodeId, newNode: node),
      ChangeSelectionRequest(
        DocumentSelection.collapsed(
          position: DocumentPosition(
            nodeId: node.id,
            nodePosition: const UpstreamDownstreamNodePosition.downstream(),
          ),
        ),
        SelectionChangeType.placeCaret,
        SelectionReason.userInteraction,
      )
    ]);

Actual behavior Crash


Exception: No such document position in the IME content: [DocumentPosition] - node: "edfa8ce7-9b4d-4366-8b5b-fd1e8871bb2c", position: (TextPosition(offset: 0, affinity: TextAffinity.downstream))

When the exception was thrown, this was the stack: 
#0      DocumentImeSerializer._documentToImePosition (package:super_editor/src/default_editor/document_ime/document_serialization.dart:351:7)
#1      DocumentImeSerializer.documentToImeRange (package:super_editor/src/default_editor/document_ime/document_serialization.dart:335:30)
#2      DocumentImeSerializer.toTextEditingValue (package:super_editor/src/default_editor/document_ime/document_serialization.dart:382:32)
#3      DocumentImeInputClient._sendDocumentToIme (package:super_editor/src/default_editor/document_ime/document_ime_communication.dart:273:58)
#4      DocumentImeInputClient._onContentChange (package:super_editor/src/default_editor/document_ime/document_ime_communication.dart:87:5)
#5      ChangeNotifier.notifyListeners (package:flutter/src/foundation/change_notifier.dart:433:24)
#6      ValueNotifier.value= (package:flutter/src/foundation/change_notifier.dart:555:5)
#7      PausableValueNotifier.resumeNotifications (package:super_editor/src/infrastructure/pausable_value_notifier.dart:49:11)

Expected behavior Horizontal ruler is added, selection is changed to horizontal ruler node

Platform Android

Flutter version Flutter v. 3.22.1

Screenshots https://github.com/superlistapp/super_editor/assets/18536122/51895511-c15d-4ff5-bff4-bd594e97200e

matthew-carroll commented 2 weeks ago

@snowb1shop your issue description mentions two programmatic steps.

Please include a minimal, runnable code sample that demonstrates this problem.

rafalplonka commented 2 weeks ago

Hey, thanks for the answer. 

I’ve forked the supereditor package from the main branch and created a demo. 

The commit is really small: https://github.com/superlistapp/super_editor/commit/1510658bfa3530a5922798a54700431e649c1cef


Only creating a horizontal rule node and placing the selection on it.

To find the issue run the example's main.dart demo on Android, place the caret on the paragraph text, start writing, see the suggestions from the keyboard and then click to create a horizontal rule node from the toolbar.

Link to the fork: https://github.com/snowb1shop/super_editor


void createHr() {
    final selectedNode =
        document.getNodeById(composer.selection!.extent.nodeId)! as TextNode;

    final newNode = HorizontalRuleNode(
      id: Editor.createNodeId(),
    );

    editor.execute([
      InsertNodeAfterNodeRequest(
          existingNodeId: selectedNode.id, newNode: newNode),
      ChangeSelectionRequest(
        DocumentSelection.collapsed(
          position: DocumentPosition(
            nodeId: newNode.id,
            nodePosition: const UpstreamDownstreamNodePosition.downstream(),
          ),
        ),
        SelectionChangeType.placeCaret,
        SelectionReason.userInteraction,
      )
    ]);
  }
matthew-carroll commented 2 weeks ago

@snowb1shop we ask for self-contained runnable reproductions. It's a friction to pull down an entire fork of the repo just to see the bug. Can you please create a minimal runnable entrypoint with just enough code to recreate the issue and post that as a code block in this issue?

rafalplonka commented 2 weeks ago

Here is the updated demo:

  1. Place caret on the text
  2. Click the ElevatedButton
import 'package:flutter/material.dart';
import 'package:super_editor/super_editor.dart';

void main() {
  runApp(
    MaterialApp(
      home: Scaffold(
        body: _StandardEditor(),
      ),
      debugShowCheckedModeBanner: false,
    ),
  );
}

class _StandardEditor extends StatefulWidget {
  const _StandardEditor();

  @override
  State<_StandardEditor> createState() => _StandardEditorState();
}

class _StandardEditorState extends State<_StandardEditor> {
  final GlobalKey _docLayoutKey = GlobalKey();

  late MutableDocument _doc;
  late MutableDocumentComposer _composer;
  late Editor _docEditor;

  late FocusNode _editorFocusNode;

  late ScrollController _scrollController;

  @override
  void initState() {
    super.initState();
    _doc = MutableDocument(nodes: [
      ParagraphNode(
        id: Editor.createNodeId(),
        text: AttributedText(
          "Place a caret on the text",
        ),
      )
    ]);
    _composer = MutableDocumentComposer();
    _docEditor =
        createDefaultDocumentEditor(document: _doc, composer: _composer);
    _editorFocusNode = FocusNode();
    _scrollController = ScrollController();
  }

  @override
  void dispose() {
    _scrollController.dispose();
    _editorFocusNode.dispose();
    _composer.dispose();
    super.dispose();
  }

  @override
  Widget build(BuildContext context) {
    return SafeArea(
      child: Container(
        margin: EdgeInsets.only(top: 40),
        child: Column(
          children: [
            ElevatedButton(
              onPressed: () {
                final selectedNode = _doc.getNodeAt(0)! as TextNode;

                final newNode = HorizontalRuleNode(
                  id: Editor.createNodeId(),
                );

                _docEditor.execute([
                  InsertNodeAfterNodeRequest(
                      existingNodeId: selectedNode.id, newNode: newNode),
                  ChangeSelectionRequest(
                    DocumentSelection.collapsed(
                      position: DocumentPosition(
                        nodeId: newNode.id,
                        nodePosition:
                            const UpstreamDownstreamNodePosition.downstream(),
                      ),
                    ),
                    SelectionChangeType.placeCaret,
                    SelectionReason.userInteraction,
                  )
                ]);
              },
              child: const Text('Create HorizontalRuleNode'),
            ),
            SuperEditor(
              editor: _docEditor,
              document: _doc,
              composer: _composer,
              focusNode: _editorFocusNode,
              scrollController: _scrollController,
              documentLayoutKey: _docLayoutKey,
            ),
          ],
        ),
      ),
    );
  }
}
angelosilvestre commented 1 week ago

@snowb1shop @matthew-carroll This happens because, on Android, when we tap at a word, Android generates a composing region for that word. In the sample code, the selection is being changed to a non-text node, but we keep the composing region of the previously selected word. As a result, we are trying to apply an invalid composing region.

Adding a ClearComposingRegionRequest to the request list fixes the issue.

In the places that SuperEditor itself issues a ChangeSelectionRequest, we also issue a ClearComposingRegionRequest. Maybe we could change ChangeSelectionCommand to also clear the composing region if the selection change from one node to another. Any thoughts on that?

rafalplonka commented 1 week ago

Thanks, it worked after adding what you suggested.

In my opinion it makes sense to include it in the ChangeSelectionCommand, especially that it worked correctly on other platforms (iOS, Web).

matthew-carroll commented 5 days ago

@angelosilvestre didn't we originally have an implementation that bundled selection changes with composing region changes, only to split them apart because it caused problems?