Red spell check selection on iOS#125162
Conversation
flutter-dashboard
bot
added
a: text input
justinmc
force-pushed
the
spell-check-red-selection
branch
from
ee4a962 to
4e0f160
Compare
justinmc
force-pushed
the
spell-check-red-selection
branch
from
4d8307d to
8b6447f
Compare
camsim99
left a comment
camsim99
left a comment
There was a problem hiding this comment.
Just left nits and questions :)
| /// {@endtemplate} | ||
| void showMagnifier(MagnifierInfo initialMagnifierInfo) { | ||
| if (_toolbar != null || _contextMenuControllerIsShown) { | ||
| if (_toolbar != null || _contextMenuController.isShown || _spellCheckMenuController.isShown) { |
There was a problem hiding this comment.
Can you use toolbarIsVisible here?
| /// | ||
| /// For example, on iOS, the selection appears red while the spell check menu | ||
| /// is showing. | ||
| final Color? misspelledSelectionColor; |
There was a problem hiding this comment.
I considered the alternative of not making this configurable, but with the way we create SpellCheckConfigurations in the text fields, I think this makes the most sense :)
There was a problem hiding this comment.
Ok cool, this was one of the main decisions I wanted to get your feedback on. I think that however we do it, CupertinoTextField needs to tell EditableText what the color should be somehow, so we can't make it completely private. The other alternative I considered was adding a parameter spellCheckSelectionColor to EditableText, but I think this way seems more organized.
There was a problem hiding this comment.
Actually before I commit 100% to this, @MitchellGoodwin do you have any thoughts?
So currently TextField and CupertinoTextField read the selection color from DefaultSelectionStyle and then pass it to EditableText as the selectionColor parameter. This PR adds the ability for CupertinoTextField to change the selection to red for spell check by passing the new misspelledSelectionColor value to EditableText as a part of SpellCheckConfiguration.
The question is, does this align with your vision for theming/styles in Cupertino?
There was a problem hiding this comment.
Yes, I don't see any issues with that. 👍
| // Manages the context menu. Not necessarily visible when non-null. | ||
| final ContextMenuController _contextMenuController = ContextMenuController(); | ||
|
|
||
| bool get _contextMenuControllerIsShown => _contextMenuController.isShown; |
There was a problem hiding this comment.
What were the side effects of using the same ContextMenuController for both the text selection toolbar and the spell check suggestions toolbar?
There was a problem hiding this comment.
Under the hood, all ContextMenuController instances share a single OverlayEntry, so only one can ever be visible at a time, which is convenient here. The main difference is that if we have two, it's easier to distinguish which one is being shown at a given time.
| tester.binding.platformDispatcher.nativeSpellCheckServiceDefinedTestValue = | ||
| true; | ||
| const Color defaultSelectionColor = Color(0x33007aff); | ||
| const Color misspelledSelectionColor = Color(0x62ff9699); |
There was a problem hiding this comment.
Nit: consider leaving a comment about where these values came from.
iOS now hides the selection handles and shows red selection when tapping a misspelled word, like native.
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="proxy.php?url=https://user-images.githubusercontent.com/389558/234087650-bcd62c89-03e7-427d-afc5-0fe8f96a5f80.png" /> | <video src="proxy.php?url=https://user-images.githubusercontent.com/389558/234087667-651b0fde-348c-467e-ba00-27b6b3966a27.mov" /> | CC @itsjustkevin @leighajarett
3 participants
Fixes #124404