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] - Lock scrolling when content fits within available space (Resolves #1409) #1421

Merged
merged 13 commits into from
Sep 30, 2023
Merged

[SuperEditor][SuperReader] - Lock scrolling when content fits within available space (Resolves #1409) #1421

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
e492bb8
lock scrolling when content fits within available space
rutvik110 Sep 9, 2023
82190bc
test tools update
rutvik110 Sep 9, 2023
9d53c2e
refactor
rutvik110 Sep 9, 2023
76a3586
reverted current scroll lock mechansim as the approach was wrong wrt …
rutvik110 Sep 12, 2023
d12e5f4
returning early instead of going ballistic when there's not content t…
rutvik110 Sep 12, 2023
f4aae47
update
rutvik110 Sep 12, 2023
dd8725f
updated jumpBy to check for max extent == 0, refactor
rutvik110 Sep 16, 2023
3245327
tests added
rutvik110 Sep 23, 2023
cf930f8
reverted example changes
rutvik110 Sep 23, 2023
dc5ff49
tests for scroll wheel added
rutvik110 Sep 27, 2023
37231fa
updates
rutvik110 Sep 27, 2023
981d39a
test description updates
rutvik110 Sep 29, 2023
da0c7b1
test updates
rutvik110 Sep 30, 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
9 changes: 8 additions & 1 deletion super_editor/lib/src/default_editor/document_scrollable.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -373,6 +373,11 @@ class AutoScrollController with ChangeNotifier {
}

final scrollPosition = _getScrollPosition!();

if (scrollPosition.maxScrollExtent == 0) {
return;
}

scrollPosition.jumpTo(
(scrollPosition.pixels + delta).clamp(0.0, scrollPosition.maxScrollExtent),
);
Expand All @@ -388,7 +393,9 @@ class AutoScrollController with ChangeNotifier {
}

if (pos is ScrollPositionWithSingleContext) {
pos.goBallistic(pixelsPerSecond);
if (pos.maxScrollExtent > 0) {
matthew-carroll marked this conversation as resolved.
Show resolved Hide resolved
pos.goBallistic(pixelsPerSecond);
}
pos.context.setIgnorePointer(false);
}
}
Expand Down
109 changes: 106 additions & 3 deletions 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 @@ -9,6 +9,7 @@ import 'package:super_editor/super_editor.dart';
import 'package:super_editor/super_editor_test.dart';

import 'supereditor_test_tools.dart';
import 'test_documents.dart';

void main() {
group("SuperEditor scrolling", () {
Expand Down Expand Up @@ -682,11 +683,103 @@ void main() {

// Ensure SuperEditor has scrolled
expect(editorScrollController.offset, greaterThan(0));

// Ensure that scrolling didn't scroll the ListView
expect(listScrollController.position.pixels, equals(0));
});
});

group("doesn't scroll editor when content fits within the available space", () {
testWidgetsOnDesktop(
rutvik110 marked this conversation as resolved.
Show resolved Hide resolved
"attempts to scroll editor vertically in both directions using trackpad",
Copy link
Contributor

Choose a reason for hiding this comment

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

These test names aren't great. Look at what the final test name is:

"doesn't scroll editor when content fits within the available space attempts to scroll editor vertically in both directions using trackpad"

I think you can describe the group and test in a way that's much more concise, and makes sense when read as the final test description.

(tester) async {
const windowSize = Size(800, 600);
rutvik110 marked this conversation as resolved.
Show resolved Hide resolved
tester.view.physicalSize = windowSize;

final isScrollingUp = _scrollDirectionVariant.currentValue == _ScrollDirection.up;

await tester //
.createDocument()
.withCustomContent(
paragraphThenHrThenParagraphDoc()
..insertNodeAt(
0,
ParagraphNode(
id: Editor.createNodeId(),
text: AttributedText('Document #1'),
metadata: {
'blockType': header1Attribution,
},
),
),
)
.pump();

final scrollState = tester.state<ScrollableState>(find.byType(Scrollable));

// Perform a fling on the editor to attemp scrolling.
await tester.trackpadFling(
find.byType(SuperEditor),
Offset(0.0, isScrollingUp ? 100 : -100),
300,
);

await tester.pump();

// Ensure SuperEditor is not scrolling.
expect(scrollState.position.activity?.isScrolling, false);
},
variant: _scrollDirectionVariant,
);

testWidgetsOnDesktop(
"attempts to scroll editor vertically in both directions using scroll wheel",
(tester) async {
const windowSize = Size(800, 600);
tester.view.physicalSize = windowSize;

final isScrollUp = _scrollDirectionVariant.currentValue == _ScrollDirection.up;

await tester //
.createDocument()
.withCustomContent(
paragraphThenHrThenParagraphDoc()
..insertNodeAt(
0,
ParagraphNode(
id: Editor.createNodeId(),
text: AttributedText('Document #1'),
metadata: {
'blockType': header1Attribution,
},
),
),
)
.pump();

final scrollState = tester.state<ScrollableState>(find.byType(Scrollable));

final Offset scrollEventLocation = tester.getCenter(find.byType(SuperEditor));
final TestPointer testPointer = TestPointer(1, PointerDeviceKind.mouse);

// Send initial pointer event to set the location for subsequent pointer scroll events.
await tester.sendEventToBinding(testPointer.hover(scrollEventLocation));

// Send pointer scroll event to start scrolling.
await tester.sendEventToBinding(
testPointer.scroll(
Offset(
0.0,
isScrollUp ? 100 : -100.0,
),
),
);

await tester.pump();

// Ensure SuperReader is not scrolling.
expect(scrollState.position.activity!.isScrolling, false);
},
variant: _scrollDirectionVariant,
);
});
});
});
}
Expand Down Expand Up @@ -850,3 +943,13 @@ MutableDocument _createExampleDocumentForScrolling() {
],
);
}

