Skip to content

Fix: Magnifier appears and won't dismiss #128545

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.

Sign up for GitHub

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

Jump to bottom
Merged
justinmc merged 8 commits into from
Jun 20, 2023
Merged

Fix: Magnifier appears and won't dismiss #128545

justinmc merged 8 commits into from
Jun 20, 2023

Conversation

@justinmc
Copy link
Contributor

@justinmc justinmc commented Jun 9, 2023 •
edited
Loading

When tapping just outside of a TextField that unfocuses itself, the magnifier appears and cannot be dismissed.

It seems like the selection handles are being built with opacity 0.0 but are still receiving the tap (as a drag start). In the image below, the GestureDetector is given in yellow, which includes the area you need to tap to reproduce this bug:

Screenshot from 2023-06-13 10-52-56

Furthermore, I noticed that some drag event callbacks were being called even after the _handles had been removed by the tap. This seems to have been caused by OverlayEntry.remove happening in the following frame instead of immediately. In the meantime, the handle was still able to receive the drag gesture.

Fixes #128321

@justinmc justinmc self-assigned this Jun 9, 2023
@github-actions github-actions bot added a: text input Entering text in a text field or keyboard related problems f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Jun 9, 2023
@justinmc justinmc marked this pull request as ready for review June 14, 2023 17:59
Renzo-Olivares
Renzo-Olivares approved these changes Jun 14, 2023
View reviewed changes
Copy link
Contributor

@Renzo-Olivares Renzo-Olivares left a comment

Choose a reason for hiding this comment

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

LGTM

