Fix SelectableRegion crash when the selection starts in a scrollable child but does not select anything initially

[Renzo-Olivares]

Original discussion: #184164

Fixes #115787

Before this change sometimes when starting a selection gesture such as a long press drag or a double tap drag directly on the empty padding of a Scrollable the selection may not be set initially because there was nothing to select in the empty padding. This caused a crash when receiving subsequent edge update events because we believe the selection starts in the scrollable but we do not have any selection edges set.

To fix this I made _selectionStartsInScrollable nullable, this way a subsequent selection edge will not try to set _selectionStartsInScrollable again if it has already been set removing the need for the assert. handleSelectionEdgeUpdate previously assumed that if the cached edges were not set then we should set _selectionStartsInScrollable, the assert was used to verify that _selectionStartsInScrollable was false i.e. had not been set/changed from its initial value. By making _selectionStartsInScrollable nullable we can ensure a subsequent edge update event does not re-set the value if it has already been set previously.

Pre-launch Checklist

• I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
• I read the [AI contribution guidelines] and understand my responsibilities, or I am not using AI tools.
• 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].
• I followed the [breaking change policy] and added [Data Driven Fixes] where supported.
• All existing and new tests are passing.

[gemini-code-assist]

Code Review

This pull request converts _selectionStartsInScrollable to a nullable boolean to fix a crash when starting selections in scrollable padding and adds corresponding regression tests. Review feedback suggests consistently using the non-null assertion operator ! in logic where the variable is guaranteed to be non-null to improve code conciseness and consistency.

[Renzo-Olivares]

I created another PR because I somehow managed to break my old one (and a few others). Apologies for any confusion.

[loic-sharma]

Just curious, why ??= instead of =?

My expectation would be that if both _currentDragStartRelatedToOrigin and _currentDragEndRelatedToOrigin are null, then _selectionStartsInScrollable is also null?

[loic-sharma]

Ahh this comment cleared up my confusion: #115787 (comment)

The problem here is that handleSelectWord is called before handleSelectionEdgeUpdate, and handleSelectWord will set _selectionStartsInScrollable but not _currentDragStartRelatedToOrigin/_currentDragEndRelatedToOrigin.

[loic-sharma]

@Renzo-Olivares What do you think of adding a comment like:

// _selectionStartsInScrollable might already be set by some other event,
// like [handleSelectWord].

That would help clarify why ??= is needed here.

[chunhtai]

Can you remind me the conclusion from the previous conversation that either select word on empty space should set the selection edge to the closest fragment or not set _selectionStartsInScrollable to true?

my original thought is that if there is a selection, one of the selection edge must not be null. I think we should keep this contract.

so depending on the behavior of select word on empty space.

1. if this counts as not creating a selection, both edge should be null and _selectionStartsInScrollable should stay false
2. If this counts as creating a selection, both edge should be set to collapse some where in the selectable, and _selectionStartsInScrollable should be set to true.

[Renzo-Olivares]

@chunhtai We did not conclude our previous conversation in the old PR #184164 (comment). Sorry I should have added a note about that.

Thanks for explaining your original thought process. After reading it, I definitely agree that we should keep the original contract intact.

Instead of collapsing the selection, I'm leaning towards selecting the nearest word. Here is my reasoning:

Currently, SelectableRegion doesn't check if a selection is collapsed before calling showToolbar or showHandles. If we were to collapse the selection here, we'd have to add those checks to prevent the selection overlay from showing up on a collapsed selection.

Maybe it's okay to add these checks but, it shifts our current assumptions. Right now, when gesture callbacks trigger in SelectableRegion, we assume that dispatching an event to the selection tree will result in a valid selection (e.g., SelectWordSelectionEvent will actually select a word). Adding checks for a collapsed selection at the SelectableRegion level implies a new state: 'we won the arena and consumed the gesture, but the selection tree potentially failed to set the type of selection we expected.' If the delegate handles this and selects the nearest word we keep the original contract, 'we won the arena and consumed the gesture, and the selection tree set the expected selection'.

Is that reasonable?

Some additional notes:

On the web the behavior generally selects the nearest word even when on empty padding (there is some nuance to this, for example double-clicking in empty padding above the first line of a multi-line text always selects the first word, and double-clicking under the empty padding below the last line selects nothing). On mobile chrome the nearest word is selected on the first line when doing a gesture in the empty padding so this seems to very by platform. https://jsfiddle.net/jbw1ck84/2/ for a demo

