Skip to content

Fix the position of the Android-style spell check toolbar #124897

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 14 commits into from
Apr 24, 2023

Conversation

justinmc
Copy link
Contributor

@justinmc justinmc commented Apr 14, 2023

Before After
Screenshot from 2023-04-14 15-10-41 image

Bonus "after" gif:

handles

Some open questions @camsim99:

  • Is this the correct position?
    • Answer: yes.
  • Should the handle be hidden as well?
    • Answer: yes

Partial fix for: #124882

@justinmc justinmc requested a review from camsim99 April 14, 2023 22:23
@justinmc justinmc self-assigned this Apr 14, 2023
@flutter-dashboard flutter-dashboard bot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Apr 14, 2023
@camsim99
Copy link
Contributor

The position should be right below the underline and I think you are right -- the handle should be hidden. I've seen it look exactly like https://developer.android.com/develop/ui/views/touch-and-input/spell-checker-framework#SpellCheckClient on emulators.

@flutter-dashboard flutter-dashboard bot added the a: text input Entering text in a text field or keyboard related problems label Apr 18, 2023
@justinmc justinmc marked this pull request as ready for review April 18, 2023 20:44
@justinmc
Copy link
Contributor Author

The failures are caused by #125144. I'll push a merge commit once that's merge and it should be green.

@@ -3990,7 +3990,8 @@ class EditableTextState extends State<EditableText> with AutomaticKeepAliveClien
|| platformNotSupported
|| widget.readOnly
|| _selectionOverlay == null
|| !_spellCheckResultsReceived) {
|| !_spellCheckResultsReceived
|| findSuggestionSpanAtCursorIndex(textEditingValue.selection.extentOffset) == null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great fix!

@@ -180,7 +182,7 @@ class SpellCheckSuggestionsToolbar extends StatelessWidget {
return Padding(
padding: EdgeInsets.fromLTRB(
CupertinoTextSelectionToolbar.kToolbarScreenPadding,
kToolbarContentDistanceBelow,
paddingAbove,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the right value? I think the menu should be right below the red, misspelled-word-indicating underline, e.g.:

Screen Shot 2022-09-06 at 3 37 51 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about this?

Screenshot 2023-04-20 at 12 54 30 PM

I think kToolbarContentDistanceBelow doesn't apply to the spell check toolbar then, if we want it right below the anchor. I removed it from this file, so I'll watch out for a breaking change in case someone was using it already.

Also by the way, the lack of red underline and red selection has me worried. Is that something we should open an issue for?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok sounds good! This looks better.

In terms of the red underline/red selection, I revisited some notes I took and seems like that is behavior that differs based on Android version. So, we can open an issue to track it, but just FYI.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created an issue here: #125424

@justinmc justinmc force-pushed the spell-check-material-position branch from 5e54391 to 01a050d Compare April 20, 2023 20:01
@justinmc justinmc requested a review from camsim99 April 20, 2023 20:04
Copy link
Contributor

@camsim99 camsim99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new value looks good to me!

@justinmc justinmc merged commit 98aaf00 into flutter:master Apr 24, 2023
@justinmc justinmc deleted the spell-check-material-position branch April 24, 2023 17:26
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 24, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 24, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 24, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 25, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 25, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 25, 2023
justinmc added a commit to justinmc/flutter that referenced this pull request Apr 25, 2023
…4897)

The spell check menu now appears directly below the misspelled word on Android.
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 26, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 26, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 26, 2023
itsjustkevin pushed a commit that referenced this pull request Apr 26, 2023
This is a cherry pick of 8 of my recent spell check bug fixes into the
beta branch.

  1. #124259
  2. #124875
  3. #124254
  4. #124899
  5. #124895
  6. #125162
  7. #124897
  8. #125432

This is the behavior of spell check with these changes:

| Screenshot | Video |
| --- | --- |
| <img
src="https://user-images.githubusercontent.com/389558/234087650-bcd62c89-03e7-427d-afc5-0fe8f96a5f80.png"
/> | <video
src="https://user-images.githubusercontent.com/389558/234087667-651b0fde-348c-467e-ba00-27b6b3966a27.mov"
/> |

CC @itsjustkevin @leighajarett
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a: text input Entering text in a text field or keyboard related problems f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants