Introduce Expansible, a base widget for ExpansionTile

[victorsanni]

Take 2

Design doc: flutter.dev/go/codeshare-expansion-tile

Fixes Codeshare between ExpansionTile and its Cupertino variant

Pre-launch Checklist

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• I read the Tree Hygiene wiki page, which explains my responsibilities.
• I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
• I signed the CLA.
• I listed at least one issue that this PR fixes in the description above.
• I updated/added relevant documentation (doc comments with ///).
• I added new tests to check the change I am making, or this PR is test-exempt.
• I followed the breaking change policy and added Data Driven Fixes where supported.
• All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

[flutter-dashboard]

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #164049 at sha d0527c1

[victorsanni]

Failing google test: https://github.com/rrousselGit/flutter_hooks/blob/2dbd2d5b466ae4ab9ce595c59150ea491db59dcd/packages/flutter_hooks/test/use_expansion_tile_controller_test.dart#L9

[chunhtai]

seems a bit weird the use the word layout, maybe just build. also maybe mention where the header and body comes from. e.g. are the the same widget returned from headerBuilder and bodyBuilder?

[chunhtai]

also since this allows arbiturary position of header and body, should we just have one ExpansibleBuilder and remove the header and body builder?

[victorsanni]

also since this allows arbiturary position of header and body, should we just have one ExpansibleBuilder and remove the header and body builder?

That would be nice if the header and body were just outputs of headerBuilder and bodyBuilder. But internally the result of headerBuilder is wrapped with a GestureDetector to toggle the expansion, and the result of bodyBuilder is wrapped in Offstage and also used for the static part of the internal AnimatedBuilder.

[dkwingsmt]

Is it still the case? I don't see any GestureDetectors in ExpansibleState any more. Does it allow a better name now?

[chunhtai]

Since this is existing API, so no immediate action item in this PR. This is something that may potentially be dangerous, if a widget somehow relies on this to build, there isn't a way to listen for change.

[victorsanni]

Is there a generally accepted solution for these kinds of situations?

[chunhtai]

IMO no, either this controller be a changenotifier so that people can listen to this property, or we should remove this property getter

[chunhtai]

In a bigger picture we should remove duplicate API in Expandible and this controller.

1. Either the controller is stateful and changenotifier where it is the single source of truth. We should remove the initialExpand and onExpandChange callback on this widget. If people want to listen to onExpandChange they should just listen to the controller.

2. remove this controller and keeps initialExpand and onExpandChange, and expose an API on ExpandibleState to toggle expand. If developer want to toggle the expand, they need to provide a globalkey to this widget and use it to call the toggle api on the state

[victorsanni]

I was able to remove onExpansionChanged by making the controller extend ChangeNotifier. But I can't remove initiallyExpanded because the controller state object is empty until it is set in Expansible's initState. So we can't call controller.expand in ExpansionTile's initState. Is there a way to work around this?

Also if we are maximizing using the controller since it is required, can we remove the isExpanded parameter from the builder callbacks and have consumers use controller.isExpanded so that would be the single source of truth?

[chunhtai]

I can't remove initiallyExpanded because the controller state object is empty until it is set in Expansible's initState. So we can't call controller.expand in ExpansionTile's initState. Is there a way to work around this?

The isExpand should be property stored in controller instead of ExpansibleState. ExpansibleState will build based on the controller's property. right now it is the other way around.

For developer who wants set the initial state, they should set it on the controller and pass it into the Expandible

Also if we are maximizing using the controller since it is required, can we remove the isExpanded parameter from the builder callbacks and have consumers use controller.isExpanded so that would be the single source of truth?

I think having a expand here is fine as long as the build method will be call every time this value change. If not, then we should remove this property and ask people to build based on the controller's state

[victorsanni]

For developer who wants set the initial state, they should set it on the controller and pass it into the Expandible

I'm having an annoying bug here. If we add onExpansionChanged as a listener to isExpanded, when the developer sets the initial state, it calls onExpansionChanged even though the expansion didn't actually change.

Edit: Fixed, I didn't set up my setter correctly.

[chunhtai]

just one last concern, otherwise the rest LGTM

[chunhtai]

Duplicating animation controller by hooking them up in this method feels error prone.

I don't think passing in the controller from material/cupertino and mutate it in Expandible is a concern. We do that all the time with TextEditingController and FocusNode. as long as the controller do not get disposed before the expandible is disposed, we should be fine

[victorsanni]

The main issue I have with passing the animation controller from material/cupertino into Expansible is that we already require the ExpansibleController which is fully controlled by Expansible. So passing it in would mean Expansible requires two controllers, the ExpansibleController and the animation controller which will then be mutated in the expansible. ExpansionTile needs the animation controller to sync other animations to the expansion animation (e.g the icon turning), but other consumers of Expansible may not have any extra animations that need the animation controller. But in those cases, they would still need to create and dispose one just to pass it in to Expansible. It just seems like a cleaner API surface if users manage any extra animations outside Expansible, but are still able to sync them up to change when the expansion state changes.

For possible errors with this approach, the one that comes to mind is if the duplicate animation controller is somehow different from the internal one (in duration for example). But that's not really an issue since the duration gets passed into Expansible anyways. And it could even be a benefit if the consumer of Expansible wants to add an extra animation that is different (e.g faster/slower) but still syncs up with the expansion animation.

Or maybe there could be more errors I haven't thought about.

[chunhtai]

Expansible requires two controllers, the ExpansibleController and the animation controller

I don't see this as a problem.

but other consumers of Expansible may not have any extra animations that need the animation controller

We generally won't expect people to directly use building block widget like this one., so having this restriction in widget layer is fine. One way to mitigate this is to make animationcontroller a optional parameter and autogenerate internal one in ExpansibleState if not provided. but I am also oppose this as this is a building block widget, and can be error prone when disposing resource

[victorsanni]

We generally won't expect people to directly use building block widget like this one

I'm not entirely sure about this. For example, RawAutocomplete is supposed to be another building block widget. But many use cases I've seen (e.g Zulip) ditch Autocomplete and just use RawAutocomplete because of more customization opportunities and less ties to material.

This expanding/collapsing behavior is very common. There is a main Cupertino variant (CupertinoCollapsible with the rotating icon), but there are also other places in Cupertino where this widget would be used directly (e.g inline date/time pickers). And extrapolating to other design languages as well.

One way to mitigate this is to make animationcontroller a optional parameter and autogenerate internal one in ExpansibleState if not provided.

But this will require creating a fallback controller with it's own issues.

[victorsanni]

But then on the flip side we are already requiring the ExpansibleController anyways. And an extra animation controller in material/cupertino feels like a hack. Although I'm still wondering about other potential cons.

[chunhtai]

But many use cases I've seen (e.g Zulip) ditch Autocomplete and just use RawAutocomplete because of more customization opportunities and less ties to material.

I think if customer decide to build their own widget on top of Expandible, creating their own animationcontroller feels like a ok compromise.

If we really want to make it easier to use, I would choose a fallback controller instead of attempting to sync two controllers. We have example of attempting to sync two controller (tab controller and pagecontroller) before in TabBar and TabBarView, and it is very messy. It is a more extreme case compare to this one, but I would like to avoid any future case where this widget spiral into another tabbar and tabbarview.

[victorsanni]

I would like to avoid any future case where this widget spiral into another tabbar and tabbarview.

I see. Although looking at TabBar and TabBarView, the complexity comes from the controllers sharing information (e.g. the offset). But in this case, the controllers (and their animations) are completely independent, the only connection they have is that they are both called when the expansion state changes (so more like independent listeners to the expansion state vs. synced). Plus the expansible's animation controller is private. Do these change the risk of that type of spiral, or there's probably some future use case that will lead to problems here anyways?

I had some other questions as well:

• If an animation controller is passed into Expansible, should it be disposed in Expansible or in the consumer?
• Is it possible to instead create the animation controller in Expansible and pass it to material/cupertino through a callback? and if it is possible would that also cause problems?

[chunhtai]

I think most complexity is coming from when one wants to sync animation curve from one to another.

If an animation controller is passed into Expansible, should it be disposed in Expansible or in the consumer?

Who creates it will dispose it, so if it is created by expansiontile, the expansiontile should be responsible for dispose it

Is it possible to instead create the animation controller in Expansible and pass it to material/cupertino through a callback? and if it is possible would that also cause problems?

yeah this may be a better way, but probably not directly as a callback.

Is it possible to do it this way? If the expansion tile is not responsible for initiate the animation, we can consider pass the animation as one of builder's parameters. If it is responsible for initiate the animation, i think it can do so by calling ExpandibleController's api

[victorsanni]

Applied in cf82ae4

[chunhtai]

LGTM, just one concern about left over mixin

[chunhtai]

IMO no, either this controller be a changenotifier so that people can listen to this property, or we should remove this property getter

[chunhtai]

In a bigger picture we should remove duplicate API in Expandible and this controller.

1. Either the controller is stateful and changenotifier where it is the single source of truth. We should remove the initialExpand and onExpandChange callback on this widget. If people want to listen to onExpandChange they should just listen to the controller.

2. remove this controller and keeps initialExpand and onExpandChange, and expose an API on ExpandibleState to toggle expand. If developer want to toggle the expand, they need to provide a globalkey to this widget and use it to call the toggle api on the state

[victorsanni]

Either the controller is stateful and changenotifier where it is the single source of truth. We should remove the initialExpand and onExpandChange callback on this widget. If people want to listen to onExpandChange they should just listen to the controller.

Will making the controller extend ChangeNotifier be a breaking change? If it isn't, I think in this PR we can refactor to remove both of those parameters and make ExpansionTile do it through the controller (since the controller is required in Expansible).

[victorsanni]

This is also another case of #53059.