[SuperEditor][mobile] Implement spellchecking support (Resolves #2353) #2378

angelosilvestre · 2024-10-16T00:37:13Z

[SuperEditor][mobile] Implement spellchecking support. Resolves #2353

Our current spellchecker plugin only works on macOS. This PR adds support for spellchecking on Android and iOS on the spellchecker plugin.

Currently, the desktop implementation is showing the suggestions toolbar whenever the currently selection sits within a misspelled word with available suggestions. However, this wouldn't work for mobile because, on mobile platforms, the suggestions toolbar is displayed when tapping on a misspelled word, but not when dragging the handle over a misspelled word. This creates an implementation problem, because a gesture (tap up) should trigger the suggestions popover and we don't have an extension point for plugins to do that.

To solve this issue, this PR adds the ability for plugins to install a delegate for tap handlers. The plugin now has the opportunity to handle the tap the way it wants, preventing our gesture system for handling them.

The tap delegate shows/hides the suggestions popover by interacting with a SpellCheckerPopoverController, a class that exposes the spellchecker popover functionality, which uses a SpellCheckerPopoverDelegate (usually a document layer) to effectively display the popover toolbar.

This PR also add an option to temporarily disable selection handles. This is needed because, when tapping on a misspelled word, we want to select the whole word, but prevent the selection handles from being displayed.

On mobile platforms, the selection color is different when the suggestions popover is visible. Currently, we don't have an option to override the selection color. On our style pipeline, the selection styler is always last, so even if we use a custom style phase, the selection styler always resets the selection color.

To solve that, this PR exposes an appendedStylePhases property on SuperEditor, so we can add style phases that are positioned after the selection styler, adding the ability to override the selection color.

This PR adds default implementations for iOS-style and Android-style suggestions popovers.

Screen.Recording.2024-10-28.at.21.27.40.mov

android_spellcheck.webm

matthew-carroll · 2024-10-18T17:12:24Z

@angelosilvestre - Is this the PR you wanted me to review? If so, there should still be a description explaining what you did and how you did it - that's what directs the PR review. Also, whenever you want a review, please tag me in the PR.

matthew-carroll

I did a partial review. Before reviewing the rest, I think we need to align on why this spell check controller was injected in the places that it was. When we talked about this online, I mentioned treating this spell check toolbar the same way we treat the mobile editing toolbars, but as far as I can tell, that's not what this PR is currently doing.

super_editor/lib/src/default_editor/document_gestures_touch_android.dart

matthew-carroll · 2024-10-18T17:25:23Z

super_editor/lib/src/default_editor/spelling_and_grammar/spell_checker_popover_controller.dart

+
+  /// Attaches this controller to a delegate that knows how to
+  /// show a popover with spelling suggestions.
+  void attach(SpellCheckerPopoverDelegate delegate) {


Did we use an attach/detach approach for the existing mobile editing toolbar, too?

matthew-carroll · 2024-10-18T17:26:11Z

super_editor/lib/src/default_editor/spelling_and_grammar/spell_checker_popover_controller.dart

@@ -0,0 +1,35 @@
+import 'package:super_editor/src/core/document_selection.dart';
+
+/// Shows/hides a popover with spelling suggestions.


There should be a lot more information in this Dart Doc. Imagine reading this as someone who needs to use this - there's no guiding information about where/when/how.

matthew-carroll · 2024-10-18T17:26:42Z

super_editor/lib/src/default_editor/spelling_and_grammar/spell_checker_popover_controller.dart

+  }
+}
+
+abstract class SpellCheckerPopoverDelegate {


Missing Dart Doc. This should have a lot of info about who is supposed to implement this.

Added some docs, can you take a look to see what else should be detailed?

matthew-carroll · 2024-10-18T17:27:15Z

super_editor/lib/src/default_editor/spelling_and_grammar/spell_checker_popover_controller.dart

+}
+
+abstract class SpellCheckerPopoverDelegate {
+  /// Shows spelling suggestions for the word at [wordRange].


What if the document changes? What if the user's selection changes? Please fully describe expected behaviors.

Added some docs, can you take a look to see what else should be detailed?

super_editor/lib/src/default_editor/super_editor.dart

angelosilvestre · 2024-10-19T20:20:33Z

@angelosilvestre - Is this the PR you wanted me to review? If so, there should still be a description explaining what you did and how you did it - that's what directs the PR review. Also, whenever you want a review, please tag me in the PR.

This PR is meant to review the controller usage only. I should have added that to the description, sorry.

angelosilvestre · 2024-10-19T20:21:54Z

I did a partial review. Before reviewing the rest, I think we need to align on why this spell check controller was injected in the places that it was. When we talked about this online, I mentioned treating this spell check toolbar the same way we treat the mobile editing toolbars, but as far as I can tell, that's not what this PR is currently doing.

@matthew-carroll I moved the controller to SuperEditorIosControlsController and SuperEditorAndroidControlsController. Can you take a look to see if that's how we should proceed?

super_editor/lib/src/default_editor/document_gestures_touch_android.dart

matthew-carroll · 2024-11-05T23:47:15Z

super_editor/lib/src/default_editor/document_gestures_mouse.dart

-    widget.contentTapHandler?.addListener(_updateMouseCursorAtLatestOffset);
+    if (widget.contentTapHandlers != null) {
+      for (final handler in widget.contentTapHandlers!) {
+        handler.addListener(_updateMouseCursorAtLatestOffset);


I don't remember what this callback is for. Are you sure it makes sense to run that callback for every handler?

matthew-carroll · 2024-11-05T23:52:26Z

super_editor/lib/src/default_editor/document_gestures_mouse.dart

@@ -62,9 +63,9 @@ class DocumentMouseInteractor extends StatefulWidget {
  final Stream<DocumentSelectionChange> selectionChanges;
  final ValueListenable<DocumentSelection?> selectionNotifier;

-  /// Optional handler that responds to taps on content, e.g., opening
+  /// Optional list of handlers that respond to taps on content, e.g., opening


For each Dart Doc related to these handlers, please specify the chain of responsibility behavior. The current Dart Docs here, and elsewhere, don't explain that only a single handler is ever run...

matthew-carroll · 2024-11-05T23:59:20Z

super_editor/lib/src/default_editor/document_gestures_touch_android.dart

+
+    if (widget.contentTapHandlers != null && docPosition != null) {
+      for (final handler in widget.contentTapHandlers!) {
+        final result = handler.onTap(docPosition);


I see that this was moved up above the long tap and scrolling handler. I'm not sure that's correct. Can you explain why they belong up here?

matthew-carroll · 2024-11-06T00:02:48Z

super_editor/lib/src/default_editor/spelling_and_grammar/spelling_and_grammar_styler.dart

@@ -34,6 +35,15 @@ class SpellingAndGrammarStyler extends SingleColumnLayoutStylePhase {
    markDirty();
  }

+  /// Whether or not we should to override the default selection color with [selectionHighlightColor].
+  ///
+  /// On mobile platforms, when the suggestions popover is opened, the selected text uses a different


This line is a bit confusing. Does this line mean that this property will be true on mobile?

matthew-carroll · 2024-11-06T00:17:15Z

super_editor/lib/src/default_editor/super_editor.dart

@@ -847,7 +861,13 @@ class SuperEditorState extends State<SuperEditor> {
          getDocumentLayout: () => editContext.documentLayout,
          selectionChanges: editContext.composer.selectionChanges,
          selectionNotifier: editContext.composer.selectionNotifier,
-          contentTapHandler: _contentTapDelegate,
+          contentTapHandlers: [
+            if (_contentTapDelegate != null) //


The term "delegate" probably needs to be replaced with "handler" because typically there's only one delegate. There might be many handlers.

Given that SuperEditor can take a list of handlers, is there a reason that plugins only get one?

matthew-carroll · 2024-11-06T00:18:55Z

super_editor/lib/src/infrastructure/platforms/android/android_document_controls.dart

@@ -311,6 +314,12 @@ class AndroidControlsDocumentLayerState
    });
  }

+  void _onSelectionHandlesAllowedChange() {
+    setState(() {
+      //


Please specify what's being rebuilt and why

matthew-carroll · 2024-11-06T00:19:27Z

super_editor/lib/src/infrastructure/platforms/android/android_document_controls.dart

@@ -319,6 +328,11 @@ class AndroidControlsDocumentLayerState
      return null;
    }

+    if (_controlsController!.areSelectionHandlesAllowed.value == false) {


Is this value nullable? It seems strange that areSelectionHandlesAllowed would ever be null...

matthew-carroll · 2024-11-06T00:21:08Z

super_editor_spellcheck/example/lib/main_super_editor_spellcheck.dart

+            autofocus: true,
+            editor: _editor,
+            appendedStylePhases: [
+              _spellingAndGrammarPlugin.styler,


Shouldn't this be added automatically as part of the plugin?

angelosilvestre · 2024-11-11T15:21:40Z

@matthew-carroll This should wait on #2397, because we will have some conflicts.

[SuperEditor][mobile] Implement spellchecking support (Resolves #2353)

dd633ff

matthew-carroll reviewed Oct 18, 2024

View reviewed changes

Move controller to scope widgets

ca6e359

matthew-carroll reviewed Oct 20, 2024

View reviewed changes

super_editor/lib/src/default_editor/document_gestures_touch_android.dart Outdated Show resolved Hide resolved

angelosilvestre added 4 commits October 24, 2024 22:11

Handle gestures in plugin

448f087

Close popover when document changes

3f48fbf

Cleanup

9a56415

Fixes for Android

ec6830d

angelosilvestre requested a review from matthew-carroll October 29, 2024 01:31

angelosilvestre marked this pull request as ready for review October 29, 2024 01:32

angelosilvestre changed the title ~~WIP: [SuperEditor][mobile] Implement spellchecking support (Resolves #2353)~~ [SuperEditor][mobile] Implement spellchecking support (Resolves #2353) Oct 29, 2024

matthew-carroll reviewed Nov 6, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SuperEditor][mobile] Implement spellchecking support (Resolves #2353) #2378

[SuperEditor][mobile] Implement spellchecking support (Resolves #2353) #2378

angelosilvestre commented Oct 16, 2024 •

edited

Loading

matthew-carroll commented Oct 18, 2024

matthew-carroll left a comment

matthew-carroll Oct 18, 2024

matthew-carroll Oct 18, 2024

matthew-carroll Oct 18, 2024

angelosilvestre Oct 29, 2024

matthew-carroll Oct 18, 2024

angelosilvestre Oct 29, 2024

angelosilvestre commented Oct 19, 2024

angelosilvestre commented Oct 19, 2024

matthew-carroll Nov 5, 2024

matthew-carroll Nov 5, 2024

matthew-carroll Nov 5, 2024

matthew-carroll Nov 6, 2024

matthew-carroll Nov 6, 2024

matthew-carroll Nov 6, 2024

matthew-carroll Nov 6, 2024

matthew-carroll Nov 6, 2024

angelosilvestre commented Nov 11, 2024

		@@ -0,0 +1,35 @@
		import 'package:super_editor/src/core/document_selection.dart';

		/// Shows/hides a popover with spelling suggestions.

[SuperEditor][mobile] Implement spellchecking support (Resolves #2353) #2378

Are you sure you want to change the base?

[SuperEditor][mobile] Implement spellchecking support (Resolves #2353) #2378

Conversation

angelosilvestre commented Oct 16, 2024 • edited Loading

matthew-carroll commented Oct 18, 2024

matthew-carroll left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

angelosilvestre commented Oct 19, 2024

angelosilvestre commented Oct 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

angelosilvestre commented Nov 11, 2024

angelosilvestre commented Oct 16, 2024 •

edited

Loading