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.

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

[SuperEditor][SuperReader][Android] Fix long-press firing after scrolling and releasing the pointer (Resolves #1535) #1540

Merged
merged 3 commits into from
Oct 24, 2023
Merged

[SuperEditor][SuperReader][Android] Fix long-press firing after scrolling and releasing the pointer (Resolves #1535) #1540

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c7bda47
[SuperEditor][SuperReader][Android] Fix long-press firing after scrol…
angelosilvestre Oct 21, 2023
f0535dd
Minor test adjustment
angelosilvestre Oct 21, 2023
490f945
PR updates
angelosilvestre Oct 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 5 additions & 7 deletions super_editor/lib/src/default_editor/document_gestures_touch_android.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,6 +324,11 @@ class _AndroidDocumentTouchInteractorState extends State<AndroidDocumentTouchInt

if (isScrolling) {
_isScrolling = true;

// The user started to scroll.
// Cancel the timer to stop trying to detect a long press.
_tapDownLongPressTimer?.cancel();
_tapDownLongPressTimer = null;
} else {
onNextFrame((_) {
// Set our scrolling flag to false on the next frame, so that our tap handlers
Expand Down Expand Up @@ -497,13 +502,6 @@ class _AndroidDocumentTouchInteractorState extends State<AndroidDocumentTouchInt

// Runs when a tap down has lasted long enough to signify a long-press.
void _onLongPressDown() {
if (_isScrolling) {
// When the editor has an ancestor scrollable, dragging won't trigger a pan gesture
// is this widget. Because of that, the timer still fires after the timeout.
// Do nothing to let the user scroll.
return;
}

_longPressStrategy = AndroidDocumentLongPressSelectionStrategy(
document: widget.document,
documentLayout: _docLayout,
Expand Down
6 changes: 6 additions & 0 deletions super_editor/lib/src/super_reader/read_only_document_android_touch_interactor.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -304,6 +304,12 @@ class _ReadOnlyAndroidDocumentTouchInteractorState extends State<ReadOnlyAndroid

if (isScrolling) {
_isScrolling = true;

// The long-press timer is cancelled if a pan gesture is detected.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm gonna merge this because there's a critical long-press ticket from Superlist, but this comment is still ordered in a way that makes it unnecessarily difficult to understand what it's trying to say.

Generally, the first thing you should say in a comment is what the code is actually doing. Beginning with contextual qualifiers is confusing because you haven't event established what you're qualifying yet.

Here's a comment in reversed order:

// Cancel the long-press timer because we're scrolling, and scrolling is mutually
// independent from long-pressing.
//
// We also cancel the timer when a pan gesture starts. However, we won't receive
// a pan gesture event if we have an ancestor scrollable. Therefore, we cancel the
// timer here, too.

// However, if we have an ancestor scrollable, we won't receive a pan gesture in this widget.
// Cancel the timer as soon as the user started scrolling.
_tapDownLongPressTimer?.cancel();
_tapDownLongPressTimer = null;
} else {
onNextFrame((_) {
// Set our scrolling flag to false on the next frame, so that our tap handlers
Expand Down
61 changes: 60 additions & 1 deletion super_editor/test/super_editor/supereditor_scrolling_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -581,7 +581,7 @@ void main() {
expect(caretOffset.dy, greaterThanOrEqualTo(screenSizeWithKeyboard.height - trailingBoundary));
});

testWidgetsOnMobile('scrolling doesn\'t cause the keyboard to open', (tester) async {
testWidgetsOnMobile('scrolling and holding the pointer doesn\'t cause the keyboard to open', (tester) async {
final scrollController = ScrollController();

// Pump an editor inside a CustomScrollView without enough room to display
Expand Down Expand Up @@ -626,8 +626,67 @@ void main() {
// The editor supports long press to select.
// Wait long enough to make sure this gesture wasn't confused with a long press.
await tester.pump(kLongPressTimeout + const Duration(milliseconds: 1));

// Ensure we scrolled, didn't changed the selection and didn't attach to the IME.
expect(scrollController.offset, greaterThan(0));
expect(SuperEditorInspector.findDocumentSelection(), isNull);
expect(tester.testTextInput.hasAnyClients, isFalse);

// Release the pointer.
await dragGesture.up();
await dragGesture.removePointer();
});

testWidgetsOnMobile('scrolling and releasing the pointer doesn\'t cause the keyboard to open', (tester) async {
final scrollController = ScrollController();

// Pump an editor inside a CustomScrollView without enough room to display
// the whole content.
await tester
.createDocument() //
.withLongTextContent()
.withCustomWidgetTreeBuilder(
(superEditor) => MaterialApp(
home: Scaffold(
body: ConstrainedBox(
constraints: const BoxConstraints(maxHeight: 200),
child: CustomScrollView(
controller: scrollController,
slivers: [
SliverToBoxAdapter(
child: superEditor,
),
],
),
),
),
),
)
.pump();

// Ensure the scrollview didn't start scrolled.
expect(scrollController.offset, 0);

final scrollableRect = tester.getRect(find.byType(CustomScrollView));

const dragFrameCount = 10;
final dragAmountPerFrame = scrollableRect.height / dragFrameCount;

// Drag from the bottom all the way up to the top of the scrollable.
final dragGesture = await tester.startGesture(scrollableRect.bottomCenter - const Offset(0, 1));
for (int i = 0; i < dragFrameCount; i += 1) {
await dragGesture.moveBy(Offset(0, -dragAmountPerFrame));
await tester.pump();
}

// Stop the scrolling gesture.
await dragGesture.up();
await dragGesture.removePointer();
await tester.pump();

// The editor supports long press to select.
// Wait long enough to make sure this gesture wasn't confused with a long press.
await tester.pump(kLongPressTimeout + const Duration(milliseconds: 1));

// Ensure we scrolled, didn't changed the selection and didn't attach to the IME.
expect(scrollController.offset, greaterThan(0));
Expand Down
60 changes: 59 additions & 1 deletion super_editor/test/super_reader/super_reader_scrolling_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,7 +314,7 @@ void main() {
});

group("with ancestor scrollable", () {
testWidgetsOnMobile('scrolling doesn\'t change selection', (tester) async {
testWidgetsOnMobile('scrolling and holding the pointer doesn\'t change selection', (tester) async {
final scrollController = ScrollController();

// Pump a reader inside a CustomScrollView without enough room to display
Expand Down Expand Up @@ -359,8 +359,66 @@ void main() {
// The reader supports long press to select.
// Wait long enough to make sure this gesture wasn't confused with a long press.
await tester.pump(kLongPressTimeout + const Duration(milliseconds: 1));

// Ensure we scrolled and didn't change the selection.
expect(scrollController.offset, greaterThan(0));
expect(SuperReaderInspector.findDocumentSelection(), isNull);

await dragGesture.up();
await dragGesture.removePointer();
});

testWidgetsOnMobile('scrolling and releasing the pointer doesn\'t change selection after gesture ended',
(tester) async {
final scrollController = ScrollController();

// Pump a reader inside a CustomScrollView without enough room to display
// the whole content.
await tester
.createDocument() //
.withLongTextContent()
.withCustomWidgetTreeBuilder(
(superReader) => MaterialApp(
home: Scaffold(
body: ConstrainedBox(
constraints: const BoxConstraints(maxHeight: 200),
child: CustomScrollView(
controller: scrollController,
slivers: [
SliverToBoxAdapter(
child: superReader,
),
],
),
),
),
),
)
.pump();

// Ensure the scrollview didn't start scrolled.
expect(scrollController.offset, 0);

final scrollableRect = tester.getRect(find.byType(CustomScrollView));

const dragFrameCount = 10;
final dragAmountPerFrame = scrollableRect.height / dragFrameCount;

// Drag from the bottom all the way up to the top of the scrollable.
final dragGesture = await tester.startGesture(scrollableRect.bottomCenter - const Offset(0, 1));
for (int i = 0; i < dragFrameCount; i += 1) {
await dragGesture.moveBy(Offset(0, -dragAmountPerFrame));
await tester.pump();
}

// Stop the scrolling gesture.
await dragGesture.up();
await dragGesture.removePointer();
await tester.pump();

// The reader supports long press to select.
// Wait long enough to make sure this gesture wasn't confused with a long press.
await tester.pump(kLongPressTimeout + const Duration(milliseconds: 1));

// Ensure we scrolled and didn't change the selection.
expect(scrollController.offset, greaterThan(0));
Expand Down