-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Add EditableText scroll into view breaking change note #8916
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
Conversation
sfshaza2
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few tweaks and a question.
| To date, `EditableText` has used multiple mechanisms ensuring the selection | ||
| extent or caret is scrolled into view on updates. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what you mean by "on updates".
| To date, `EditableText` has used multiple mechanisms ensuring the selection | |
| extent or caret is scrolled into view on updates. | |
| Previously, `EditableText` used multiple mechanisms to determine the | |
| extent of the selection, or to ensure that the caret is scrolled into view. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the edits. Yes, this is a bit convoluted: what I meant were the mechanisms used on scrolling into view (which happens on user updates) to determine either of:
- selection extent (non-collapsed selection)
- caret position (collapsed selection).
Let me reword as:
Previously, upon scrolling into view to show user updates,
EditableTextused multiple mechanisms to determine the extent of the selection or the caret location.
| By removing the `Editable.onCaretChanged` callback, `EditableText` will always | ||
| use the most up to selection extent location when scrolling to show it. | ||
| Specifically this results in improved scroll into view behavior after | ||
| changing selection from collapsed to non-collapsed via | ||
| `userUpdateTextEditingValue()`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does "use the most up to selection extent location" mean?
I've made some tweaks, but would do more once I understand this phrase.
| By removing the `Editable.onCaretChanged` callback, `EditableText` will always | |
| use the most up to selection extent location when scrolling to show it. | |
| Specifically this results in improved scroll into view behavior after | |
| changing selection from collapsed to non-collapsed via | |
| `userUpdateTextEditingValue()`. | |
| By removing the `Editable.onCaretChanged` callback, `EditableText` will always | |
| use the most up to selection extent location when scrolling to show it. | |
| Specifically, this improves scroll into view behavior after | |
| changing selection from collapsed to non-collapsed using | |
| `userUpdateTextEditingValue()`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The underlying issue had to do with using multiple sources of truth where one wasn't necessarily up to date (could be one frame behind).
What is clearly missing is "date" as in "up to date" :). Correcting this.
| ## Timeline | ||
|
|
||
| Landed in version: 3.12.0-4.0.pre<br> | ||
| In stable release: 3.13.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 3.13 release? It's probably best to just say "TBD" for now.
Co-authored-by: Shams Zakhour (ignore Sfshaza) <[email protected]>
|
@sfshaza2 wondering if you could take another look so we can merge framework's PR 109114? Thanks! |
sfshaza2
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm. Thanks, @tgucio!
Breaking change note for flutter/flutter#109114
Presubmit checklist