[SuperEditor][iOS] Add tests for drag to select (Resolves #1613) #1627
Conversation
| @@ -159,6 +159,7 @@ extension SuperEditorRobot on WidgetTester { | |||
| Future<void> dragSelectDocumentFromPositionByOffset({ | |||
| required DocumentPosition from, | |||
| required Offset delta, | |||
| PointerDeviceKind pointerDeviceKind = PointerDeviceKind.mouse, | |||
There was a problem hiding this comment.
If this was added because all mobile tests need to drag with a finger instead of a mouse, then that means every mobile test will have to specify this. That seems fragile. Would it make more sense to make this nullable and then if it's null, the implementation chooses the most sensible device kind for the default platform?
| @@ -46,3 +47,74 @@ extension DragExtensions on WidgetTester { | |||
| return dragGesture; | |||
| } | |||
| } | |||
|
|
|||
| /// Compares two selections, ignoring selection affinities. | |||
There was a problem hiding this comment.
Most of what's here looks like it should exist in the package, not in test code. I assume that inspecting selection bounds is something that anyone might want to do.
Also, in this case, I don't think "selection equivalency" is appropriate, the way it was with node positions. Upstream vs downstream could truly result in different rendering, whereas the affinity for a single position shouldn't impact any visual behavior. I would approach this condition as one of equivalent bounds, or perhaps even equal bounds. But the point is that it's about the bounds, not the overall selection.
There was a problem hiding this comment.
Most of what's here looks like it should exist in the package, not in test code. I assume that inspecting selection bounds is something that anyone might want to do.
Are you suggesting that we add a method to the DocumentSelection class? I'm not sure we'll be using Matchers in the package code.
Also, in this case, I don't think "selection equivalency" is appropriate, the way it was with node positions. Upstream vs downstream could truly result in different rendering, whereas the affinity for a single position shouldn't impact any visual behavior.
My description wasn't accurate. I'm not ignoring the selection affinity itself, I'm ignoring the affinity of each NodePosition included in the selection.
There was a problem hiding this comment.
Yeah, we wouldn't add a Matcher to the source code. I was looking mostly at the code related to inspecting the selection. If this matcher still respects affinity of the selection, but ignores the affinity of the node positions, then that's a very specific differentiation. I wonder if that nuance might get lost and misunderstood over time.
Does this matcher need to care about selection affinity? Would it make sense to look only at the bounds, while ignoring all affinity?
There was a problem hiding this comment.
If this matcher still respects affinity of the selection, but ignores the affinity of the node positions, then that's a very specific differentiation.
This is what it does.
Does this matcher need to care about selection affinity? Would it make sense to look only at the bounds, while ignoring all affinity?
I think it should care about selection affinity. As it was mentioned, the selection affinity will impact the selection rendering.
[SuperEditor][iOS] Add tests for drag to select. Resolves #1613
It was reported in #1610 that performing a horizontal drag isn't changing the selection. #1611 fixed the bug, but didn't include any tests.
This PR adds test for horizontal dragging inside a
CustomScrollView. During the tests I got some failures because dragging can cause anupstreamselection affinity. I added a customMatcherto compare the selections, ignoring the affinity. We have something similar to this inNodePosition.isEquivalentTo.Initially, these tests weren't failing, even without the fix. The reason is that
dragSelectDocumentFromPositionByOffsetalways usesPointerDeviceKind.mouseas the device kind when starting the gesture. As this device kind isn't considered a drag device in mobile, theCustomScrollViewdidn't try to accept the gesture.This PR adds a
pointerDeviceKindparameter todragSelectDocumentFromPositionByOffset, so we can override the device kind.We should consider changing all
dragXXmethods to default the device kind depending on the platform