Fix ExpansionTile splash effect

[TahaTesser]

fixes ExpansionTile InkSplash doesn't respect Shape's borderRadius
fixes ExpansionTile.backgroundColor & ExpansionTile.collapsedBackgroundColor removes splash effect

Code sample

expand to view the code sample

import 'package:flutter/material.dart';

void main() => runApp(const MyApp());

class MyApp extends StatelessWidget {
  const MyApp({super.key});

  @override
  Widget build(BuildContext context) {
    return const MaterialApp(
      debugShowCheckedModeBanner: false,
      home: Example(),
    );
  }
}

class Example extends StatelessWidget {
  const Example({super.key});

  @override
  Widget build(BuildContext context) {
    return const Scaffold(
      body: Center(
          child: Padding(
        padding: EdgeInsets.symmetric(horizontal: 24.0),
        child: ExpansionTile(
          collapsedBackgroundColor: Colors.red,
          backgroundColor: Colors.amber,
          collapsedShape: RoundedRectangleBorder(
            borderRadius: BorderRadius.all(Radius.circular(30.0)),
            side: BorderSide(color: Colors.black, width: 2.0),
          ),
          shape: RoundedRectangleBorder(
            borderRadius: BorderRadius.all(Radius.circular(30.0)),
            side: BorderSide(color: Colors.black, width: 2.0),
          ),
          clipBehavior: Clip.hardEdge,
          title: Text('Expansion Tile'),
          children: <Widget>[
            FlutterLogo(size: 50),
            FlutterLogo(size: 50),
            FlutterLogo(size: 50),
            FlutterLogo(size: 50),

          ],
        ),
      )),
    );
  }
}

Before

After

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.
• All existing and new tests are passing.

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

[HansMuller]

LGTM

Note: it's clear that this PR fixes the reported problem. What about other shapes? StadiumBorder for an obvious example.

[TahaTesser]

LGTM

Note: it's clear that this PR fixes the reported problem. What about other shapes? StadiumBorder for an obvious example.

That's a good point. Unfortunately, It's not easy to retrieve to border radius from StadiumBorder for the expanded state.
I wish we had some utility function that returns border radius from internal implementation when a ShapeBorder is provided. Here I had to add if and else, check type just to get border radius from one type of shape. Then there is also BeveledRectangleBorder to get border radius from.

Overall, I'm not happy with this current PR implementation just catering to one shape.
Do you think I should close this and come back to it later when we have more complete implementation?

[TahaTesser]

FYI, I also marked existing ExpansionTile.clipBehavior property as deprecated in this PR.
It actually doesn't serve any purpose in ExpansionTile. It doesn't clip the ink well effect or the anything that needs clipping. It was added in #112218.

[HansMuller]

I think it's OK to land this, since it fixes a common case. However please open an issue and a TODO in the code per your comment: #135855 (comment)

[TahaTesser]

@HansMuller
Thanks for the suggestion. I've added a todo and updated deprecation message for the unused clipBehavior parameter.

[TahaTesser]

Looks like this is failing "Google Testing"

[HansMuller]

The Google testing failures were mostly the sorts of golden image test failures that you'd expect. There were a few failures that were harder to understand, notably this:

Expected	Actual

It's not clear why your change would affect the entire background of the ExpansionTile. I've asked the developer about it.

[TahaTesser]

It's not clear why your change would affect the entire background of the ExpansionTile. I've asked the developer about it.

Interesting! Appreciate the help.

[HansMuller]

@TahaTesser - The developer took a look at the test failures; replacing the ExpansionTile's Container with an Ink widget is definitely a breaking change. Could the ExpansionTile widget just create its own Material? That's how the button classes ensure that the background and ink splash are clipped to the button's shape.

[TahaTesser]

Could the ExpansionTile widget just create its own Material? That's how the button classes ensure that the background and ink splash are clipped to the button's shape.

We've stayed from adding Material to ListTile for performance implications. Doing so the for the ExpansionTile would essentially be that same thing.

I've made a similar fix using the Ink widget for the ListTIle while back

flutter/packages/flutter/lib/src/material/list_tile.dart

