Draggable feedback positioning

[timcreatedit]

We changed the coordinates used to position the Draggable feedback by transforming them into the Overlays coordinate space. This has no influence on any untransformed Overlay, most Flutter apps should be not affected.
This PR fixes the positioning of the feedback in transformed context (see #145639 for before video):

Screenshot.2024-03-23.at.20.30.06.mp4

• fixes Draggable Feedback is positioned incorrectly when the Overlay is scaled #145639

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]

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!).

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.

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[goderbauer]

From triage: @timcreatedit looks like this is failing some checks. Could you take a look? It also appears to be missing a test. Could you please add one? Thank you!

[timcreatedit]

I will take a look, yes! I'll add a test for the behaviour, I wasn't sure if that's desired since the class I touched seems to be untested.
Regarding the checks, it seems like these build checks failed for all everybody during the time I opened this, but that means once they re-run they should pass.

[goderbauer]

Looks like there are still some failures. Some of them look related to this change, e.g. https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8752209309943481745/+/u/run_test.dart_for_framework_tests_shard_and_subshard_widgets/stdout

[timcreatedit]

I can reproduce the test failures in test/widgets/draggable_test.dart, but I'm unsure where they come from. I'm investigating

[timcreatedit]

I think I have found the issue, however, I don't really understand why this change triggered the failures.

Some tests in draggable_test.dart don't finish their gestures. With the current state of the PR, all tests pass in isolation, but when these tests are run back to back, they fail because resources are not cleaned up properly.

Adding the following lines to the end of these tests resolves all test failures:

// Finish gesture to release resources.
await gesture.up();
await tester.pump();

Finishing gestures like this seems to be common practice anyways, and is done in many other tests within the same file.
3df4223 adds these changes to the tests, although I'm not sure if this should be done within this PR.

[timcreatedit]

@goderbauer is there anything I should still do before this can be reviewed?

[goderbauer]

LGTM

[justinmc]

LGTM with a nit and a question.

Thanks for fixing this, I was just noticing another place where we weren't doing transforms properly (#146019).

[justinmc]

It seems weird to me that we're subtracting dragStartPoint both from a point in global space (_lastOffset = globalPosition - dragStartPoint;) and from a point in local/overlay space (_overlayOffset = overlaySpaceOffset - dragStartPoint;). Wouldn't it be in the wrong coordinate space in one of those operations?

I guess based on your test and your video demo it works, though, so maybe I'm misunderstanding what dragStartPoint is.

[timcreatedit]

I had the same question at first. This transformation is definitely correct for the purposes of _overlayOffset and I've added another test to verify that. However, it might be incorrect for _lastOffset, which we purposefully haven't touched. We wanted this PR to focus on the visual feedback only, since we thought that would be least likely to break existing Flutter code. Adjusting _lastOffset_ can be done in another PR, where we could then make sure all Draggable callbacks use the Overlay space instead of global coordinates as they do now.

[justinmc]

Ok sounds good. I agree that it looks like dragStartPoint is not in the same coordinate space as globalPosition, but _overlayOffset should be right.

[jmagman]

Reason for revert: Failing draggable_test in post-submit.

[jmagman]

10:24 +15103 ~145: /Volumes/Work/s/w/ir/x/w/flutter/packages/flutter/test/widgets/draggable_test.dart: Drag and drop - DragTarget<Object> can accept Draggable<int> data
══╡ EXCEPTION CAUGHT BY GESTURE LIBRARY ╞═══════════════════════════════════════════════════════════
The following assertion was thrown while routing a pointer event:
This widget has been unmounted, so the State no longer has a context (and should be considered
defunct).
Consider canceling any active work during "dispose" or using the "mounted" getter to determine if
the State is still active.

When the exception was thrown, this was the stack:
#0      State.context.<anonymous closure> (package:flutter/src/widgets/framework.dart:951:9)
#1      State.context (package:flutter/src/widgets/framework.dart:957:6)
#2      _DragAvatar.updateDrag (package:flutter/src/widgets/drag_target.dart:862:40)
#3      _DragAvatar.update (package:flutter/src/widgets/drag_target.dart:843:5)
#4      MultiDragPointerState._move (package:flutter/src/gestures/multidrag.dart:100:16)
#5      MultiDragGestureRecognizer._handleEvent (package:flutter/src/gestures/multidrag.dart:267:13)
#6      PointerRouter._dispatch (package:flutter/src/gestures/pointer_router.dart:98:12)
#7      PointerRouter._dispatchEventToRoutes.<anonymous closure> (package:flutter/src/gestures/pointer_router.dart:143:9)
#8      _LinkedHashMapMixin.forEach (dart:collection-patch/compact_hash.dart:633:13)
#9      PointerRouter._dispatchEventToRoutes (package:flutter/src/gestures/pointer_router.dart:141:18)
#10     PointerRouter.route (package:flutter/src/gestures/pointer_router.dart:127:7)
#11     GestureBinding.handleEvent (package:flutter/src/gestures/binding.dart:495:19)
#12     GestureBinding.dispatchEvent (package:flutter/src/gestures/binding.dart:475:22)
#13     RendererBinding.dispatchEvent (package:flutter/src/rendering/binding.dart:450:11)
#14     GestureBinding._handlePointerEventImmediately (package:flutter/src/gestures/binding.dart:420:7)
#15     GestureBinding.handlePointerEvent (package:flutter/src/gestures/binding.dart:383:5)
#16     TestWidgetsFlutterBinding.handlePointerEventForSource.<anonymous closure> (package:flutter_test/src/binding.dart:662:42)
#17     TestWidgetsFlutterBinding.withPointerEventSource (package:flutter_test/src/binding.dart:672:11)
#18     TestWidgetsFlutterBinding.handlePointerEventForSource (package:flutter_test/src/binding.dart:662:5)
#19     WidgetTester.sendEventToBinding.<anonymous closure> (package:flutter_test/src/widget_tester.dart:868:15)
#22     TestAsyncUtils.guard (package:flutter_test/src/test_async_utils.dart:71:41)
#23     WidgetTester.sendEventToBinding (package:flutter_test/src/widget_tester.dart:867:27)
#24     TestGesture.moveTo.<anonymous closure> (package:flutter_test/src/test_pointer.dart:533:27)
#27     TestAsyncUtils.guard (package:flutter_test/src/test_async_utils.dart:71:41)
#28     TestGesture.moveTo (package:flutter_test/src/test_pointer.dart:531:27)
#29     main.<anonymous closure> (file:///Volumes/Work/s/w/ir/x/w/flutter/packages/flutter/test/widgets/draggable_test.dart:3246:19)
<asynchronous suspension>
#30     testWidgets.<anonymous closure>.<anonymous closure> (package:flutter_test/src/widget_tester.dart:183:15)
<asynchronous suspension>
#31     TestWidgetsFlutterBinding._runTestBody (package:flutter_test/src/binding.dart:1017:5)
<asynchronous suspension>
<asynchronous suspension>
(elided 5 frames from dart:async and package:stack_trace)

router: Instance of 'PointerRouter'
route: Closure: (PointerEvent) => void from Function '_handleEvent@425470698':.
event: PointerMoveEvent#b34eb(position: Offset(400.0, 98.0))
════════════════════════════════════════════════════════════════════════════════════════════════════

https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8749120090464225025/+/u/run_test.dart_for_framework_tests_shard_and_subshard_impeller/stdout

[justinmc]

@timcreatedit It looks like that's not a test you touched directly. Is it related to the gesture.up calls you were adding?