[CupertinoAlertDialog] Add tap-slide gesture #154853
Conversation
github-actions
bot
added
framework
|
Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change). If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
| @override | ||
| bool didEnter({required bool fromPointerDown, required bool innerEnabled}) { | ||
| widget.onPressStateChange?.call(innerEnabled); | ||
| if (innerEnabled) { |
There was a problem hiding this comment.
nit: maybe && over nested ifs?
| State<CupertinoDialogAction> createState() => _CupertinoDialogActionState(); | ||
| } | ||
|
|
||
| class _CupertinoDialogActionState extends State<CupertinoDialogAction> |
There was a problem hiding this comment.
Is this going to be a leaf _SlideTarget? Since this is a public widget that takes a Key, I think it's possible for the if (_currentTargets.firstOrNull != foundTargets.firstOrNull) check in _updateDrag to be false if a CupertinoDialogAction gets reparented while a "sliding" gesture is in progress?
There was a problem hiding this comment.
Yeah it's technically possible. In fact, if the action buttons somehow moves (for example, if a button at the top expands by itself so that the highlighted button is moved out of the pointer position), the dialog will not re-perform hit tests without a pointer event and will not update.
However, in reality I can't imagine when this would happen, considering the dialog and action sheets tend to be pretty isolated.
Completely solving this requires the recognizer to use the same logic as MouseTracker, which additionally performs a hit test per frame - in fact, we should re-use the code from MouseTracker (or, better put, to conditionally enable hovering for touch events). However that might need some refactor for MouseTracker for a really rare edge case, and therefore I suggest we leave to a future PR, such as as part of #19786. What do you think?
There was a problem hiding this comment.
Maybe add a "TODO"?
which additionally performs a hit test per frame
I thought we are already doing that in the current implementation (in _updateDrag)? We are just not comparing the hit-testing paths in a per-entry fashion.
There was a problem hiding this comment.
Maybe add a "TODO"?
Sure!
I thought we are already doing that in the current implementation
_updateDrag is only triggered every time the pointer moves. But if the app is updated while the pointer stays still, then the pointer won't hit test the new layout. Really tricky situation.
|
Golden file changes are available for triage from new commit, Click here to view. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
dkwingsmt
added
the
autosubmit
|
Golden file changes are available for triage from new commit, Click here to view. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
auto-submit
bot
removed
the
autosubmit
|
auto label is removed for flutter/flutter/154853, due to - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label. |
dkwingsmt
added
the
autosubmit
Manual roll requested by [email protected] flutter/flutter@c4c9f47...7fe8237 2024-09-18 [email protected] Roll Flutter Engine from 4bdcbf39c200 to 15e9e3153266 (1 revision) (flutter/flutter#155371) 2024-09-18 [email protected] Roll Flutter Engine from fdf38910bad4 to 4bdcbf39c200 (5 revisions) (flutter/flutter#155368) 2024-09-18 [email protected] Roll Packages from 4f2b9cd to c7406b3 (1 revision) (flutter/flutter#155362) 2024-09-18 [email protected] Roll Flutter Engine from 4f2d866aef32 to fdf38910bad4 (1 revision) (flutter/flutter#155339) 2024-09-18 [email protected] Roll Flutter Engine from 311ba971bb3a to 4f2d866aef32 (4 revisions) (flutter/flutter#155329) 2024-09-18 [email protected] Fix missing icon props in button `styleFrom` methods (flutter/flutter#154821) 2024-09-17 [email protected] Roll Flutter Engine from 0ef18a3ef064 to 311ba971bb3a (8 revisions) (flutter/flutter#155325) 2024-09-17 [email protected] [CupertinoAlertDialog] Add tap-slide gesture (flutter/flutter#154853) 2024-09-17 [email protected] Uninstall /can fail/ (flutter/flutter#155314) 2024-09-17 [email protected] Roll Packages from df88c81 to 4f2b9cd (3 revisions) (flutter/flutter#155312) 2024-09-17 [email protected] Roll Flutter Engine from a1700b9ea2db to 0ef18a3ef064 (1 revision) (flutter/flutter#155311) 2024-09-17 [email protected] Roll Flutter Engine from 1376288f5c2a to a1700b9ea2db (2 revisions) (flutter/flutter#155290) 2024-09-17 [email protected] Delete packages/flutter_tools/lib/src/fuchsia directory (flutter/flutter#154880) 2024-09-17 [email protected] Adds ColorSwatch matcher (flutter/flutter#155272) 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
2 participants
This PR adds "sliding tap" to
CupertinoAlertDialogand fixes #19786.Much of the needed infrastructure has been implemented in #150219, but this time with a new challenge to support disabled buttons, i.e. the button should not show tap highlight when pressed (#107371).
CupertinoDialogAction, while the background is rendered by a private class that wraps the action widget and built by the dialog body. We need a way to pass the boolean "enabled" from the child to the parent when the action is pressed. After much experimentation, I think the best way is to propagate this boolean using the custom gesture callback.CupertinoDialogActionto place aValueGetter<bool> getEnabledto the parent as soon as it's mounted. However, this is pretty ugly...This PR also fixes #107371, i.e. disabled
CupertinoDialogActionno longer triggers the pressing highlight. However, while legacy buttons (custom button classes that are implemented byGestureDetector.onTap) still functions (theironPressedcontinues to work), disabled legacy buttons will still show pressing highlight, and there's no plan (actually, no way) to fix it.All tests related to sliding taps in
CupertinoActionSheethas been copied toCupertinoAlertDialog, with additional tests for disabled buttons.Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.