Fix selecting backwards when involving widget spans#184423
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the selection logic in MultiSelectableSelectionContainerDelegate to support bidirectional sweeping starting from an existing selection edge, which addresses issues with backwards selection. It also adds a regression test to verify selection behavior across multiple Text widgets and WidgetSpan elements. Review feedback suggests simplifying the control flow within the SelectionResult switch cases to improve readability and using the null-coalescing operator for a more idiomatic index update.
|
I created another PR because I somehow managed to break my old one (and a few others). Apologies for any confusion. |
b808519 to
ec28d05
Compare
| final Offset localPosition = MatrixUtils.transformPoint(transform, globalPosition); | ||
| if (_rect.isEmpty) { | ||
| return SelectionUtils.getResultBasedOnRect(_rect, localPosition); | ||
| final SelectionResult result = SelectionUtils.getResultBasedOnRect(_rect, localPosition); |
There was a problem hiding this comment.
Here and elsewhere we snap the selection edge to the appropriate boundary so that this fragment's internal state is consistent with the SelectionResult it reports. Without this, the edge stays null and the fragment creates a gap in the selection range.
This covers the test, 'triple-click-drag backwards involving WidgetSpans', where a selectable fragment may be of zero-width since it only contains a new line. Without it the new lines are not included in the selection.
a76c1fe to
ecb68c4
Compare
loic-sharma
left a comment
There was a problem hiding this comment.
This looks good to me, but my understanding of selection is weak so please also get an approval from @chunhtai! :)
|
Google testing failures seem unrelated on this one. |
a8576c4
…t manage multiple selectables
…lementation does the same thing
a8576c4 to
9152072
Compare
|
I have applied the suggestion in #184423 (comment). |
Original discussion: #184031
Fixes #166462
Before this change
_initSelectionwould always begin its selection search at index 0, this caused issues when selecting backwards and the children of the selection delegate also contained children. This happens because when we select backwards and always start our selection search at index 0, if our pointer moves backwards from point b to point a and point a is already on index 0 (of a selection container with N selectables), the selection logic will short-circuit early thinking we found the selection, skipping any selectables in between point b and point a.After this change
_initSelectionwill begin its selection search at the opposite edge if it exists. This fixes the issue because when entering a new selectable, while dragging backwards and updating the end edge, first the selectable will callensureChildUpdatedand synthesize a start edge. Then it processes the end edge event and begins to search for that selection edge, we now start this search at the opposite edge which allows us ensure all items between both edges are visited so we can create a continuous selection.Since starting
_initSelectionat the opposite edge is what_SelectableTextContainerDelegatedoes with its_initSelectionwe can remove it. I added a test to verify we aren't breaking anything here.Pre-launch Checklist
///).