handleSelectWord in MultiSelectableSelectionContainerDelegate should handle rects inside of rects

[Renzo-Olivares]

Fixes #127076

Sometimes a Selectables rect may contain another Selectables rect within it. In the case of handleSelectWord when choosing which Selectable to dispatch the SelectionEvent to, the event would be dispatched to the wrong Selectable causing an assertion error to be thrown.

In the picture above the red outline shows the rect of a two-line piece of text. And the blue rect shows the rect of a piece of text that is on the second line of the two-line piece of text, but has been separated into its own rect for some case, for example when TextSpans are separated by a WidgetSpan.

We should check if the text layout of the selectable that has been dispatched the SelectionEvent contains the word, if not then return SelectionResult.next, and continue to look through the list of selectables.

Pre-launch Checklist

• I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
• I read the [Tree Hygiene] wiki page, which explains my responsibilities.
• I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
• I signed the [CLA].
• I listed at least one issue that this PR fixes in the description above.
• I updated/added relevant documentation (doc comments with ///).
• I added new tests to check the change I am making, or this PR is [test-exempt].
• All existing and new tests are passing.

[justinmc]

LGTM but maybe give @chunhtai a chance to take a look too.

[justinmc]

Is this making this O(n^2) instead of O(n) before? Not sure if this is a bottle neck at all, just pointing it out.

[Renzo-Olivares]

Yeah this would be O(n^2) now. I couldn't think of a re-factor for the current solution that would make this O(n). @chunhtai do you have any thoughts on this?

[chunhtai]

I am not sure if this is the right fix. I thought we may run into the same problem even if the inner selectable is not completely contained by the outer one?

In the example

It seems to me the right behavior is that if the selectword at the inside of inner selectable, the outer box should return SelectionResult.next because it should have known the coordinate is outside its actual text layout.

[Renzo-Olivares]

That makes a lot of sense! That makes the solution a lot simpler and keeps this O(n). Thanks for the input!

[justinmc]

LGTM 👍

Good call @chunhtai, this way seems a lot better.

[chunhtai]

There should only be three situation

1. both word.end and word.start is smaller than range.start
2. both word.end and word.start is larger than range.end
3. range completely cover the word.

(1) should return prev.
(2) should return next.
(3) should return end.

if not in above three case it should throw.

[chunhtai]

Since we change the selectword contract that it should return next/prev we also need to update handleSelectWord to return pre/next. i.e. if all selectable return next. or if the selectable return prev.

Also should probably assert if a selectable return prev, it must be the first in the selectables.

[Renzo-Olivares]

I think I understand why this is needed but for clarification purposes can you confirm my assumptions. We need to update handleSelectWord to return next/prev because before this change it only return none/end. none if the word was not selected given the list of selectables managed by the container delegate. And end if the word was selected selected given the list of selectables.

Now since select word can return next/prev, handleSelectWord should be updated to also do this in the following cases:
(1) return next when the last SelectionResult is next
(2) return prev when the last SelectionResult is prev

Is this correct?

Also should probably assert if a selectable return prev, it must be the first in the selectables.

• Not sure I completely understand why for this.

[chunhtai]

none if the word was not selected given the list of selectables managed by the container delegate. And end if the word was selected selected given the list of selectables.

This is correct prior to this change, though the return value was never used so it doesn't really matter what value is return.

Now since select word can return next/prev, handleSelectWord should be updated to also do this in the following cases:
(1) return next when the last SelectionResult is next
(2) return prev when the last SelectionResult is prev

(2) is incorrect, handleSelectWord should only return prev if the selectionResult of first selectable return prev.

Is this correct?

yes this is correct and it is necessary because this selectionContainer can be a child selectable of another container, and it can also have the overlap issue with another selectable.

Not sure I completely understand why for this.

Nvm I think i was wrong, it is possible a selectable that is not the first selectable.

[Renzo-Olivares]

Thank you for the clarification, I think I got it!

[chunhtai]

This seems weird, it should stop as soon as dispatchSelectionEventToChild does not return next.

imagine this case

in sequential order

selectable1 -> next (continue)
selectable2 -> next (continue)
selectable3 -> prev (should stop)

this meant the selectword land in between selectable 2 and 3, one of them should be set to the currentSelectionStartIndex and currentSelectionEndIndex, I don't think it matter which one.

[Renzo-Olivares]

That makes sense, but is this assuming that the selectables are in order of how the text is visually laid out?

Trying something like the following code, and the first selectable in the list is Hello world this is some long text that goes\non two lines. The second selectable is Word, and the third selectable is Word Word2.

If I change it to stop as soon as dispatchSelectionEventToChild does not return next then I get the following.

In sequential order this happens.

selectable1 -> prev (should stop)
currentSelectionStartIndex/currentSelectionEndIndex : null
should return SelectionResult.prev

Is this expected? Trying to right click Word at the beginning of the text fails since we never reach it.

import 'package:flutter/material.dart';

void main() {
  runApp(const MainApp());
}

class MainApp extends StatelessWidget {
  const MainApp({super.key});

  @override
  Widget build(BuildContext context) {
    return const MaterialApp(
      home: Scaffold(
        body: Center(
          child: SelectionArea(
            child: Text.rich(
              TextSpan(
                children: <InlineSpan>[
                  TextSpan(text: 'Word'),
                  WidgetSpan(child: SizedBox.shrink()),
                  TextSpan(text: ' Hello world this is some long text that goes\non two lines.'),
                  WidgetSpan(child: SizedBox.shrink()),
                  TextSpan(text: 'Word'),
                  TextSpan(text: ' Word2'),
                ],
              ),
            ),
          ),
        ),
      ),
    );
  }
}

[chunhtai]

the selectables are in order of how the text is visually laid out?

they should be. I think the issue you encounter was a bug that the function to calculate screen order does not do well when the rect overlaps.

flutter/packages/flutter/lib/src/widgets/selectable_region.dart

Line 1635 in 764f4cc

int _compareScreenOrder(Selectable a, Selectable b) {

[Renzo-Olivares]

Oh okay I have tried to fix that, and have updated the handleSelectWord logic as well.

[chunhtai]

This doesn't look right. consider these cases, assuming 5 child, skip means the selectword point is not in the rect

selectable 1 -> skipped (continue)
selectable 2 -> next (continue)
selectable 3 -> next (continue)
selectable 4 -> prev (stop here)

currentSelectionStartIndex/currentSelectionEndIndex : 3 or 4
should return SelectionResult.end

selectable 1 -> skipped (continue)
selectable 2 -> prev (stop here)

currentSelectionStartIndex/currentSelectionEndIndex : 2
should return SelectionResult.end

selectable 1 -> prev (stop here)

currentSelectionStartIndex/currentSelectionEndIndex : null
should return SelectionResult.prev

selectable 1 -> skip (continue)
selectable 2 -> next (continue)
selectable 3 -> end (stop here)

currentSelectionStartIndex/currentSelectionEndIndex : 3
should return SelectionResult.end

selectable 1 -> skip (continue)
selectable 2 -> next (continue)
selectable 3 -> skip (stop here)

currentSelectionStartIndex/currentSelectionEndIndex : 2
should return SelectionResult.end

selectable 1 -> skip (continue)
selectable 2 -> skip (continue)
selectable 3 -> skip (continue)
selectable 4 -> next (continue)
selectable 5 -> next (at the end)

currentSelectionStartIndex/currentSelectionEndIndex : null
should return SelectionResult.next

selectable 1 -> skip (continue)
selectable 2 -> skip (continue)
selectable 3 -> skip (continue)
selectable 4 -> skip (continue)
selectable 5 -> skip (at the end)

currentSelectionStartIndex/currentSelectionEndIndex : null
should probably return SelectionResult.next

[chunhtai]

It seems this is still not addressed #127478 (comment)

[Renzo-Olivares]

You're right, think it should be fixed now. But I'm a bit unsure about this case

5.
selectable 1 -> skip (continue)
selectable 2 -> next (continue)
selectable 3 -> skip (stop here)

currentSelectionStartIndex/currentSelectionEndIndex : 2
should return SelectionResult.end

Why does it stop at the last skip?

[chunhtai]

This assume all selectable after 3 are skipped. If not, it is likely a bug in sorting screen order. I am actually not sure whether this should return next or end, but I can't think of a case returning end would be a problem.

[Renzo-Olivares]

I see, so what this case means is that in a given list of selectables a next was reported but it was not followed by a prev/or end that would indicate the word is inside of the list of selectables? Why in this case then do we still have to set the currentSelectionStartIndex/currentSelectionEndIndex : 2, when that might not be where the word is? Thank you for explaining.

[chunhtai]

After I thought about it more, I think we should return end to short-circuit the logic. Otherwise the select world logic would need to go through every selectable in the subtree. to figure out whether the position has hit any selectables.

There are two mechanism in play here: the rect and selection result. In theory, we don't need rect, and we can completely rely on selection result. The rect is here so that we can trim the search tree. If we were to return next in this kind of un-decided case, we might as well remove the rect check completely.

That also reminding me I was wrong on a previous case. These two case should be the following

selectable 1 -> skip (continue)
selectable 2 -> next (continue)
selectable 3 -> skip (stop here)

currentSelectionStartIndex/currentSelectionEndIndex : 2
should return SelectionResult.end

7
selectable 1 -> skip (continue)
selectable 2 -> skip (continue)
selectable 3 -> skip (continue)
selectable 4 -> skip (continue)
selectable 5 -> skip (at the end)

currentSelectionStartIndex/currentSelectionEndIndex : null
should return SelectionResult.end

[chunhtai]

basically,

if the first selection return prev, this method return prev
if the [n...length-1] return next, this method return next
else return SelectionResult.end.

[Renzo-Olivares]

Thank you for the very in depth explanation. I understand now.

[chunhtai]

This method should never return none

[chunhtai]

LGTM, just one concern about the assert being to strict

[chunhtai]

This could be null if every selectable is skipped?

[Renzo-Olivares]

This should be okay right? We should always return inside of the loop, if we are not that means every selectable was skipped, and in that case lastSelectionResult should always be null. Can't really think of a case where we don't return inside of the loop besides the screen order not being correct.

If lastSelectionResult == prev is inside the loop, it will always return, same for end, and also next followed by a skip, and next as the final selectable.

[chunhtai]

oh sorry I read the assert wrong. Yes this is correct.