[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

super_editor/lib/src/default_editor/document_gestures_mouse.dart

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?

Updated after the scrolling rework and the #2397. I guess it should be correct now.

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?

This is a transitive state, while the suggestions popover is open this will be true, otherwise it will be false.

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?

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

Updated on a previous PR.

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

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...

super_editor_spellcheck/example/lib/main_super_editor_spellcheck.dart

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

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

angelosilvestre · 2024-11-23T23:12:44Z

@matthew-carroll I also added the Android and iOS platforms to the spellcheck demo app.

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

matthew-carroll · 2024-12-04T22:06:34Z

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

@@ -59,13 +69,25 @@ class SpellingAndGrammarStyler extends SingleColumnLayoutStylePhase {
    markDirty();
  }

+  /// Temporarily use the [selectionHighlightColor] to override the default selection color.


Please describe this in a way that informs the caller how to use this. When/why should this be called? What's the impact of calling it?

Please don't ever use the word "temporarily" unless it's followed by a description of how much time passes before it goes back to what it was before. This particular call doesn't appear to have any connection to time, and in fact it looks like it might not be temporary at all if the user never calls useDefaultSelectionColor().

matthew-carroll · 2024-12-04T22:07:30Z

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

+    markDirty();
+  }
+
+  /// Restore the default selection color.


What does it mean to "restore" it? Please formulate Dart Docs so that they describe observable results, and/or describe code contract obligations. Otherwise, the caller has no context to understand when/where/why they'd want to "restore the default the selection color"

super_editor/lib/src/default_editor/super_editor.dart

matthew-carroll · 2024-12-04T22:10:58Z

super_editor/lib/src/default_editor/super_editor.dart

@@ -1156,6 +1171,13 @@ abstract class SuperEditorPlugin {

  /// Additional overlay [SuperEditorLayerBuilder]s that will be added to a given [SuperEditor].
  List<SuperEditorLayerBuilder> get documentOverlayBuilders => [];
+
+  /// Optional handler that responds to taps on content, e.g., opening


"handler that responds" -> "handlers that respond" - also this Dart Doc is missing any mention of whether this is a chain of responsibility, and if so, what that means in terms of priority. Any property that has a list of handlers needs to specify details about order of execution and/or whether one of them can prevent events flowing to the rest.

super_editor_spellcheck/lib/src/super_editor/spelling_error_suggestion_overlay.dart

matthew-carroll · 2024-12-04T23:00:58Z

super_editor_spellcheck/lib/src/super_editor/spelling_error_suggestion_overlay.dart

@@ -513,3 +669,273 @@ class _DesktopSpellingSuggestionToolbarState extends State<DesktopSpellingSugges
    }
  }
 }
+
+/// A spelling suggestion toolbar, designed for the Android platform,
+/// which displays a vertical list alternative spellings for a given mis-spelled


I don't know what "a vertical list alternative spellings" is.

super_editor_spellcheck/lib/src/super_editor/spelling_error_suggestion_overlay.dart

matthew-carroll · 2025-01-10T02:05:20Z

On our call we discovered that behavior in iOS 17 is different than iOS 16.

On iOS 17:

When the user taps a mis-spelled word, the mis-spelled word becomes fully selected. The selection color is the normal selection color. No handles are shown.
Pressing and dragging from either end of the word begins to drag that end of the selection. The side that's being dragged shows a non-blinking caret (instead of a handle). The other side of the selection shows a handle.
When the user stops dragging, the caret turns into a handle.

We looked at Android but found inconsistent behaviors between Keep, Gmail, SMS, etc.

Gmail and Calendar do the following:

Places the caret where tapped
Hides the caret
Highlights the misspelled word in red
Doesn't show any handles
Shows a popover list of spelling suggestions - this list is vertical
It seems like the suggestions are sitting in an invisible fullscreen modal. Tapping anywhere else causes the suggestions to close, and the previous caret position is restored. Even tapping on buttons while the suggestions are open doesn't result in the buttons being activated.

We should ensure that these behaviors work by default with this plugin.

angelosilvestre · 2025-01-10T23:05:46Z

@matthew-carroll I updated the behavior for iOS:

Simulator.Screen.Recording.-.iPhone.15.Pro.-.2025-01-10.at.20.01.17.mp4

matthew-carroll reviewed Oct 18, 2024

View reviewed changes

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 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

angelosilvestre force-pushed the 2353_mobile_spellcheck branch from ec6830d to 329b24e Compare November 23, 2024 23:09

angelosilvestre requested a review from matthew-carroll November 23, 2024 23:12

matthew-carroll reviewed Dec 4, 2024

View reviewed changes

angelosilvestre requested a review from matthew-carroll December 10, 2024 17:36

angelosilvestre added 12 commits January 9, 2025 21:47

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

823087d

Move controller to scope widgets

2eace8e

Handle gestures in plugin

d729d52

Close popover when document changes

a485038

Cleanup

7a77b37

Fixes for Android

5365f0d

PR updates

26f4cff

Add Android and iOS platforms to spellcheck demo app

4da2958

Bring back some code lost during rebase

a8a0c88

PR updates

623de69

PR updates

3d87056

Replace deprecated method

5b8e79e

angelosilvestre force-pushed the 2353_mobile_spellcheck branch from 8970f99 to 5b8e79e Compare January 10, 2025 00:52

Update iOS behavior

26d7dcd

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

angelosilvestre Nov 23, 2024

matthew-carroll Nov 6, 2024

angelosilvestre Nov 23, 2024

matthew-carroll Nov 6, 2024

angelosilvestre Nov 23, 2024

matthew-carroll Nov 6, 2024

angelosilvestre Nov 23, 2024

angelosilvestre commented Nov 11, 2024

angelosilvestre commented Nov 23, 2024

matthew-carroll Dec 4, 2024

angelosilvestre Dec 10, 2024

matthew-carroll Dec 4, 2024

angelosilvestre Dec 10, 2024

matthew-carroll Dec 4, 2024

angelosilvestre Dec 10, 2024

matthew-carroll Dec 4, 2024

angelosilvestre Dec 10, 2024

matthew-carroll commented Jan 10, 2025

angelosilvestre commented Jan 10, 2025

		@@ -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 Nov 23, 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

matthew-carroll commented Jan 10, 2025

angelosilvestre commented Jan 10, 2025

angelosilvestre commented Oct 16, 2024 •

edited

Loading