Skip to content

Conversation

@Snonky
Copy link
Contributor

@Snonky Snonky commented Jun 13, 2023

Fixes #128646

It is possible to add a MaterialGap to a MergeableMaterial without its AnimationController for the separation animation being kicked off.
This leads to the gap instantly being removed again by _removeEmptyGaps() during the next build.

The fix adds an AnimationController.forward() call where the MaterialGap insertion happens.

I found this bug while using ExpansionPanelList as the issue describes. Here is a video of an ExpansionPanelList before and after the fix. Note the gaps between the panels:

Before
2023-06-11.03-12-09.mp4
After
2023-06-13.18-29-52.mp4

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.

@github-actions github-actions bot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Jun 13, 2023
@Snonky Snonky force-pushed the fix/mergeable-material branch 2 times, most recently from e3cb7cf to eddad96 Compare June 13, 2023 17:23
@HansMuller HansMuller requested a review from QuncCccccc June 14, 2023 18:14
Copy link
Contributor

@QuncCccccc QuncCccccc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for the contribution!! This looks good:) Just left one question and some nits below.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In _insertChild method, there is a check if (child is MaterialGap) {. So should we just add this line in that check and revert the code change here in this while loop?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not always AnimationController.forward() that is used when _inserChild is called for a MaterialGap. There are cases within didUpdateWidget where it is AnimationController.reverse() or the animation is not being touched at all like in these places:

https://github.com/flutter/flutter/blob/eddad960e992a10f5c9d893f2106d3696b1cb6b4/packages/flutter/lib/src/material/mergeable_material.dart#L437-L441

https://github.com/flutter/flutter/blob/eddad960e992a10f5c9d893f2106d3696b1cb6b4/packages/flutter/lib/src/material/mergeable_material.dart#L356-L362

The handling of a gap's animation is decided for each gap insertion case.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see. Thanks for the explanation!

@Snonky Snonky force-pushed the fix/mergeable-material branch from eddad96 to ec9490c Compare July 5, 2023 21:05
Copy link
Contributor

@QuncCccccc QuncCccccc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks:)

Copy link
Contributor

@gspencergoog gspencergoog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

32384589-a60f0e74-c078-11e7-9bc1-e5b5287aea9d

@QuncCccccc QuncCccccc merged commit e0a9ad1 into flutter:master Jul 5, 2023
@Snonky Snonky deleted the fix/mergeable-material branch July 6, 2023 15:01
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 6, 2023
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Jul 6, 2023
flutter/flutter@35085c3...bc49cd1

2023-07-06 [email protected] Allow long-press gestures to continue even if buttons change. (flutter/flutter#127877)
2023-07-06 [email protected] Enable unreachable_from_main lint - it is stable now!!1 (flutter/flutter#129854)
2023-07-06 [email protected] Update labeler to new label names (flutter/flutter#130040)
2023-07-05 [email protected] MergeableMaterial: Fix adding a slice and separating it (flutter/flutter#128804)
2023-07-05 [email protected] Update infrastructure issue template for new priority scheme (flutter/flutter#129741)
2023-07-05 [email protected] Fix typo in canvas example (flutter/flutter#129879)
2023-07-05 [email protected] Reland Fix AnimatedList & AnimatedGrid doesn't apply MediaQuery padding #129556 (flutter/flutter#129860)
2023-07-05 [email protected] Change from "created via performance template" to "from: performance template" (flutter/flutter#130035)
2023-07-05 [email protected] Removes deprecated APIs from v2.6 in `binding.dart` and `widget_tester.dart` (flutter/flutter#129663)
2023-07-05 [email protected] Add new hot reload case string (flutter/flutter#130008)
2023-07-05 [email protected] Manual roll Flutter Engine from 987b621eac4e to bd2e42b203e1 (32 revisions) (flutter/flutter#130023)
2023-07-05 [email protected] Add simple unit tests for annotations.dart file (flutter/flutter#128902)
2023-07-05 [email protected] fix a bug when android uses CupertinoPageTransitionsBuilder... (flutter/flutter#114303)
2023-07-05 [email protected] Add .env file support for  option `--dart-define-from-file` (flutter/flutter#128668)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages
Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 16, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

" '_children[j] is MaterialGap': is not true " when updating state of ExpansionPanelList

3 participants