TextField should only set EditableText.cursorOffset for iOS #27663
Conversation
zoechi
added
a: text input
| TextSelectionControls textSelectionControls; | ||
| bool paintCursorAboveText; | ||
| bool cursorOpacityAnimates; | ||
| Offset cursorOffset; |
There was a problem hiding this comment.
Do we not need the default value of 2 coming from the widget.cursorOffset?
Some random yak shaving while we're here. I had to click through all the editables to realize that RenderEditable handles nulls. Can we add it to the doc?
There was a problem hiding this comment.
I also thing I'm leaning towards liking the getters more. It takes less effort to read and realize that we're only dealing with an Android and an iOS value and don't have to follow the local variable through to watch out for the fallthrough/null case.
There was a problem hiding this comment.
The cursorOffset is only defined for iOS.
I moved the couple of getters that were returning platform-specific values to the block of code that handles configuring the EditableText's other platform-specific values because it's easier to read (all in one place), simpler, and that's the way the rest of it already was.
I'll be happy to add some documentation about null parameters to RenderEditable in a separate PR.
There was a problem hiding this comment.
oops, sorry, there is no widget.cursorOffset. I misread widget.cursorWidth
| expect(textField.cursorRadius, const Radius.circular(3.0)); | ||
| }); | ||
|
|
||
| // TODO(hansmuller): restore these tests after the fix for #24876 has landed. |
There was a problem hiding this comment.
Not sure what this comment means. Isn't this PR the fix for the fix for #24876?
There was a problem hiding this comment.
Yes. I was going to land the fix and then update the golden images and then update this test.
|
LGTM |
|
cc @jslavitz |
4 participants
Fixes #27626
Restored the Material textfield tests that were changed in #24876.
Cleaned up the platform-specific configuration code in _TextFieldState.build().