Rework interaction gesture handling #2322
Conversation
knopp
force-pushed
the
rework_gesture_detection
branch
from
ee22d46
to
a4cdeac
Compare
knopp
changed the title
knopp
force-pushed
the
rework_gesture_detection
branch
from
3827834
to
81e1f6d
Compare
| @@ -749,7 +749,7 @@ void main() { | |||
|
|
|||
| await tester // | |||
| .createDocument() | |||
| .withSingleParagraph() | |||
There was a problem hiding this comment.
By default Flutter disables overscroll of contents is too short.
|
@angelosilvestre can you give this a first pass? |
| @@ -1287,7 +1200,31 @@ class _AndroidDocumentTouchInteractorState extends State<AndroidDocumentTouchInt | |||
| @override | |||
| Widget build(BuildContext context) { | |||
| final gestureSettings = MediaQuery.maybeOf(context)?.gestureSettings; | |||
| return RawGestureDetector( | |||
| final layerAbove = RawGestureDetector( | |||
There was a problem hiding this comment.
Can you add a comment explaining why we need two layers of gesture detectors so it's clear for future readers?
| @@ -1046,28 +978,23 @@ class _ReadOnlyAndroidDocumentTouchInteractorState extends State<ReadOnlyAndroid | |||
| @override | |||
| Widget build(BuildContext context) { | |||
| final gestureSettings = MediaQuery.maybeOf(context)?.gestureSettings; | |||
| return OverlayPortal( | |||
| final layerAbove = OverlayPortal( | |||
There was a problem hiding this comment.
Same here about adding a comment explaining why we need the layers.
| } | ||
| } | ||
|
|
||
| final gestureSettings = MediaQuery.maybeOf(context)?.gestureSettings; | ||
| return RawGestureDetector( | ||
| behavior: HitTestBehavior.opaque, | ||
| final layerAbove = RawGestureDetector( |
There was a problem hiding this comment.
Same here about adding a comment explaining why we need the layers.
super_editor/test_goldens/editor/mobile/mobile_selection_test.dart
Outdated
Show resolved
Hide resolved
| required this.child, | ||
| required this.fillViewport, |
There was a problem hiding this comment.
Please place child as the last parameter.
There was a problem hiding this comment.
Please adjust this, as Angelo mentioned. Additionally, please move fillViewport above showDebugPaint, which is typically the last property before child.
| // The user wasn't dragging over a selection. Do nothing. | ||
| _dragMode = null; | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
If we end up removing DragMode.scroll and DragMode.waitingForScrollDirection, can we simplify this switch to the following code?
if (_dragMode != null) {
// The user was dragging a selection change in some way, either with handles
// or with a long-press. Finish that interaction.
_onDragSelectionEnd();
} | } | ||
| } | ||
|
|
||
| final gestureSettings = MediaQuery.maybeOf(context)?.gestureSettings; | ||
| return RawGestureDetector( | ||
| behavior: HitTestBehavior.opaque, | ||
| final layerAbove = RawGestureDetector( |
There was a problem hiding this comment.
Same here about adding a comment explaining the need of two layers of gesture detectors.
| @@ -1511,6 +1378,7 @@ class SuperEditorIosToolbarOverlayManagerState extends State<SuperEditorIosToolb | |||
|
|
|||
| @override | |||
| Widget build(BuildContext context) { | |||
| final ancestorScrollable = context.findAncestorScrollableWithVerticalScroll; | |||
There was a problem hiding this comment.
It doesn't seem we are using this variable.
knopp
force-pushed
the
rework_gesture_detection
branch
from
81e1f6d
to
f962f4a
Compare
|
@angelosilvestre, can you give this another go? |
There was a problem hiding this comment.
It looks good to me. @matthew-carroll should take a look now.
knopp
force-pushed
the
rework_gesture_detection
branch
from
f962f4a
to
c7cf43b
Compare
There was a problem hiding this comment.
I left a number of code organization and documentation comments. Since we have a lot of duplication in the gesture area of the codebase, most of the comments apply to a number of similar locations in this PR. For each comment, please check all the changes in this PR for other places where the same comment applies.
| required this.child, | ||
| required this.fillViewport, |
There was a problem hiding this comment.
Please adjust this, as Angelo mentioned. Additionally, please move fillViewport above showDebugPaint, which is typically the last property before child.
| @@ -69,12 +71,14 @@ class DocumentMouseInteractor extends StatefulWidget { | |||
| /// Auto-scrolling delegate. | |||
| final AutoScrollController autoScroller; | |||
|
|
|||
| final bool fillViewport; | |||
There was a problem hiding this comment.
Please always include a Dart Doc for non-standard properties. What does this do? When should someone use it? When should they not use it? Dart Docs are our instructions for all downstream users of the package.
Please double check the rest of this PR for any other missing Dart Docs, too.
| child: _buildCursorStyle( | ||
| child: _buildGestureInput( | ||
| child: widget.child ?? const SizedBox(), | ||
| return SliverHybridStack( |
There was a problem hiding this comment.
Why do we need a SliverHybridStack here?
| @@ -619,6 +598,10 @@ class _AndroidDocumentTouchInteractorState extends State<AndroidDocumentTouchInt | |||
| return viewportBox.globalToLocal(globalOffset); | |||
| } | |||
|
|
|||
| final _interactor = GlobalKey(); | |||
There was a problem hiding this comment.
Please move this property up to the top with all the other properties, and please add a Dart Doc to get interactorBox.
| return SliverHybridStack( | ||
| fillViewport: widget.fillViewport, | ||
| children: [ | ||
| layerBelow, |
There was a problem hiding this comment.
Please try not to assign intermediate widgets. Either place all of them here in this tree, or breakout private build methods to keep the tree size tractable, e.g., _buildBottomGestureLayer() and _buildTopGestureLayer().
| _buildMagnifierFocalPoint(), | ||
| ], | ||
| ), | ||
| ); | ||
| final layerBelow = RawGestureDetector( |
There was a problem hiding this comment.
Same note here about creating intermediate widgets in a build method.
| child, | ||
| ], | ||
| ); | ||
| return widget.gestureBuilder(context, child: child); |
There was a problem hiding this comment.
I'm not sure where this method is called, but now that all the content of this method has been replaced by a single line, is there any reason not to call that builder directly from wherever this method is being called?
| @@ -17,6 +17,8 @@ class EagerPanGestureRecognizer extends DragGestureRecognizer { | |||
| super.allowedButtonsFilter, | |||
| }); | |||
|
|
|||
| bool Function()? canAccept; | |||
There was a problem hiding this comment.
Please add a Dart Doc. What does this do? When should it be used? How should it be used?
| ), | ||
| ); | ||
| final layerBelow = RawGestureDetector( |
There was a problem hiding this comment.
Same note about creating intermediate widgets.
| @@ -378,6 +379,9 @@ void main() { | |||
| const DocumentPosition(nodeId: "1", nodePosition: TextNodePosition(offset: 34)), | |||
| ); | |||
| await tester.pumpAndSettle(); | |||
| // Wait a bit to ensure that the interactor recognizer doesn't consider this a double | |||
| // tap. | |||
| await tester.pump(kDoubleTapTimeout + const Duration(seconds: 1)); | |||
There was a problem hiding this comment.
Was this truly meant to be Duration(seconds: 1) or was it supposed to be Duration(milliseconds: 1)?
This PR reworks gesture handling inside interactors with the goal of super_editor gesture detectors playing nicely with other detectors. With this PR it is no longer necessary for interactor to drive scrolling and it is now possible to have working dismissable rows in the editor.
The gesture recognizer configuration on mobile has been changed so that the document contents is sandwiched between pan and tap recognizers, i.e.
EagerPanGestureRecognizer(above document)TapSequenceGestureRecognizer(below document)EagerPanGestureRecognizeralways wins, but it gets only accepted conditionally (if user drags handles or long press is in progress).TapSequenceGestureRecognzeris below document so that it doesn't interfere with checkboxes or buttons inside document.