final _scrollDirectionVariant = ValueVariant<_ScrollDirection>({
_ScrollDirection.up,
_ScrollDirection.down,
});

enum _ScrollDirection {
up,
down;
}
106 changes: 106 additions & 0 deletions 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 @@ -6,6 +6,7 @@ import 'package:super_editor/super_editor.dart';
import 'package:super_editor/super_reader_test.dart';

import 'reader_test_tools.dart';
import 'test_documents.dart';

void main() {
group("SuperReader scrolling", () {
Expand Down Expand Up @@ -218,5 +219,110 @@ void main() {
isTrue,
);
});

group("doesn't scroll reader when content fits within the available space", () {
testWidgetsOnDesktop(
"attempts to scroll reader vertically in both directions using trackpad",
(tester) async {
const windowSize = Size(800, 600);
tester.view.physicalSize = windowSize;

final isScrollUp = _scrollDirectionVariant.currentValue == _ScrollDirection.up;

await tester //
.createDocument()
.withCustomContent(
paragraphThenHrThenParagraphDoc()
..insertNodeAt(
0,
ParagraphNode(
id: Editor.createNodeId(),
text: AttributedText('Document #1'),
metadata: {
'blockType': header1Attribution,
},
),
),
)
.pump();

final scrollState = tester.state<ScrollableState>(find.byType(Scrollable));

// Perform a fling on the reader to attemp scrolling.
await tester.trackpadFling(
find.byType(SuperReader),
Offset(0.0, isScrollUp ? 100 : -100),
300,
);

await tester.pump();

// Ensure SuperReader is not scrolling.
expect(scrollState.position.activity?.isScrolling, false);
},
variant: _scrollDirectionVariant,
);

testWidgetsOnDesktop(
"attempts to scroll reader vertically in both directions using scroll wheel",
(tester) async {
const windowSize = Size(800, 600);
tester.view.physicalSize = windowSize;

final isScrollUp = _scrollDirectionVariant.currentValue == _ScrollDirection.up;

await tester //
.createDocument()
.withCustomContent(
paragraphThenHrThenParagraphDoc()
..insertNodeAt(
0,
ParagraphNode(
id: Editor.createNodeId(),
text: AttributedText('Document #1'),
metadata: {
'blockType': header1Attribution,
},
),
),
)
.pump();

final scrollState = tester.state<ScrollableState>(find.byType(Scrollable));

final Offset scrollEventLocation = tester.getCenter(find.byType(SuperReader));
final TestPointer testPointer = TestPointer(1, PointerDeviceKind.mouse);

// Send initial pointer event to set the location for subsequent pointer scroll events.
await tester.sendEventToBinding(testPointer.hover(scrollEventLocation));

// Send pointer scroll event to start scrolling.
await tester.sendEventToBinding(
testPointer.scroll(
Offset(
0.0,
isScrollUp ? 100 : -100.0,
),
),
);

await tester.pump();

// Ensure SuperReader is not scrolling.
expect(scrollState.position.activity!.isScrolling, false);
},
variant: _scrollDirectionVariant,
);
});
});
}

final _scrollDirectionVariant = ValueVariant<_ScrollDirection>({
_ScrollDirection.up,
_ScrollDirection.down,
});

enum _ScrollDirection {
up,
down;
}