On jetpack compose with SelectionContainer a gesture on the empty padding is consumed by the Scrollable.

[loic-sharma]

Thanks for the excellent demo! This behavior sounds good to me, but I'll defer to @chunhtai as they have more context on selection invariants.

[chunhtai]

we assume that dispatching an event to the selection tree will result in a valid selection (e.g., SelectWordSelectionEvent will actually select a word)

Can you point me to the code where we made this assumption? I find it weird that we always expect a word is selected, but maybe I forgot some assumptions i made before.

As for whether select nearest word, I don't have strong opinion. I think select word at an empty space doesn't seem like a normal gesture where user will expect a specific result.

[Renzo-Olivares]

For example at the end of a long press we always call _showToolbar and do not confirm whether a word was actually selected.

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

Lines 1020 to 1028 in e4ae4e6

	void _handleTouchLongPressEnd(LongPressEndDetails details) {
	_finalizeSelection();
	_updateSelectedContentIfNeeded();
	_finalizeSelectableRegionStatus();
	_showToolbar();
	if (defaultTargetPlatform == TargetPlatform.android) {
	_showHandles();
	}
	}

Similarly for a double-click on tap up,

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

Lines 942 to 966 in e4ae4e6

	switch (_getEffectiveConsecutiveTapCount(details.consecutiveTapCount)) {
	case 1:
	switch (defaultTargetPlatform) {
	case TargetPlatform.android:
	case TargetPlatform.fuchsia:
	case TargetPlatform.iOS:
	hideToolbar();
	_collapseSelectionAt(offset: details.globalPosition);
	_selectionStatusNotifier.value = SelectableRegionSelectionStatus.changing;
	case TargetPlatform.macOS:
	case TargetPlatform.linux:
	case TargetPlatform.windows:
	// On desktop platforms the selection is set on tap down.
	}
	case 2:
	final bool isPointerPrecise = _isPrecisePointerDevice(details.kind);
	switch (defaultTargetPlatform) {
	case TargetPlatform.android:
	case TargetPlatform.fuchsia:
	if (!isPointerPrecise) {
	// On Android, a double tap will only show the selection overlay after
	// the following tap up when the pointer device kind is not precise.
	_showHandles();
	_showToolbar();
	}

This is similar to TextSelectionGestureDetector, i.e. a call to showToolbar will always show the toolbar as long as a selection overlay is present.

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

Lines 2930 to 2937 in e4ae4e6

	void onDoubleTapDown(TapDragDownDetails details) {
	if (delegate.selectionEnabled) {
	renderEditable.selectWord(cause: SelectionChangedCause.doubleTap);
	if (shouldShowSelectionToolbar) {
	editableText.showToolbar();
	}
	}
	}

All this to say i'm not against adding logic that says "is our selection uncollapsed after selecting a word?, if so then show the toolbar, if not then do not show anything" but it does add more cases to handle.

[loic-sharma]

Looks good to me!

I left some questions, but those are mainly because I don't have a strong understanding of selectable scrollables.

[chunhtai]

I still have concern over this #184420 (comment)

[loic-sharma]

Re-LGTM!

[chunhtai]

LGTM, left a suggestion

[chunhtai]

yeah, you are right I missed we bypasses the selectable based on bounding box

[chunhtai]

Not blocking the merge, but can this and line 3020 be refactor out?

[Renzo-Olivares]

Do you mean if we can do the calculation for _closestSelectableIndex inside the first loop?

[chunhtai]

I meant a share helper method

[Renzo-Olivares]

I tried doing a shared helper method for the boundingBoxes loop but it felt a little clunky since we do extra work in the _closestSelectableIndexTo method and have two different outputs in both loops. Though maybe i'm not following what part should be the share helper method.

I did try some clean up:

8df91c6

and

0257bab

The latter eliminates _closestSelectableIndex and tracks the nearest index in line in the first pass of the boundingBoxes. I think the calculations we do with clampDouble are negligible.

[loic-sharma]

Oh I think @chunhtai was suggesting a helper for the "Dispatch a selection event to a child and reset other selectables if the child's SelectionGeometry changed" logic. I don't feel strongly either-ways though.

[Renzo-Olivares]

I'll land this as is for now, I don't feel strongly either.

[Renzo-Olivares]

Apologies for the re-review, I decided to do a little clean up.

The changes are in:

8df91c6

and

0257bab

[chunhtai]

LGTM

[loic-sharma]

Re-LGTM