mirror of
https://github.com/flutter/flutter.git
synced 2026-02-20 02:29:02 +08:00
b6e78b9305
_**Note:** Alongside this PR, I've also prepared [another PR](https://github.com/flutter/flutter/pull/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 https://github.com/flutter/flutter/issues/90856#issuecomment-2599368939; 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](https://github.com/flutter/flutter/issues/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](https://github.com/flutter/flutter/pull/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