Draggable feedback positioning #149040

timcreatedit · 2024-05-24T12:16:19Z

Reopened after revert in #147658

Another test was added in the meantime to draggable_test.dart that didn't call gesture.up(). I added this call to the test and now all tests pass.

Original Description (#145647):

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.

dkwingsmt

LGTM!

justinmc

LGTM 👍. Thanks for following up after the revert.

I wish we had an automated way to catch this lack of .up calls. I was just asking @polina-c about this.

justinmc · 2024-05-28T20:01:05Z

packages/flutter/test/widgets/draggable_test.dart

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


Nit: Should this be pump? Just noticing that it's usually pump elsewhere. Same for the two new tests below.

Good point, works just as well with pump! I think I just copied the three lines from the test right above the ones I added without thinking too much about it. Fixed!

dkwingsmt · 2024-05-28T22:06:25Z

I wish we had an automated way to catch this lack of .up calls. I was just asking @polina-c about this.

When I worked on mouse we immediately add gesture.up to tear down after the startGesture. Not sure if it works here.

However a bigger problem for me is that, the error seems to be triggered by the case where the dragging widget is unmounted during a drag, which is a totally normal case. Will this case trigger error in prod? If so we should fix this case instead of avoiding them in the tests.

dkwingsmt · 2024-05-28T22:20:38Z

Ok, I've tested that this will trigger error if the root widget (which supposedly provides the overlay) is take away during the drag.

Reproduction:

Apply this PR (or simply access the context during _DragAvatar.updateDrag

  void updateDrag(Offset globalPosition) {
    final RenderBox box = overlayState.context.findRenderObject()! as RenderBox;
    final Offset overlaySpaceOffset = box.globalToLocal(globalPosition);
    print(overlaySpaceOffset - dragStartPoint);

Run the following app:

import 'package:flutter/material.dart';

class Main extends StatefulWidget {

  @override
  _MainState createState() => _MainState();
}

class _MainState extends State<Main> {
  bool _show = true;

  @override
  Widget build(BuildContext context) {
    return !_show ? Container() : MaterialApp(
      home: ListView(
        scrollDirection: Axis.horizontal,
        children: <Widget>[
          DragTarget<int>(
            builder: (BuildContext context, List<int?> data, List<dynamic> rejects) {
              return const Text('Target');
            },
            onAcceptWithDetails: (DragTargetDetails<int> _) {
            },
          ),
          Container(width: 400.0),
          Draggable<int>(
            data: 1,
            feedback: Text('H'),
            onDragStarted: () {
              Future<void>.delayed(Duration(seconds: 2)).then((_) {
                setState(() {
                  _show = false;
                });
              });
            },
            childWhenDragging: SizedBox(),
            axis: Axis.horizontal,
            child: Text('H'),
          ),
        ],
      ),
    );
  }
}

void main() {
  runApp(Main());
}

Drag the H and hold for 2 seconds before moving

Throws error:


══╡ 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:857:40)
#3      _DragAvatar.update (package:flutter/src/widgets/drag_target.dart:839: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     GestureBinding._flushPointerEventQueue (package:flutter/src/gestures/binding.dart:330:7)
#17     GestureBinding._handlePointerDataPacket (package:flutter/src/gestures/binding.dart:299:9)
#18     _invoke1 (dart:ui/hooks.dart:328:13)
#19     PlatformDispatcher._dispatchPointerDataPacket (dart:ui/platform_dispatcher.dart:442:7)
#20     _dispatchPointerDataPacket (dart:ui/hooks.dart:262:31)

router: Instance of 'PointerRouter'
route: Closure: (PointerEvent) => void from Function '_handleEvent@171470698':.
event: PointerMoveEvent#dc09b(position: Offset(379.5, 32.1))
════════════════════════════════════════════════════════════════════════════════════════════════════

Another exception was thrown: This widget has been unmounted, so the State no longer has a context (and should be considered defunct).

We should probably fix this error instead of patching the tests.

dkwingsmt

See comment

timcreatedit · 2024-05-28T22:35:58Z

Thanks for finding this, that's very insightful. As I mentioned on the other PR it didn't quite sit right with me that the test failures were triggered by the changes. I will take a look at this again!

timcreatedit · 2024-06-12T08:45:15Z

@dkwingsmt I added a new test that verifies the behaviour you found when unmounting, and added a mounted check to the context access in _DragAvatar.updateDrag. Now the test passes. Do you want me to revert the changes I made to other tests, or do we still want to finish the gestures there? I reckon since unmounting is now covered by a test, we might as well leave it in?

Thanks again for your help!

dkwingsmt

LGTM, thanks for working on this!

dkwingsmt · 2024-06-13T17:18:54Z

I would prefer removing the gesture.ups in existing tests since they are unneeded changes (for the PR) and unneeded lines (for the tests), but I'm not strong on this.

Awesome test for the graceful unmounting, BTW!

justinmc

LGTM, but see the analyzer failure.

timcreatedit · 2024-06-19T18:10:30Z

I would prefer removing the gesture.ups in existing tests since they are unneeded changes (for the PR) and unneeded lines (for the tests), but I'm not strong on this.

Awesome test for the graceful unmounting, BTW!

Removed them all! Thanks :)

timcreatedit · 2024-06-19T18:11:22Z

LGTM, but see the analyzer failure.

@justinmc Ready, it's all green 🌿

timcreatedit · 2024-06-28T15:33:56Z

@dkwingsmt @justinmc Just wanted to touch base again, is there anything you still need from me?

dkwingsmt · 2024-06-28T18:14:38Z

Nope. Here we go! Thanks for the work!

Manual roll requested by [email protected] flutter/flutter@15f95ce...651a17d 2024-06-28 [email protected] Roll Flutter Engine from a78f5ce743ce to 2f7e9ab27493 (11 revisions) (flutter/flutter#151002) 2024-06-28 [email protected] Draggable feedback positioning (flutter/flutter#149040) 2024-06-28 [email protected] Add support for type-safe plugin apply (flutter/flutter#150958) 2024-06-28 [email protected] Use caret syntax with flutter create command (flutter/flutter#150920) 2024-06-28 [email protected] Roll Packages from 03f5f6d to 412ec46 (12 revisions) (flutter/flutter#150985) 2024-06-28 [email protected] [flutter_tools] Include more details in structured errors sent to a DAP client (flutter/flutter#150698) 2024-06-28 [email protected] Roll Flutter Engine from 94591ffb20df to a78f5ce743ce (1 revision) (flutter/flutter#150972) 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://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

Reopened after revert in flutter#147658 Another test was added in the meantime to `draggable_test.dart` that didn't call `gesture.up()`. I added this call to the test and now all tests pass. --- # Original Description (flutter#145647): We changed the coordinates used to position the `Draggable` feedback by transforming them into the `Overlay`s 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 flutter#145639 for before video): https://github.com/flutter/flutter/assets/42270125/df34e198-0667-453d-a27a-a79b2e2825a1 - fixes flutter#145639 ## 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. If you need help, consider asking for advice on the #hackers-new channel on [Discord].  [Contributor Guide]: https://github.com/flutter/flutter/wiki/Tree-hygiene#overview [Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene [test-exempt]: https://github.com/flutter/flutter/wiki/Tree-hygiene#tests [Flutter Style Guide]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo [Features we expect every widget to implement]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#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/wiki/Tree-hygiene#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/wiki/Chat [Data Driven Fixes]: https://github.com/flutter/flutter/wiki/Data-driven-Fixes --------- Co-authored-by: Jesper Bellenbaum <[email protected]>

github-actions bot added the framework flutter/packages/flutter repository. See also f: labels. label May 24, 2024

justinmc requested review from dkwingsmt and justinmc May 24, 2024 21:09

dkwingsmt approved these changes May 28, 2024

View reviewed changes

justinmc approved these changes May 28, 2024

View reviewed changes

timcreatedit requested a review from justinmc May 28, 2024 21:57

dkwingsmt requested changes May 28, 2024

View reviewed changes

timcreatedit requested a review from dkwingsmt June 12, 2024 08:45

dkwingsmt approved these changes Jun 13, 2024

View reviewed changes

justinmc approved these changes Jun 13, 2024

View reviewed changes

timcreatedit and others added 15 commits June 19, 2024 15:28

use overlay coordinate space for draggable feedback

5f9362e

use late

790cad5

added test for non-transformed draggable

30b669a

added tests for scaled and unscaled apps

cc95a43

added expected header to new test file and formatted it

739a3c2

finish gestures in tests

a21381e

moved tests to correct file

045ddeb

finished gestures on tests in draggable_test.dart

092898f

added test for rotation

0c53028

added test for dragAnchorStrategy

3bfd162

removed unneccessary import

03e4268

released resources in remaining test in draggable_test.dart

5bda9f6

use pump instead of pumpAndSettle in new tests

b0af932

added failing test for unmounting overlay during drag

aa1cd19

added mounted check to updating drag target position

df1da61

removed gesture.up calls

5d331e1

timcreatedit force-pushed the draggable-feedback-positioning branch from 01ab09b to 5d331e1 Compare June 19, 2024 13:28

added explanatory comment to unmount test

bd08017

dkwingsmt merged commit f5b671a into flutter:master Jun 28, 2024

engine-flutter-autoroll mentioned this pull request Jun 28, 2024

Roll Flutter from e726eb401c2c to f5b671aac723 (54 revisions) flutter/packages#7009

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 28, 2024

f5b671a Draggable feedback positioning (flutter/flutter#149040)

847a1da

engine-flutter-autoroll mentioned this pull request Jun 28, 2024

Manual roll Flutter from 15f95ce0c38e to 651a17db5427 (7 revisions) flutter/packages#7013

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 28, 2024

f5b671a Draggable feedback positioning (flutter/flutter#149040)

fabf66c

polina-c mentioned this pull request Jul 1, 2024

Clean up leaky test. #151131

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 6, 2024

f5b671a Draggable feedback positioning (flutter/flutter#149040)

822f23d

This was referenced Jul 23, 2025

draggable feedback location Codelessly/ResponsiveFramework#17

Closed

List reordering visual glitch problem Codelessly/ResponsiveFramework#195

Open

Draggable feedback positioning #149040

Draggable feedback positioning #149040

Uh oh!

Conversation

timcreatedit commented May 24, 2024

Original Description (#145647):

Pre-launch Checklist

Uh oh!

dkwingsmt left a comment

Choose a reason for hiding this comment

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

justinmc May 28, 2024

Choose a reason for hiding this comment

Uh oh!

timcreatedit May 28, 2024

Choose a reason for hiding this comment

Uh oh!

dkwingsmt commented May 28, 2024

Uh oh!

dkwingsmt commented May 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dkwingsmt left a comment

Choose a reason for hiding this comment

Uh oh!

timcreatedit commented May 28, 2024

Uh oh!

timcreatedit commented Jun 12, 2024

Uh oh!

dkwingsmt left a comment

Choose a reason for hiding this comment

Uh oh!

dkwingsmt commented Jun 13, 2024

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

timcreatedit commented Jun 19, 2024

Uh oh!

timcreatedit commented Jun 19, 2024

Uh oh!

timcreatedit commented Jun 28, 2024

Uh oh!

dkwingsmt commented Jun 28, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dkwingsmt commented May 28, 2024 •

edited

Loading