Skip to content
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

Exclude Tooltip's overlay child from SelectableRegion #130181

Merged
merged 3 commits into from Jul 13, 2023

Conversation

LongCatIsLooong
Copy link
Contributor

@LongCatIsLooong LongCatIsLooong commented Jul 7, 2023

Fixes #129969 by making tooltip text unselectable (for now).
Also fixes some other issues uncovered when I was writing the tests.

Currently getTransformTo only works on ancestors. I'll try to add a new method that computes the transform from 2 arbitrary render objects in the same render tree in a follow-up PR and make Selectable use that method instead.

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.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Jul 7, 2023
}

@override
void dispose() {
GestureBinding.instance.pointerRouter.removeGlobalRoute(_handleGlobalPointerEvent);
Tooltip._openedTooltips.remove(this);
// Remove the onCancel callbacks to prevent the registered callbacks from
// triggering unnecessary sideeffects (such as animation).
_longPressRecognizer?.onLongPressCancel = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is surprising, why would it still fire if _longPressRecognizer is disposed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

onCancel is called when an OneSequenceGestureRecognizer disposes:

stack trace:

#9      TooltipState._handleTapToDismiss (package:flutter/src/material/tooltip.dart:564:5)
#10     GestureRecognizer.invokeCallback (package:flutter/src/gestures/recognizer.dart:275:24)
#11     LongPressGestureRecognizer._checkLongPressCancel (package:flutter/src/gestures/long_press.dart:687:13)
#12     LongPressGestureRecognizer.resolve (package:flutter/src/gestures/long_press.dart:822:9)
#13     OneSequenceGestureRecognizer.dispose (package:flutter/src/gestures/recognizer.dart:393:5)
#14     PrimaryPointerGestureRecognizer.dispose (package:flutter/src/gestures/recognizer.dart:689:11)

Copy link
Contributor

Choose a reason for hiding this comment

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

This seems weird to me, the on cancel usually used for cleaning up the state in the owner when gesture is in progress. The owner won't expect the on cancel to be called if it is actively disposing the recognizer. it would be the owner's responsibility to clean up and state left behind when disposing the resource. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From the gesture recognizer's perspective I guess that's expected? When there's an onGestureStart someone would expect either an onGestureUp or onGestureCancel. The owner of the gesture recognizer may not be the owner of the gesture callbacks ?

e.g. SomeWidgetWhoseElementOwnsALongPressGestureRecognizer(this.onLongPress, this.onLongPressCancel), when this widget unmounts the parent widget may appreciate that they're getting an onLongPressCancel to pair up with the onLongPress they received previously?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the correct way would probably be SomeWidgetWhoseElementOwnsALongPressGestureRecognizer would have to call the cancel or up callback on dispose instead of directly hook up to the recognizer and expect it to call the cancel on dispose, but I can see this design can be hard to maintain.

Can the TooltipState be smart to ignore on cancel when the state itself is disposed? if not, i am ok with the code as-is. We should document the reason here more specific why we need to remove the cancel. ie.
Disposing the recognizer can fire oncancell if gesture is in progress.

Copy link
Contributor

@chunhtai chunhtai left a comment

Choose a reason for hiding this comment

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

LGTM.

}

@override
void dispose() {
GestureBinding.instance.pointerRouter.removeGlobalRoute(_handleGlobalPointerEvent);
Tooltip._openedTooltips.remove(this);
// Remove the onCancel callbacks to prevent the registered callbacks from
// triggering unnecessary sideeffects (such as animation).
_longPressRecognizer?.onLongPressCancel = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the correct way would probably be SomeWidgetWhoseElementOwnsALongPressGestureRecognizer would have to call the cancel or up callback on dispose instead of directly hook up to the recognizer and expect it to call the cancel on dispose, but I can see this design can be hard to maintain.

Can the TooltipState be smart to ignore on cancel when the state itself is disposed? if not, i am ok with the code as-is. We should document the reason here more specific why we need to remove the cancel. ie.
Disposing the recognizer can fire oncancell if gesture is in progress.

@LongCatIsLooong LongCatIsLooong added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 12, 2023
@auto-submit auto-submit bot merged commit 2da353a into flutter:master Jul 13, 2023
73 checks passed
@LongCatIsLooong LongCatIsLooong deleted the unselectable-tooltip branch July 13, 2023 00:49
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 13, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 13, 2023
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Jul 13, 2023
flutter/flutter@544d30d...c40173f

2023-07-13 jonahwilliams@google.com Revert "Roll Flutter Engine from 16e2ab7e986c to 1b1ccdd1f527 (13 revisions)" (flutter/flutter#130479)
2023-07-13 engine-flutter-autoroll@skia.org Roll Flutter Engine from 16e2ab7e986c to 1b1ccdd1f527 (13 revisions) (flutter/flutter#130458)
2023-07-13 31859944+LongCatIsLooong@users.noreply.github.com Exclude `Tooltip`'s overlay child from SelectableRegion (flutter/flutter#130181)
2023-07-12 36861262+QuncCccccc@users.noreply.github.com Update `Checkbox` tests for M2/M3 (flutter/flutter#130351)
2023-07-12 58529443+srujzs@users.noreply.github.com Refactor JSNumber.toDart and Object.toJS (flutter/flutter#129436)
2023-07-12 82336674+gilnobrega@users.noreply.github.com  Reland [a11y] CupertinoSwitch On/Off labels (flutter/flutter#130173)
2023-07-12 gspencergoog@users.noreply.github.com Add missing links to examples that aren't linked anywhere (flutter/flutter#130422)
2023-07-12 thkim1011@users.noreply.github.com Use platform specific line separator in gen-l10n (flutter/flutter#130090)
2023-07-12 tessertaha@gmail.com Update `Divider`/`VerticalDivider` and theme tests for M2/M3 (flutter/flutter#130415)
2023-07-12 engine-flutter-autoroll@skia.org Roll Flutter Engine from 5c887028810d to 16e2ab7e986c (2 revisions) (flutter/flutter#130421)

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 rmistry@google.com,stuartmorgan@google.com,ychris@google.com 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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
LouiseHsu pushed a commit to LouiseHsu/flutter that referenced this pull request Jul 13, 2023
Fixes flutter#129969 by making tooltip text unselectable (for now). 
Also fixes some other issues uncovered when I was writing the tests.

Currently `getTransformTo` only works on ancestors. I'll try to add a new method that computes the transform from 2 arbitrary render objects in the same render tree in a follow-up PR and make `Selectable` use that method instead.
@LongCatIsLooong LongCatIsLooong restored the unselectable-tooltip branch July 25, 2023 19:50
@LongCatIsLooong LongCatIsLooong deleted the unselectable-tooltip branch July 25, 2023 19:53
LongCatIsLooong added a commit to LongCatIsLooong/flutter that referenced this pull request Jul 25, 2023
Fixes flutter#129969 by making tooltip text unselectable (for now). 
Also fixes some other issues uncovered when I was writing the tests.

Currently `getTransformTo` only works on ancestors. I'll try to add a new method that computes the transform from 2 arbitrary render objects in the same render tree in a follow-up PR and make `Selectable` use that method instead.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Application crash when dragging mouse over Tooltip while selecting text in SelectionArea
2 participants