Lines 856 to 863 in 4f959b9

	selected: selected,
	enabled: enabled,
	child: Ink(
	decoration: ShapeDecoration(
	shape: shape ?? tileTheme.shape ?? const Border(),
	color: _tileBackgroundColor(theme, tileTheme, defaults),
	),
	child: SafeArea(

I think the performance reasons to not add Material to ListTile also apply to ExpansionTile. As there could be tens of ExpansionTiles in a list view and each instance creating a Material widget in tree.

cc: @Piinks We've discussed this and agreed to not add Material widget and I documented performance considerations in the ListTIle

[TahaTesser]

The developer took a look at the test failures; replacing the ExpansionTile's Container with an Ink widget is definitely a breaking change.

I might be able to figure something out if you could please share the test case (test code sample) that's breaking.

[HansMuller]

Not sure if a test case is forthcoming, will ask though. @Piinks suggested only creating a Material widget per ExpansionTile when a shape was specified. That might mitigate the performance impact a little. Assuming there still is a performance penalty for creating extra Material widgets and that lazy list rendering doesn't suffice to overcome whatever penalty that is.

[TahaTesser]

Not sure if a test case is forthcoming, will ask though. @Piinks suggested only creating a Material widget per ExpansionTile when a shape was specified. That might mitigate the performance impact a little. Assuming there still is a performance penalty for creating extra Material widgets and that lazy list rendering doesn't suffice to overcome whatever penalty that is.

This will be great if we can mitigate the performance penalty.

Adding Material widget when shape is provided will eliminate the all the hurdles we had earlier in this PR. 🎉

1. We can clip any provided shape including RoundedRectangleBorder, StadiumBorder, BeveledRectangleBorder.
2. We don't need to deprecate the clipBehavior. this will be useful in the Material widget to clip the widget when necessary.

[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 #135855 at sha 267511c50a1989f626540ed696eeed164b1c31b0

[TahaTesser]

I've implemented the changes. please take a look.

[HansMuller]

Is the Container necessary when isShapeProvided is true?

[TahaTesser]

Thank you! Great point. I found something important with your comment. We need border side padding even for Material widget when a custom shape is provided.

Container's decoration adds padding for the top and bottom border sides.
Here you can see total height is 50px (48px + 1px (top) + 1px (bottom))

With just DecoratedBox, there is no such border side padding.

This illustrates what's happening. We want the bottom behavior for both default and custom shapes.

To fix this I have wrapped the column with a Padding widget to fix the border side padding for default and custom shapes and replaced Container with DecoratedBox.
FYI, DecoratedBox is needed for the background color.

[flutter-dashboard]

Golden file changes are available for triage from new commit, Click here to view.

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 #135855 at sha 280636a0d9f83930402086ee6714a70d84b55713

[HansMuller]

Looking into the Google testing failure. There are still some golden image diffs however they're pretty subtle:

Expected	Actual	Difference

The difference appears to be due to about a one pixel vertical shift. Not sure what would cause that.

[flutter-dashboard]

Golden file changes are available for triage from new commit, Click here to view.

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 #135855 at sha f25c3a30dc59e0c3f73bfebe0019821a81aa9f6e

[flutter-dashboard]

Golden file changes are available for triage from new commit, Click here to view.

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 #135855 at sha 3a7209e9eded21be0f2eb888911f17130a3680f0

[flutter-dashboard]

Golden file changes are available for triage from new commit, Click here to view.

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 #135855 at sha 2be38a9edbb3320bd0822d932993083cec5c607f

[TahaTesser]

Update on this
cc: @Piinks

This is most likely to be reverted just like multiple recent PRs with ink sparkle golden tests from me and Bruno.

I'm waiting for a workaround tried by @bleroux in #137998. If it works out, i will update the golden test in this PR otherwise remove it so it doesn't get reverted.

[TahaTesser]

For context, @Piinks raised a suspicion that these PRs had to be reverted due to randomness in the sparkle effect when testing for M3.

[Piinks]

FYI - confirmed the InkSparkle.constantTurbulenceSeedSplashFactory is the right way to eliminate randomness in testing. You can set it on the ThemeData for any relevant tests like in #137998

[flutter-dashboard]

Golden file changes are available for triage from new commit, Click here to view.

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 #135855 at sha 49a3fb2050bceb03fe7383e8182381a8ead0f24f

[flutter-dashboard]

Golden file changes are available for triage from new commit, Click here to view.

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 #135855 at sha 5fd143dcabf68bfc04076358f365257c824f8c29

[TahaTesser]

@HansMuller
I have updated the test for ink sparkle issue. Can you please take a look if this breaks anything for the customers? :)

[TahaTesser]

Closing this in favor of a fresh PR with clean history and golden files