-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Fix ReorderableList return-to-origin animation (refactoring approach)
#172882
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
Fix ReorderableList return-to-origin animation (refactoring approach)
#172882
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
ReorderableList return-to-origin animation (refactoring approach)
c8c2c2a to
6bb0730
Compare
0c54351 to
cccddce
Compare
Revises proxy target calculation to convert to finalDropIndex-based coordinates before conditional logic, substantially clarifying the code and inherently resolving edge cases that previously required special-case fixes. Fixes flutter#88331 Fixes flutter#90856
Tests when dragging an item back 90% toward its original position, ensuring the proxy animates in the correct direction. Complements existing overshoot tests that test dragging back 110%.
cccddce to
2907edd
Compare
|
Self-ping to return to and compare notes over simplifying this logic. :) |
_**Note:** Alongside this PR, I've also prepared [another PR](#172882) with an alternative solution involving a more substantial refactor that addresses the root cause, rather than adding more conditional logic._ ## Description This PR fixes the proxy animation bug where dragging a `ReorderableList` item downward and then back to its original position causes it to animate to the wrong location (one position too low). ## The Problem When dragging a `ReorderableList` item downward and then back to its original position, the proxy widget briefly animates to the wrong location (one position too low) before snapping to the correct spot. **Reproduction**: Drag any item down past at least one other item, then drag it back to where it started. <p align="center"> <img src="https://github.com/user-attachments/assets/d0931dff-5600-441c-8536-2c61789767d0" alt="demo2" width="250"> </p> ## Root Cause This bug is specific to dragging an item down and then bringing it back up to nearly (but not 100% of the way ) to its original position: 1. When the item approaches its original position **from below**, `_insertIndex` becomes `item.index + 1` - This happens because Flutter's `ReorderableList` calculates `_insertIndex` with the dragged item still present in the list (see #24786) 2. The proxy _should_ animate to the item's original position at `item.index` - _But the proxy actually animates one position too low._ - This happens because `_dragEnd` incorrectly calculates `_finalDropPosition = _itemOffsetAt(_insertIndex! - 1) + _extentOffset(...)` - The `_extentOffset(...)` addition, designed for items dropping _between other items_, shifts the position down by one item's height - The correct calculation for "returning home from below" should be just `_itemOffsetAt(_insertIndex! - 1)` Note that this only occurs when returning from below (`_insertIndex > item.index`). Dragging upward (in a vertical list for example) or doesn't trigger this bug. ## Existing Implementation The existing `_dragEnd` method in `reorderable_list.dart`: ```dart void _dragEnd(_DragInfo item) { setState(() { if (_insertIndex == item.index) { _finalDropPosition = _itemOffsetAt(_insertIndex!); } else if (_reverse) { if (_insertIndex! >= _items.length) { _finalDropPosition = _itemOffsetAt(_items.length - 1) - _extentOffset(item.itemExtent, _scrollDirection); } else { _finalDropPosition = _itemOffsetAt(_insertIndex!) + _extentOffset(_itemExtentAt(_insertIndex!), _scrollDirection); } } else { if (_insertIndex! == 0) { _finalDropPosition = _itemOffsetAt(0) - _extentOffset(item.itemExtent, _scrollDirection); } else { _finalDropPosition = _itemOffsetAt(_insertIndex! - 1) + _extentOffset(_itemExtentAt(_insertIndex! - 1), _scrollDirection); } } }); } ``` When returning from below, the code falls through to the final else block, which incorrectly adds `_extentOffset`. ## Fix Detect when `_insertIndex - item.index == 1` (indicating a return to original position from below) and animate to the correct position. ```dart if (_insertIndex! - item.index == 1) { // Drop at the original position when item returns from below _finalDropPosition = _itemOffsetAt(_insertIndex! - 1); } ``` This fix was proposed by @frankpape in #90856 (comment); I've merely validated and researched the background of why the fix works, and supported it with tests. **_Demo of the fixed implementation:_** <p align="center"> <img src="https://github.com/user-attachments/assets/a53e8920-ebca-4326-abe9-3b43b34419e5" alt="fixed" width="250"> </p> Fixes #88331 Fixes #90856 Fixes #150843 ## A note about a previous PR: While investigating this issue, I found a PR addressing what seemed to be [the same exact issue](#150843): PR #151026; it turns out that that PR solved a _portion_ of the edge case: the case where an item is dragged down and back and slightly **overshoots** its original position when being dragged back & dropped—but that PR did not account for the presence of this bug when the dragged item slightly **undershoots** its original position on the return drag. This new PR effectively addresses the 'undershooting' case. With this, I've added a new pair of regression tests that are identical to the [previous PR's tests](https://github.com/flutter/flutter/blob/master/packages/flutter/test/widgets/reorderable_list_test.dart#L734), except for the fact that they simulate an undershoot on the return trip (90% of the way back instead of 110% like the original tests). This definitively captures the issue, failing in the master branch and passing in this PR's branch. Here is the specific case resolved by the [**old** PR](#151026): <table> <tr> <td align="center"> <img src="https://github.com/user-attachments/assets/b0ddc745-6e9e-4f12-97da-454e2e76b06d" alt="Before" width="200"><br> <sub>Before</sub> </td> <td align="center"> <img src="https://github.com/user-attachments/assets/03e181fa-f43b-4405-b0c0-16d3465ad990" alt="After" width="200"><br> <sub>After</sub> </td> </tr> </table> Here is the additional case resolved by **this** PR: <table> <tr> <td align="center"> <img src="https://github.com/user-attachments/assets/9b4bb591-aa2f-4cf0-88b8-a3ec32b0f0ac" alt="Before" width="200"><br> <sub>Before</sub> </td> <td align="center"> <img src="https://github.com/user-attachments/assets/31646e9c-78f4-4252-921f-53583193868f" alt="After" width="200"><br> <sub>After</sub> </td> </tr> </table> Two final observations worth noting: - The fix proposed in this PR seems to **supersede** the previous PR's solution; it addresses both cases (overshooting and undershooting) even in my tests with the [original PR's changes ](https://github.com/flutter/flutter/pull/151026/files#diff-23a4bb073009d89f09084bdf5f85232de135b8f11be625e6312bb85900a90e67) reverted. Probably best to keep the old PR's code anyway to be conservative, but noteworthy. - I also found it notable that neither this PR nor the older PR fix any issue with "reversed lists", which, in my tests, are simply not subject to this edge case as we've defined it. The regression tests added for the reverse case are thus purely precautionary. ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
…172380) _**Note:** Alongside this PR, I've also prepared [another PR](flutter#172882) with an alternative solution involving a more substantial refactor that addresses the root cause, rather than adding more conditional logic._ ## Description This PR fixes the proxy animation bug where dragging a `ReorderableList` item downward and then back to its original position causes it to animate to the wrong location (one position too low). ## The Problem When dragging a `ReorderableList` item downward and then back to its original position, the proxy widget briefly animates to the wrong location (one position too low) before snapping to the correct spot. **Reproduction**: Drag any item down past at least one other item, then drag it back to where it started. <p align="center"> <img src="https://github.com/user-attachments/assets/d0931dff-5600-441c-8536-2c61789767d0" alt="demo2" width="250"> </p> ## Root Cause This bug is specific to dragging an item down and then bringing it back up to nearly (but not 100% of the way ) to its original position: 1. When the item approaches its original position **from below**, `_insertIndex` becomes `item.index + 1` - This happens because Flutter's `ReorderableList` calculates `_insertIndex` with the dragged item still present in the list (see flutter#24786) 2. The proxy _should_ animate to the item's original position at `item.index` - _But the proxy actually animates one position too low._ - This happens because `_dragEnd` incorrectly calculates `_finalDropPosition = _itemOffsetAt(_insertIndex! - 1) + _extentOffset(...)` - The `_extentOffset(...)` addition, designed for items dropping _between other items_, shifts the position down by one item's height - The correct calculation for "returning home from below" should be just `_itemOffsetAt(_insertIndex! - 1)` Note that this only occurs when returning from below (`_insertIndex > item.index`). Dragging upward (in a vertical list for example) or doesn't trigger this bug. ## Existing Implementation The existing `_dragEnd` method in `reorderable_list.dart`: ```dart void _dragEnd(_DragInfo item) { setState(() { if (_insertIndex == item.index) { _finalDropPosition = _itemOffsetAt(_insertIndex!); } else if (_reverse) { if (_insertIndex! >= _items.length) { _finalDropPosition = _itemOffsetAt(_items.length - 1) - _extentOffset(item.itemExtent, _scrollDirection); } else { _finalDropPosition = _itemOffsetAt(_insertIndex!) + _extentOffset(_itemExtentAt(_insertIndex!), _scrollDirection); } } else { if (_insertIndex! == 0) { _finalDropPosition = _itemOffsetAt(0) - _extentOffset(item.itemExtent, _scrollDirection); } else { _finalDropPosition = _itemOffsetAt(_insertIndex! - 1) + _extentOffset(_itemExtentAt(_insertIndex! - 1), _scrollDirection); } } }); } ``` When returning from below, the code falls through to the final else block, which incorrectly adds `_extentOffset`. ## Fix Detect when `_insertIndex - item.index == 1` (indicating a return to original position from below) and animate to the correct position. ```dart if (_insertIndex! - item.index == 1) { // Drop at the original position when item returns from below _finalDropPosition = _itemOffsetAt(_insertIndex! - 1); } ``` This fix was proposed by @frankpape in flutter#90856 (comment); I've merely validated and researched the background of why the fix works, and supported it with tests. **_Demo of the fixed implementation:_** <p align="center"> <img src="https://github.com/user-attachments/assets/a53e8920-ebca-4326-abe9-3b43b34419e5" alt="fixed" width="250"> </p> Fixes flutter#88331 Fixes flutter#90856 Fixes flutter#150843 ## A note about a previous PR: While investigating this issue, I found a PR addressing what seemed to be [the same exact issue](flutter#150843): PR flutter#151026; it turns out that that PR solved a _portion_ of the edge case: the case where an item is dragged down and back and slightly **overshoots** its original position when being dragged back & dropped—but that PR did not account for the presence of this bug when the dragged item slightly **undershoots** its original position on the return drag. This new PR effectively addresses the 'undershooting' case. With this, I've added a new pair of regression tests that are identical to the [previous PR's tests](https://github.com/flutter/flutter/blob/master/packages/flutter/test/widgets/reorderable_list_test.dart#L734), except for the fact that they simulate an undershoot on the return trip (90% of the way back instead of 110% like the original tests). This definitively captures the issue, failing in the master branch and passing in this PR's branch. Here is the specific case resolved by the [**old** PR](flutter#151026): <table> <tr> <td align="center"> <img src="https://github.com/user-attachments/assets/b0ddc745-6e9e-4f12-97da-454e2e76b06d" alt="Before" width="200"><br> <sub>Before</sub> </td> <td align="center"> <img src="https://github.com/user-attachments/assets/03e181fa-f43b-4405-b0c0-16d3465ad990" alt="After" width="200"><br> <sub>After</sub> </td> </tr> </table> Here is the additional case resolved by **this** PR: <table> <tr> <td align="center"> <img src="https://github.com/user-attachments/assets/9b4bb591-aa2f-4cf0-88b8-a3ec32b0f0ac" alt="Before" width="200"><br> <sub>Before</sub> </td> <td align="center"> <img src="https://github.com/user-attachments/assets/31646e9c-78f4-4252-921f-53583193868f" alt="After" width="200"><br> <sub>After</sub> </td> </tr> </table> Two final observations worth noting: - The fix proposed in this PR seems to **supersede** the previous PR's solution; it addresses both cases (overshooting and undershooting) even in my tests with the [original PR's changes ](https://github.com/flutter/flutter/pull/151026/files#diff-23a4bb073009d89f09084bdf5f85232de135b8f11be625e6312bb85900a90e67) reverted. Probably best to keep the old PR's code anyway to be conservative, but noteworthy. - I also found it notable that neither this PR nor the older PR fix any issue with "reversed lists", which, in my tests, are simply not subject to this edge case as we've defined it. The regression tests added for the reverse case are thus purely precautionary. ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
…172380) _**Note:** Alongside this PR, I've also prepared [another PR](flutter#172882) with an alternative solution involving a more substantial refactor that addresses the root cause, rather than adding more conditional logic._ ## Description This PR fixes the proxy animation bug where dragging a `ReorderableList` item downward and then back to its original position causes it to animate to the wrong location (one position too low). ## The Problem When dragging a `ReorderableList` item downward and then back to its original position, the proxy widget briefly animates to the wrong location (one position too low) before snapping to the correct spot. **Reproduction**: Drag any item down past at least one other item, then drag it back to where it started. <p align="center"> <img src="https://github.com/user-attachments/assets/d0931dff-5600-441c-8536-2c61789767d0" alt="demo2" width="250"> </p> ## Root Cause This bug is specific to dragging an item down and then bringing it back up to nearly (but not 100% of the way ) to its original position: 1. When the item approaches its original position **from below**, `_insertIndex` becomes `item.index + 1` - This happens because Flutter's `ReorderableList` calculates `_insertIndex` with the dragged item still present in the list (see flutter#24786) 2. The proxy _should_ animate to the item's original position at `item.index` - _But the proxy actually animates one position too low._ - This happens because `_dragEnd` incorrectly calculates `_finalDropPosition = _itemOffsetAt(_insertIndex! - 1) + _extentOffset(...)` - The `_extentOffset(...)` addition, designed for items dropping _between other items_, shifts the position down by one item's height - The correct calculation for "returning home from below" should be just `_itemOffsetAt(_insertIndex! - 1)` Note that this only occurs when returning from below (`_insertIndex > item.index`). Dragging upward (in a vertical list for example) or doesn't trigger this bug. ## Existing Implementation The existing `_dragEnd` method in `reorderable_list.dart`: ```dart void _dragEnd(_DragInfo item) { setState(() { if (_insertIndex == item.index) { _finalDropPosition = _itemOffsetAt(_insertIndex!); } else if (_reverse) { if (_insertIndex! >= _items.length) { _finalDropPosition = _itemOffsetAt(_items.length - 1) - _extentOffset(item.itemExtent, _scrollDirection); } else { _finalDropPosition = _itemOffsetAt(_insertIndex!) + _extentOffset(_itemExtentAt(_insertIndex!), _scrollDirection); } } else { if (_insertIndex! == 0) { _finalDropPosition = _itemOffsetAt(0) - _extentOffset(item.itemExtent, _scrollDirection); } else { _finalDropPosition = _itemOffsetAt(_insertIndex! - 1) + _extentOffset(_itemExtentAt(_insertIndex! - 1), _scrollDirection); } } }); } ``` When returning from below, the code falls through to the final else block, which incorrectly adds `_extentOffset`. ## Fix Detect when `_insertIndex - item.index == 1` (indicating a return to original position from below) and animate to the correct position. ```dart if (_insertIndex! - item.index == 1) { // Drop at the original position when item returns from below _finalDropPosition = _itemOffsetAt(_insertIndex! - 1); } ``` This fix was proposed by @frankpape in flutter#90856 (comment); I've merely validated and researched the background of why the fix works, and supported it with tests. **_Demo of the fixed implementation:_** <p align="center"> <img src="https://github.com/user-attachments/assets/a53e8920-ebca-4326-abe9-3b43b34419e5" alt="fixed" width="250"> </p> Fixes flutter#88331 Fixes flutter#90856 Fixes flutter#150843 ## A note about a previous PR: While investigating this issue, I found a PR addressing what seemed to be [the same exact issue](flutter#150843): PR flutter#151026; it turns out that that PR solved a _portion_ of the edge case: the case where an item is dragged down and back and slightly **overshoots** its original position when being dragged back & dropped—but that PR did not account for the presence of this bug when the dragged item slightly **undershoots** its original position on the return drag. This new PR effectively addresses the 'undershooting' case. With this, I've added a new pair of regression tests that are identical to the [previous PR's tests](https://github.com/flutter/flutter/blob/master/packages/flutter/test/widgets/reorderable_list_test.dart#L734), except for the fact that they simulate an undershoot on the return trip (90% of the way back instead of 110% like the original tests). This definitively captures the issue, failing in the master branch and passing in this PR's branch. Here is the specific case resolved by the [**old** PR](flutter#151026): <table> <tr> <td align="center"> <img src="https://github.com/user-attachments/assets/b0ddc745-6e9e-4f12-97da-454e2e76b06d" alt="Before" width="200"><br> <sub>Before</sub> </td> <td align="center"> <img src="https://github.com/user-attachments/assets/03e181fa-f43b-4405-b0c0-16d3465ad990" alt="After" width="200"><br> <sub>After</sub> </td> </tr> </table> Here is the additional case resolved by **this** PR: <table> <tr> <td align="center"> <img src="https://github.com/user-attachments/assets/9b4bb591-aa2f-4cf0-88b8-a3ec32b0f0ac" alt="Before" width="200"><br> <sub>Before</sub> </td> <td align="center"> <img src="https://github.com/user-attachments/assets/31646e9c-78f4-4252-921f-53583193868f" alt="After" width="200"><br> <sub>After</sub> </td> </tr> </table> Two final observations worth noting: - The fix proposed in this PR seems to **supersede** the previous PR's solution; it addresses both cases (overshooting and undershooting) even in my tests with the [original PR's changes ](https://github.com/flutter/flutter/pull/151026/files#diff-23a4bb073009d89f09084bdf5f85232de135b8f11be625e6312bb85900a90e67) reverted. Probably best to keep the old PR's code anyway to be conservative, but noteworthy. - I also found it notable that neither this PR nor the older PR fix any issue with "reversed lists", which, in my tests, are simply not subject to this edge case as we've defined it. The regression tests added for the reverse case are thus purely precautionary. ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
…172380) _**Note:** Alongside this PR, I've also prepared [another PR](flutter#172882) with an alternative solution involving a more substantial refactor that addresses the root cause, rather than adding more conditional logic._ ## Description This PR fixes the proxy animation bug where dragging a `ReorderableList` item downward and then back to its original position causes it to animate to the wrong location (one position too low). ## The Problem When dragging a `ReorderableList` item downward and then back to its original position, the proxy widget briefly animates to the wrong location (one position too low) before snapping to the correct spot. **Reproduction**: Drag any item down past at least one other item, then drag it back to where it started. <p align="center"> <img src="https://github.com/user-attachments/assets/d0931dff-5600-441c-8536-2c61789767d0" alt="demo2" width="250"> </p> ## Root Cause This bug is specific to dragging an item down and then bringing it back up to nearly (but not 100% of the way ) to its original position: 1. When the item approaches its original position **from below**, `_insertIndex` becomes `item.index + 1` - This happens because Flutter's `ReorderableList` calculates `_insertIndex` with the dragged item still present in the list (see flutter#24786) 2. The proxy _should_ animate to the item's original position at `item.index` - _But the proxy actually animates one position too low._ - This happens because `_dragEnd` incorrectly calculates `_finalDropPosition = _itemOffsetAt(_insertIndex! - 1) + _extentOffset(...)` - The `_extentOffset(...)` addition, designed for items dropping _between other items_, shifts the position down by one item's height - The correct calculation for "returning home from below" should be just `_itemOffsetAt(_insertIndex! - 1)` Note that this only occurs when returning from below (`_insertIndex > item.index`). Dragging upward (in a vertical list for example) or doesn't trigger this bug. ## Existing Implementation The existing `_dragEnd` method in `reorderable_list.dart`: ```dart void _dragEnd(_DragInfo item) { setState(() { if (_insertIndex == item.index) { _finalDropPosition = _itemOffsetAt(_insertIndex!); } else if (_reverse) { if (_insertIndex! >= _items.length) { _finalDropPosition = _itemOffsetAt(_items.length - 1) - _extentOffset(item.itemExtent, _scrollDirection); } else { _finalDropPosition = _itemOffsetAt(_insertIndex!) + _extentOffset(_itemExtentAt(_insertIndex!), _scrollDirection); } } else { if (_insertIndex! == 0) { _finalDropPosition = _itemOffsetAt(0) - _extentOffset(item.itemExtent, _scrollDirection); } else { _finalDropPosition = _itemOffsetAt(_insertIndex! - 1) + _extentOffset(_itemExtentAt(_insertIndex! - 1), _scrollDirection); } } }); } ``` When returning from below, the code falls through to the final else block, which incorrectly adds `_extentOffset`. ## Fix Detect when `_insertIndex - item.index == 1` (indicating a return to original position from below) and animate to the correct position. ```dart if (_insertIndex! - item.index == 1) { // Drop at the original position when item returns from below _finalDropPosition = _itemOffsetAt(_insertIndex! - 1); } ``` This fix was proposed by @frankpape in flutter#90856 (comment); I've merely validated and researched the background of why the fix works, and supported it with tests. **_Demo of the fixed implementation:_** <p align="center"> <img src="https://github.com/user-attachments/assets/a53e8920-ebca-4326-abe9-3b43b34419e5" alt="fixed" width="250"> </p> Fixes flutter#88331 Fixes flutter#90856 Fixes flutter#150843 ## A note about a previous PR: While investigating this issue, I found a PR addressing what seemed to be [the same exact issue](flutter#150843): PR flutter#151026; it turns out that that PR solved a _portion_ of the edge case: the case where an item is dragged down and back and slightly **overshoots** its original position when being dragged back & dropped—but that PR did not account for the presence of this bug when the dragged item slightly **undershoots** its original position on the return drag. This new PR effectively addresses the 'undershooting' case. With this, I've added a new pair of regression tests that are identical to the [previous PR's tests](https://github.com/flutter/flutter/blob/master/packages/flutter/test/widgets/reorderable_list_test.dart#L734), except for the fact that they simulate an undershoot on the return trip (90% of the way back instead of 110% like the original tests). This definitively captures the issue, failing in the master branch and passing in this PR's branch. Here is the specific case resolved by the [**old** PR](flutter#151026): <table> <tr> <td align="center"> <img src="https://github.com/user-attachments/assets/b0ddc745-6e9e-4f12-97da-454e2e76b06d" alt="Before" width="200"><br> <sub>Before</sub> </td> <td align="center"> <img src="https://github.com/user-attachments/assets/03e181fa-f43b-4405-b0c0-16d3465ad990" alt="After" width="200"><br> <sub>After</sub> </td> </tr> </table> Here is the additional case resolved by **this** PR: <table> <tr> <td align="center"> <img src="https://github.com/user-attachments/assets/9b4bb591-aa2f-4cf0-88b8-a3ec32b0f0ac" alt="Before" width="200"><br> <sub>Before</sub> </td> <td align="center"> <img src="https://github.com/user-attachments/assets/31646e9c-78f4-4252-921f-53583193868f" alt="After" width="200"><br> <sub>After</sub> </td> </tr> </table> Two final observations worth noting: - The fix proposed in this PR seems to **supersede** the previous PR's solution; it addresses both cases (overshooting and undershooting) even in my tests with the [original PR's changes ](https://github.com/flutter/flutter/pull/151026/files#diff-23a4bb073009d89f09084bdf5f85232de135b8f11be625e6312bb85900a90e67) reverted. Probably best to keep the old PR's code anyway to be conservative, but noteworthy. - I also found it notable that neither this PR nor the older PR fix any issue with "reversed lists", which, in my tests, are simply not subject to this edge case as we've defined it. The regression tests added for the reverse case are thus purely precautionary. ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
…172380) _**Note:** Alongside this PR, I've also prepared [another PR](flutter#172882) with an alternative solution involving a more substantial refactor that addresses the root cause, rather than adding more conditional logic._ ## Description This PR fixes the proxy animation bug where dragging a `ReorderableList` item downward and then back to its original position causes it to animate to the wrong location (one position too low). ## The Problem When dragging a `ReorderableList` item downward and then back to its original position, the proxy widget briefly animates to the wrong location (one position too low) before snapping to the correct spot. **Reproduction**: Drag any item down past at least one other item, then drag it back to where it started. <p align="center"> <img src="https://github.com/user-attachments/assets/d0931dff-5600-441c-8536-2c61789767d0" alt="demo2" width="250"> </p> ## Root Cause This bug is specific to dragging an item down and then bringing it back up to nearly (but not 100% of the way ) to its original position: 1. When the item approaches its original position **from below**, `_insertIndex` becomes `item.index + 1` - This happens because Flutter's `ReorderableList` calculates `_insertIndex` with the dragged item still present in the list (see flutter#24786) 2. The proxy _should_ animate to the item's original position at `item.index` - _But the proxy actually animates one position too low._ - This happens because `_dragEnd` incorrectly calculates `_finalDropPosition = _itemOffsetAt(_insertIndex! - 1) + _extentOffset(...)` - The `_extentOffset(...)` addition, designed for items dropping _between other items_, shifts the position down by one item's height - The correct calculation for "returning home from below" should be just `_itemOffsetAt(_insertIndex! - 1)` Note that this only occurs when returning from below (`_insertIndex > item.index`). Dragging upward (in a vertical list for example) or doesn't trigger this bug. ## Existing Implementation The existing `_dragEnd` method in `reorderable_list.dart`: ```dart void _dragEnd(_DragInfo item) { setState(() { if (_insertIndex == item.index) { _finalDropPosition = _itemOffsetAt(_insertIndex!); } else if (_reverse) { if (_insertIndex! >= _items.length) { _finalDropPosition = _itemOffsetAt(_items.length - 1) - _extentOffset(item.itemExtent, _scrollDirection); } else { _finalDropPosition = _itemOffsetAt(_insertIndex!) + _extentOffset(_itemExtentAt(_insertIndex!), _scrollDirection); } } else { if (_insertIndex! == 0) { _finalDropPosition = _itemOffsetAt(0) - _extentOffset(item.itemExtent, _scrollDirection); } else { _finalDropPosition = _itemOffsetAt(_insertIndex! - 1) + _extentOffset(_itemExtentAt(_insertIndex! - 1), _scrollDirection); } } }); } ``` When returning from below, the code falls through to the final else block, which incorrectly adds `_extentOffset`. ## Fix Detect when `_insertIndex - item.index == 1` (indicating a return to original position from below) and animate to the correct position. ```dart if (_insertIndex! - item.index == 1) { // Drop at the original position when item returns from below _finalDropPosition = _itemOffsetAt(_insertIndex! - 1); } ``` This fix was proposed by @frankpape in flutter#90856 (comment); I've merely validated and researched the background of why the fix works, and supported it with tests. **_Demo of the fixed implementation:_** <p align="center"> <img src="https://github.com/user-attachments/assets/a53e8920-ebca-4326-abe9-3b43b34419e5" alt="fixed" width="250"> </p> Fixes flutter#88331 Fixes flutter#90856 Fixes flutter#150843 ## A note about a previous PR: While investigating this issue, I found a PR addressing what seemed to be [the same exact issue](flutter#150843): PR flutter#151026; it turns out that that PR solved a _portion_ of the edge case: the case where an item is dragged down and back and slightly **overshoots** its original position when being dragged back & dropped—but that PR did not account for the presence of this bug when the dragged item slightly **undershoots** its original position on the return drag. This new PR effectively addresses the 'undershooting' case. With this, I've added a new pair of regression tests that are identical to the [previous PR's tests](https://github.com/flutter/flutter/blob/master/packages/flutter/test/widgets/reorderable_list_test.dart#L734), except for the fact that they simulate an undershoot on the return trip (90% of the way back instead of 110% like the original tests). This definitively captures the issue, failing in the master branch and passing in this PR's branch. Here is the specific case resolved by the [**old** PR](flutter#151026): <table> <tr> <td align="center"> <img src="https://github.com/user-attachments/assets/b0ddc745-6e9e-4f12-97da-454e2e76b06d" alt="Before" width="200"><br> <sub>Before</sub> </td> <td align="center"> <img src="https://github.com/user-attachments/assets/03e181fa-f43b-4405-b0c0-16d3465ad990" alt="After" width="200"><br> <sub>After</sub> </td> </tr> </table> Here is the additional case resolved by **this** PR: <table> <tr> <td align="center"> <img src="https://github.com/user-attachments/assets/9b4bb591-aa2f-4cf0-88b8-a3ec32b0f0ac" alt="Before" width="200"><br> <sub>Before</sub> </td> <td align="center"> <img src="https://github.com/user-attachments/assets/31646e9c-78f4-4252-921f-53583193868f" alt="After" width="200"><br> <sub>After</sub> </td> </tr> </table> Two final observations worth noting: - The fix proposed in this PR seems to **supersede** the previous PR's solution; it addresses both cases (overshooting and undershooting) even in my tests with the [original PR's changes ](https://github.com/flutter/flutter/pull/151026/files#diff-23a4bb073009d89f09084bdf5f85232de135b8f11be625e6312bb85900a90e67) reverted. Probably best to keep the old PR's code anyway to be conservative, but noteworthy. - I also found it notable that neither this PR nor the older PR fix any issue with "reversed lists", which, in my tests, are simply not subject to this edge case as we've defined it. The regression tests added for the reverse case are thus purely precautionary. ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
This PR is an alternative to PR #172380, proposing a more substantial refactor to
ReorderableList._dragEnd(...)to address the root cause, rather than adding more conditional logic.Description
This PR fixes the proxy animation bug where dragging a
ReorderableListitem downward and then back to its original position causes it to animate to the wrong location (one position too low).The Problem
When dragging a
ReorderableListitem downward and then back to its original position, the proxy widget briefly animates to the wrong location (one position too low) before snapping to the correct spot.Reproduction: Drag any item down past at least one other item, then drag it back to where it started.
Root Cause
The bug stems from a fundamental mismatch: Flutter's
onReordercallback receives an insertion index (calculated with the dragged item still in the list), but_dragEndneeds to animate to where the item will actually end up. This creates complex logic trying to work backwards from insertion point to final position, especially when dragging down where these indices differ by 1.This complexity exists because of #24786:
onReorderpasses an insertion index rather than a final index.Solution
Rather than adding another special case, this PR separates the concerns:
_insertionIndexToFinalIndexhelper explicitly converts from insertion index to final position_getProxyAnimationTargetmethod handles all position calculations using the final index_dragEnd: Now just orchestrates by calling_getProxyAnimationTargetThis architectural change makes the correct behavior emerge naturally from the abstraction rather than from conditional patches.
Integration test running on patched code:
Before/After
Before: Mixed concerns with complex conditionals
After: Separated concerns with clear abstractions
By separating index conversion from position calculation, the bug fix emerges naturally: we simply calculate where the item will actually be, rather than trying to patch the symptoms of working with the wrong index type.
Fixes #88331
Fixes #90856
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.