packages/flutter/lib/src/widgets/text_selection.dart
// Make sure the GestureDetector is big enough to be easily interactive.
final Rect interactiveRect = handleRect.expandToInclude(
Rect.fromCircle(center: handleRect.center, radius: kMinInteractiveDimension/ 2),
Rect.fromCircle(center: handleRect.center, radius: kMinInteractiveDimension / 2),
Copy link
Contributor

@Renzo-Olivares Renzo-Olivares Jun 14, 2023

Choose a reason for hiding this comment

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

Nice!

packages/flutter/lib/src/widgets/text_selection.dart
final ValueChanged<DragStartDetails>? onStartHandleDragStart;

void _handleStartHandleDragStart(DragStartDetails details) {
// Calling OverlayEntry.remove may not happen until the following frame, so
Copy link
Contributor

@Renzo-Olivares Renzo-Olivares Jun 14, 2023

Choose a reason for hiding this comment

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

Good catch! This seems like a safe fix to me.

LongCatIsLooong
LongCatIsLooong reviewed Jun 15, 2023
View reviewed changes
packages/flutter/test/material/text_field_test.dart Outdated
child: TextField(
magnifierConfiguration: TextMagnifierConfiguration(
magnifierBuilder: (
_,
Copy link
Contributor

@LongCatIsLooong LongCatIsLooong Jun 15, 2023

Choose a reason for hiding this comment

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

nit: avoid anonymous parameter names.

Copy link
Contributor

@LongCatIsLooong LongCatIsLooong Jun 15, 2023

Choose a reason for hiding this comment

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

On a side note, Github stopped syntax highlighting the file for me. Maybe that's a sign we should break this file down...

packages/flutter/lib/src/widgets/text_selection.dart
void _handleStartHandleDragEnd(DragEndDetails details) {
// Calling OverlayEntry.remove may not happen until the following frame, so
// it's possible for the handles to receive a gesture after calling remove.
if (_handles == null) {
Copy link
Contributor

@LongCatIsLooong LongCatIsLooong Jun 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Would this result in a unbalanced call? You're handling a handle drag event and expect an end event at some point, but you never receive an end event because the handle being dragged becomes invisible?

Copy link
Contributor Author

@justinmc justinmc Jun 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

That's true, but it's the same as the current behavior. _handleStart/EndHandleDragEnd is never called when the bug is reproduced.

I just tried it and it looks like the same thing will happen if you suddenly remove a GestureDetector from the Widget tree:

Example of removing a GestureDetector while it's detecting a drag. Drag end is never called. 
import 'package:flutter/material.dart';

void main() {
  runApp(const MyApp());
}

class MyApp extends StatelessWidget {
  const MyApp({super.key});

  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      title: 'Flutter Demo',
      theme: ThemeData(
        primarySwatch: Colors.blue,
      ),
      home: const MyHomePage(title: 'Flutter Demo Home Page'),
    );
  }
}

class MyHomePage extends StatelessWidget {
  const MyHomePage({super.key, required this.title});

  final String title;

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      appBar: AppBar(
        title: Text(title),
      ),
      body: Center(
        child: Column(
          mainAxisAlignment: MainAxisAlignment.center,
          children: <Widget>[
            DisappearingWidget(),
          ],
        ),
      ),
    );
  }
}

class DisappearingWidget extends StatefulWidget {
  const DisappearingWidget({super.key});

  @override
  State<DisappearingWidget> createState() => _DisappearingWidgetState();
}

class _DisappearingWidgetState extends State<DisappearingWidget> {
  bool detecting = true;

  @override
  Widget build(BuildContext context) {
    const Text child = Text('Drag me');
    if (!detecting) {
      return child;
    }
    return GestureDetector(
      onHorizontalDragStart: (DragStartDetails details) {
        print('justin drag');
        Future<void>.delayed(const Duration(seconds: 2)).then((_) {
          print('justin turning off detection');
          setState(() {
            detecting = false;
          });
        });
      },
      onHorizontalDragUpdate: (DragUpdateDetails details) {
        print('justin drag update');
      },
      onHorizontalDragEnd: (DragEndDetails details) {
        print('justin drag end');
      },
      onHorizontalDragCancel: () {
        print('justin drag cancel');
      },
      child: child,
    );
  }
}

I did just change things around so that _isDraggingStart/EndHandle are always updated though, even if this if is tripped.

Copy link
Contributor

@LongCatIsLooong LongCatIsLooong Jun 16, 2023

Choose a reason for hiding this comment

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

You'll get a drag cancel I think?

Copy link
Contributor Author

@justinmc justinmc Jun 16, 2023

Choose a reason for hiding this comment

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

I just tried it again with a cancel handler and it was never called. I've edited my previous comment with the example code to include the cancel handler.

I wonder if that's a bug and we should be calling cancel in the dispose method...

For completeness I also tried reproducing the bug on master with a cancel handler and the cancel handler also was not called, as expected.

Copy link
Contributor Author

@justinmc justinmc Jun 20, 2023

Choose a reason for hiding this comment

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

I'm merging this but I'm happy to revisit anything if you want.

@justinmc
Copy link
Contributor Author

justinmc commented Jun 16, 2023

The Google testing failure seems to show nothing failing, even though it's red here... Maybe I'm reading it wrong. Pushing a merge commit to see if that fixes it.

@Renzo-Olivares
Copy link
Contributor

Renzo-Olivares commented Jun 16, 2023

@justinmc I think there has been problems with Google testing failing this week. I also have similar failure on my PRs.

@justinmc justinmc merged commit 40b4bc9 into flutter:master Jun 20, 2023
@justinmc justinmc deleted the loupe-stuck branch June 20, 2023 23:36
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jun 21, 2023
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 21, 2023
@engine-flutter-autoroll
40b4bc9 Fix: Magnifier appears and won't dismiss (flutter/flutter#128545
4eb3b1e 
)
@ditman ditman mentioned this pull request Jun 21, 2023
Merged
11 tasks
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Jun 22, 2023
@ditman
Roll Flutter from fc8856e to c40baf4 (57 revisions) (#4270)
24ad5a4 
Manual version of #4269

----

Roll Flutter from fc8856e to c40baf4 (57 revisions)

flutter/flutter@fc8856e...c40baf4

2023-06-21 [email protected] Roll Packages from 6e1918f to 0fdf05f (8 revisions) (flutter/flutter#129286)
2023-06-21 [email protected] move test ownership from zanderso -> tools team (flutter/flutter#129199)
2023-06-21 [email protected] Refactor `Analytics` global getter to point to context only (flutter/flutter#129196)
2023-06-21 [email protected] Roll Flutter Engine from cfbd3652532d to f973fb4636d3 (1 revision) (flutter/flutter#129253)
2023-06-21 [email protected] Roll Flutter Engine from 059643dcc8e3 to cfbd3652532d (1 revision) (flutter/flutter#129243)
2023-06-21 [email protected] Roll Flutter Engine from 5313ca367549 to 059643dcc8e3 (1 revision) (flutter/flutter#129240)
2023-06-21 [email protected] Roll Flutter Engine from 946f523859fe to 5313ca367549 (2 revisions) (flutter/flutter#129234)
2023-06-21 [email protected] Roll Flutter Engine from e5a860c5479c to 946f523859fe (2 revisions) (flutter/flutter#129232)
2023-06-21 [email protected] Relax `OverlayPortal` asserts (flutter/flutter#129053)
2023-06-21 [email protected] Roll Flutter Engine from adfc3af300a9 to e5a860c5479c (3 revisions) (flutter/flutter#129228)
2023-06-21 [email protected] Move all the firebase lab device configs to .ci.yaml. (flutter/flutter#129219)
2023-06-21 [email protected] Roll Flutter Engine from 7d4abb81ccd1 to adfc3af300a9 (2 revisions) (flutter/flutter#129225)
2023-06-20 [email protected] update resolution-aware asset docs links (flutter/flutter#128769)
2023-06-20 [email protected] Fix: Magnifier appears and won't dismiss (flutter/flutter#128545)
2023-06-20 [email protected] DecoratedSliver (flutter/flutter#127823)
2023-06-20 [email protected] Roll Flutter Engine from 666244148e89 to 7d4abb81ccd1 (1 revision) (flutter/flutter#129217)
2023-06-20 [email protected] Roll Flutter Engine from 06d0c08460e5 to 666244148e89 (2 revisions) (flutter/flutter#129208)
2023-06-20 [email protected] fixed PreferredSize constuctor invocations (flutter/flutter#128181)
2023-06-20 [email protected] Roll Flutter Engine from 1c16af76ca26 to 06d0c08460e5 (3 revisions) (flutter/flutter#129200)
2023-06-20 [email protected] Roll Flutter Engine from ec64672afd91 to 1c16af76ca26 (1 revision) (flutter/flutter#129197)
2023-06-20 [email protected] Roll Flutter Engine from 4444ede34a9c to ec64672afd91 (3 revisions) (flutter/flutter#129194)
2023-06-20 [email protected] Fix duplicate devices from xcdevice with iOS 17 (flutter/flutter#128802)
2023-06-20 [email protected] iOS info.plist template: make UIViewControllerBasedStatusBar to be true (flutter/flutter#128970)
2023-06-20 [email protected] Fix detection that tests are running on monorepo bots (flutter/flutter#129173)
2023-06-20 [email protected] Use the new `getIsolatePauseEvent` method from VM service to check for pause event. (flutter/flutter#128834)
2023-06-20 [email protected] Adding ScrollController support for Stepper widget (flutter/flutter#128814)
2023-06-20 [email protected] Roll Packages from 59d93d6 to 6e1918f (6 revisions) (flutter/flutter#129176)
2023-06-20 [email protected] Refactor generate_localizations_test.dart (flutter/flutter#128974)
2023-06-20 [email protected] [process] Add a design doc issue template. (flutter/flutter#128361)
2023-06-20 [email protected] Roll Flutter Engine from bd6d3fc90462 to 4444ede34a9c (1 revision) (flutter/flutter#129169)
2023-06-20 [email protected] Roll Flutter Engine from 73c4ba4240cc to bd6d3fc90462 (1 revision) (flutter/flutter#129168)
2023-06-20 [email protected] Roll Flutter Engine from 7ee874792067 to 73c4ba4240cc (1 revision) (flutter/flutter#129162)
2023-06-20 [email protected] Roll Flutter Engine from 6a6c8fb591f5 to 7ee874792067 (1 revision) (flutter/flutter#129160)
2023-06-20 [email protected] Add to API docs to explain what Assist and Suggestion chips are (flutter/flutter#129034)
2023-06-20 [email protected] Roll Flutter Engine from a91bb3f566b9 to 6a6c8fb591f5 (1 revision) (flutter/flutter#129158)
2023-06-20 [email protected] Roll Flutter Engine from e0d456d9251b to a91bb3f566b9 (1 revision) (flutter/flutter#129148)
2023-06-20 [email protected] Roll Flutter Engine from 55418e648958 to e0d456d9251b (1 revision) (flutter/flutter#129146)
2023-06-19 [email protected] Roll Flutter Engine from 84ecaa053ec6 to 55418e648958 (1 revision) (flutter/flutter#129145)
2023-06-19 [email protected] Roll Flutter Engine from 23a2c246600f to 84ecaa053ec6 (2 revisions) (flutter/flutter#129142)
2023-06-19 [email protected] Introduce MaterialState `color` property for chips (flutter/flutter#128584)
2023-06-19 [email protected] Roll Flutter Engine from 280491d4cc21 to 23a2c246600f (8 revisions) (flutter/flutter#129140)
2023-06-19 [email protected] Fix an ordering dependency in the flutter_tools upgrade test (flutter/flutter#129131)
2023-06-19 [email protected] Roll Flutter Engine from 164c6b49dfb5 to 280491d4cc21 (1 revision) (flutter/flutter#129102)
2023-06-19 [email protected] Fix `InputDecoration.applyDefaults` ignoring some properties (flutter/flutter#129010)
2023-06-19 [email protected] Roll Flutter Engine from d298f0bf720c to 164c6b49dfb5 (1 revision) (flutter/flutter#129100)
2023-06-19 [email protected] Roll Flutter Engine from 7ffa1355f718 to d298f0bf720c (24 revisions) (flutter/flutter#129092)
...

----
@HansMuller HansMuller mentioned this pull request Jun 30, 2023
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 16, 2023
@engine-flutter-autoroll
40b4bc9 Fix: Magnifier appears and won't dismiss (flutter/flutter#128545
87a2e6e 
)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023
@engine-flutter-autoroll
40b4bc9 Fix: Magnifier appears and won't dismiss (flutter/flutter#128545
d5f0356 
)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023
@engine-flutter-autoroll
40b4bc9 Fix: Magnifier appears and won't dismiss (flutter/flutter#128545
776aaeb 
)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023
@engine-flutter-autoroll
40b4bc9 Fix: Magnifier appears and won't dismiss (flutter/flutter#128545
9ce34b7 
)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@LongCatIsLooong LongCatIsLooong LongCatIsLooong left review comments

@Renzo-Olivares Renzo-Olivares Renzo-Olivares approved these changes

Assignees

@justinmc justinmc

Labels

a: text input Entering text in a text field or keyboard related problems f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Android, TextFormField: when onTapOutside at left bottom corner magnification appears and doesn't disappear

3 participants

@justinmc @Renzo-Olivares @LongCatIsLooong