Material Text Selection Toolbar improvements #69428
Conversation
…ding handling overflow
…ted, tests should be passing
|
Gold has detected about 15 untriaged digest(s) on patchset 12. |
|
Gold has detected about 15 untriaged digest(s) on patchset 13. |
|
Gold has detected about 15 untriaged digest(s) on patchset 14. |
|
Gold has detected about 18 untriaged digest(s) on patchset 16. |
I took only my changes and then fixed NNBD problems.
18fa862 to
857a6c8
Compare
|
@LongCatIsLooong This is ready for re-review. Thanks for all of the great feedback. I've moved several things into separate issues, so double check that you're ok with those not being fixed here. All of them existed in our toolbar before this PR, I've just moved the code and made it public now. |
|
|
||
| @override | ||
| bool shouldRelayout(_TextSelectionToolbarLayoutDelegate oldDelegate) { | ||
| return anchor != oldDelegate.anchor; |
There was a problem hiding this comment.
Should fitsAbove be checked here as well?
|
|
||
| @override | ||
| void updateRenderObject(BuildContext context, _TextSelectionToolbarTrailingEdgeAlignRenderBox renderObject) { | ||
| renderObject.overflowOpen = overflowOpen; |
There was a problem hiding this comment.
missing textDirection update?
| /// | ||
| /// Useful for customizing the high-level background of the toolbar. The given | ||
| /// child Widget will contain all of the [children]. | ||
| final _ToolbarBuilder toolbarBuilder; |
There was a problem hiding this comment.
Should we make the type alias public?
There was a problem hiding this comment.
I guess we should, since users will be passing in their own toolbarBuilders. I thought about making this more generic and maybe naming it ChildWidgetBuilder, but since it's possible that we'll need to add a new toolbar-specific parameter to this in the future, I'll keep the name as ToolbarBuilder.
| /// See also: | ||
| /// * [TextSelectionToolbarTextButton], which builds a default Material- | ||
| /// style text selection toolbar text button. | ||
| final List<Widget> children; |
There was a problem hiding this comment.
If this is empty, the toolbar doesn't show, is that correct?
There was a problem hiding this comment.
There is an assertion in this file that children.length > 0, and in text_selection.dart's buildToolbar method there is a check that won't even build TextSelectionToolbar if there are no children.
I'll add another assertion to the top level TextSelectionToolbar and also note this in the docs.
|
No worries! I'm happy to have caught all of those bugs now. I'll plan to merge this when the tests go green. |
Description
This PR refactors material/text_selection.dart into more reasonable classes, and it makes public a top-level TextSelectionToolbar class and the default Material text selection menu button TextSelectionToolbarTextButton.
See the text selection toolbar customization design doc for full details.
The PR stops short of making any breaking changes, so the developer experience is still not perfect, just much better than before. A larger refactor of the text editing code is coming (design doc), and we should be able to improve the experience further there.
Example
I've published a working example of how to create a custom menu with the code in this branch in a repo here.
Newly public classes
TextSelectionToolbar
A fully-functional Android-style text selection menu. Built by TextSelectionControls's buildToolbar.
There is a bit of math to do in order to calculate the parameters needed (see where I do this in buildToolbar in the example).
TextSelectionToolbarTextButton
A standard Android-style text selection menu button. Used to create custom buttons and pass them into TextSelectionToolbar.
Private classes
I cleaned up many of the private classes as well, to make sure that they have single responsibilities that make sense. We may want to make some of these public in the future if users are making very custom menus and asking for these abilities.
_TextSelectionToolbarLayoutDelegate
Centers the toolbar at the given anchor and ensures it remains on screen.
_TextSelectionToolbarOverflowable
Maintains the state of whether or not the overflow menu is open, and renders the rest of the menu with this state.
_TextSelectionToolbarRightEdgeAlign
Just lines up the right edge of the menu when overflow is open (a quirk of how the overflow menu lays out on Android).
_TextSelectionToolbarItemsLayout
This does all of the hard overflow work. Detects overflow and lays out the buttons in the regular or overflow menu.
_TextSelectionToolbarContainer
Just the appearance of the background of the menu (outline and elevation).
Related Issues
#36985
#31412
This is not a full solution for those issues yet. It's still fairly verbose to do something simple like add a custom menu button. I think we'll need to make a breaking change to TextSelectionControls to get an acceptible solution for those.
Tests
Breaking Change
Not a breaking change, but does expose some things that were previously private.