Skip to content

Commit

Permalink
Fix SelectionArea select-word edge cases (#136920)
Browse files Browse the repository at this point in the history
This change fixes issues with screen order comparison logic when rects are encompassed within each other. This was causing issues when trying to select text that includes inline `WidgetSpan`s inside of a `SelectionArea`.

* Adds `boundingBoxes` to `Selectable` for a more precise hit testing region.

Fixes #132821
Fixes updating selection edge by word boundary when widget spans are involved.
Fixes crash when sending select word selection event to an unselectable element.
  • Loading branch information
Renzo-Olivares authored Dec 11, 2023
1 parent 67edaef commit f60e54b
Show file tree
Hide file tree
Showing 8 changed files with 369 additions and 58 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,9 @@ class _RenderSelectableAdapter extends RenderProxyBox with Selectable, Selection

// Selectable APIs.

@override
List<Rect> get boundingBoxes => <Rect>[paintBounds];

// Adjust this value to enlarge or shrink the selection highlight.
static const double _padding = 10.0;
Rect _getSelectionHighlightRect() {
Expand Down
79 changes: 60 additions & 19 deletions packages/flutter/lib/src/rendering/paragraph.dart
Original file line number Diff line number Diff line change
Expand Up @@ -409,7 +409,13 @@ class RenderParagraph extends RenderBox with ContainerRenderObjectMixin<RenderBo
if (end == -1) {
end = plainText.length;
}
result.add(_SelectableFragment(paragraph: this, range: TextRange(start: start, end: end), fullText: plainText));
result.add(
_SelectableFragment(
paragraph: this,
range: TextRange(start: start, end: end),
fullText: plainText,
),
);
start = end;
}
start += 1;
Expand Down Expand Up @@ -1314,7 +1320,7 @@ class RenderParagraph extends RenderBox with ContainerRenderObjectMixin<RenderBo
/// [PlaceholderSpan]. The [RenderParagraph] splits itself on [PlaceholderSpan]
/// to create multiple `_SelectableFragment`s so that they can be selected
/// separately.
class _SelectableFragment with Selectable, ChangeNotifier implements TextLayoutMetrics {
class _SelectableFragment with Selectable, Diagnosticable, ChangeNotifier implements TextLayoutMetrics {
_SelectableFragment({
required this.paragraph,
required this.fullText,
Expand Down Expand Up @@ -1366,7 +1372,6 @@ class _SelectableFragment with Selectable, ChangeNotifier implements TextLayoutM
? startOffsetInParagraphCoordinates
: paragraph._getOffsetForPosition(TextPosition(offset: selectionEnd));
final bool flipHandles = isReversed != (TextDirection.rtl == paragraph.textDirection);
final Matrix4 paragraphToFragmentTransform = getTransformToParagraph()..invert();
final TextSelection selection = TextSelection(
baseOffset: selectionStart,
extentOffset: selectionEnd,
Expand All @@ -1377,12 +1382,12 @@ class _SelectableFragment with Selectable, ChangeNotifier implements TextLayoutM
}
return SelectionGeometry(
startSelectionPoint: SelectionPoint(
localPosition: MatrixUtils.transformPoint(paragraphToFragmentTransform, startOffsetInParagraphCoordinates),
localPosition: startOffsetInParagraphCoordinates,
lineHeight: paragraph._textPainter.preferredLineHeight,
handleType: flipHandles ? TextSelectionHandleType.right : TextSelectionHandleType.left
),
endSelectionPoint: SelectionPoint(
localPosition: MatrixUtils.transformPoint(paragraphToFragmentTransform, endOffsetInParagraphCoordinates),
localPosition: endOffsetInParagraphCoordinates,
lineHeight: paragraph._textPainter.preferredLineHeight,
handleType: flipHandles ? TextSelectionHandleType.left : TextSelectionHandleType.right,
),
Expand Down Expand Up @@ -1665,7 +1670,16 @@ class _SelectableFragment with Selectable, ChangeNotifier implements TextLayoutM
// we do not need to look up the word boundary for that position. This is to
// maintain a selectables selection collapsed at 0 when the local position is
// not located inside its rect.
final _WordBoundaryRecord? wordBoundary = !_rect.contains(localPosition) ? null : _getWordBoundaryAtPosition(position);
_WordBoundaryRecord? wordBoundary = _rect.contains(localPosition) ? _getWordBoundaryAtPosition(position) : null;
if (wordBoundary != null
&& (wordBoundary.wordStart.offset < range.start && wordBoundary.wordEnd.offset <= range.start
|| wordBoundary.wordStart.offset >= range.end && wordBoundary.wordEnd.offset > range.end)) {
// When the position is located at a placeholder inside of the text, then we may compute
// a word boundary that does not belong to the current selectable fragment. In this case
// we should invalidate the word boundary so that it is not taken into account when
// computing the target position.
wordBoundary = null;
}
final TextPosition targetPosition = _clampTextPosition(isEnd ? _updateSelectionEndEdgeByWord(wordBoundary, position, existingSelectionStart, existingSelectionEnd) : _updateSelectionStartEdgeByWord(wordBoundary, position, existingSelectionStart, existingSelectionEnd));

_setSelectionPosition(targetPosition, isEnd: isEnd);
Expand Down Expand Up @@ -1717,23 +1731,26 @@ class _SelectableFragment with Selectable, ChangeNotifier implements TextLayoutM
}

SelectionResult _handleSelectWord(Offset globalPosition) {
_selectableContainsOriginWord = true;

final TextPosition position = paragraph.getPositionForOffset(paragraph.globalToLocal(globalPosition));
if (_positionIsWithinCurrentSelection(position) && _textSelectionStart != _textSelectionEnd) {
return SelectionResult.end;
}
final _WordBoundaryRecord wordBoundary = _getWordBoundaryAtPosition(position);
if (wordBoundary.wordStart.offset < range.start && wordBoundary.wordEnd.offset < range.start) {
// This fragment may not contain the word, decide what direction the target
// fragment is located in. Because fragments are separated by placeholder
// spans, we also check if the beginning or end of the word is touching
// either edge of this fragment.
if (wordBoundary.wordStart.offset < range.start && wordBoundary.wordEnd.offset <= range.start) {
return SelectionResult.previous;
} else if (wordBoundary.wordStart.offset > range.end && wordBoundary.wordEnd.offset > range.end) {
} else if (wordBoundary.wordStart.offset >= range.end && wordBoundary.wordEnd.offset > range.end) {
return SelectionResult.next;
}
// Fragments are separated by placeholder span, the word boundary shouldn't
// expand across fragments.
assert(wordBoundary.wordStart.offset >= range.start && wordBoundary.wordEnd.offset <= range.end);
_textSelectionStart = wordBoundary.wordStart;
_textSelectionEnd = wordBoundary.wordEnd;
_selectableContainsOriginWord = true;
return SelectionResult.end;
}

Expand Down Expand Up @@ -1957,13 +1974,9 @@ class _SelectableFragment with Selectable, ChangeNotifier implements TextLayoutM
}
}

Matrix4 getTransformToParagraph() {
return Matrix4.translationValues(_rect.left, _rect.top, 0.0);
}

@override
Matrix4 getTransformTo(RenderObject? ancestor) {
return getTransformToParagraph()..multiply(paragraph.getTransformTo(ancestor));
return paragraph.getTransformTo(ancestor);
}

@override
Expand All @@ -1982,6 +1995,28 @@ class _SelectableFragment with Selectable, ChangeNotifier implements TextLayoutM
}
}

List<Rect>? _cachedBoundingBoxes;
@override
List<Rect> get boundingBoxes {
if (_cachedBoundingBoxes == null) {
final List<TextBox> boxes = paragraph.getBoxesForSelection(
TextSelection(baseOffset: range.start, extentOffset: range.end),
);
if (boxes.isNotEmpty) {
_cachedBoundingBoxes = <Rect>[];
for (final TextBox textBox in boxes) {
_cachedBoundingBoxes!.add(textBox.toRect());
}
} else {
final Offset offset = paragraph._getOffsetForPosition(TextPosition(offset: range.start));
final Rect rect = Rect.fromPoints(offset, offset.translate(0, - paragraph._textPainter.preferredLineHeight));
_cachedBoundingBoxes = <Rect>[rect];
}
}
return _cachedBoundingBoxes!;
}

Rect? _cachedRect;
Rect get _rect {
if (_cachedRect == null) {
final List<TextBox> boxes = paragraph.getBoxesForSelection(
Expand All @@ -2000,7 +2035,6 @@ class _SelectableFragment with Selectable, ChangeNotifier implements TextLayoutM
}
return _cachedRect!;
}
Rect? _cachedRect;

void didChangeParagraphLayout() {
_cachedRect = null;
Expand Down Expand Up @@ -2028,12 +2062,11 @@ class _SelectableFragment with Selectable, ChangeNotifier implements TextLayoutM
textBox.toRect().shift(offset), selectionPaint);
}
}
final Matrix4 transform = getTransformToParagraph();
if (_startHandleLayerLink != null && value.startSelectionPoint != null) {
context.pushLayer(
LeaderLayer(
link: _startHandleLayerLink!,
offset: offset + MatrixUtils.transformPoint(transform, value.startSelectionPoint!.localPosition),
offset: offset + value.startSelectionPoint!.localPosition,
),
(PaintingContext context, Offset offset) { },
Offset.zero,
Expand All @@ -2043,7 +2076,7 @@ class _SelectableFragment with Selectable, ChangeNotifier implements TextLayoutM
context.pushLayer(
LeaderLayer(
link: _endHandleLayerLink!,
offset: offset + MatrixUtils.transformPoint(transform, value.endSelectionPoint!.localPosition),
offset: offset + value.endSelectionPoint!.localPosition,
),
(PaintingContext context, Offset offset) { },
Offset.zero,
Expand Down Expand Up @@ -2071,4 +2104,12 @@ class _SelectableFragment with Selectable, ChangeNotifier implements TextLayoutM

@override
TextRange getWordBoundary(TextPosition position) => paragraph.getWordBoundary(position);

@override
void debugFillProperties(DiagnosticPropertiesBuilder properties) {
super.debugFillProperties(properties);
properties.add(DiagnosticsProperty<String>('textInsideRange', range.textInside(fullText)));
properties.add(DiagnosticsProperty<TextRange>('range', range));
properties.add(DiagnosticsProperty<String>('fullText', fullText));
}
}
4 changes: 4 additions & 0 deletions packages/flutter/lib/src/rendering/selection.dart
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,10 @@ mixin Selectable implements SelectionHandler {
/// The size of this [Selectable].
Size get size;

/// A list of [Rect]s that represent the bounding box of this [Selectable]
/// in local coordinates.
List<Rect> get boundingBoxes;

/// Disposes resources held by the mixer.
void dispose();
}
Expand Down
37 changes: 22 additions & 15 deletions packages/flutter/lib/src/widgets/selectable_region.dart
Original file line number Diff line number Diff line change
Expand Up @@ -1817,6 +1817,14 @@ abstract class MultiSelectableSelectionContainerDelegate extends SelectionContai
_updateHandleLayersAndOwners();
}

Rect _getBoundingBox(Selectable selectable) {
Rect result = selectable.boundingBoxes.first;
for (int index = 1; index < selectable.boundingBoxes.length; index += 1) {
result = result.expandToInclude(selectable.boundingBoxes[index]);
}
return result;
}

/// The compare function this delegate used for determining the selection
/// order of the selectables.
///
Expand All @@ -1827,11 +1835,11 @@ abstract class MultiSelectableSelectionContainerDelegate extends SelectionContai
int _compareScreenOrder(Selectable a, Selectable b) {
final Rect rectA = MatrixUtils.transformRect(
a.getTransformTo(null),
Rect.fromLTWH(0, 0, a.size.width, a.size.height),
_getBoundingBox(a),
);
final Rect rectB = MatrixUtils.transformRect(
b.getTransformTo(null),
Rect.fromLTWH(0, 0, b.size.width, b.size.height),
_getBoundingBox(b),
);
final int result = _compareVertically(rectA, rectB);
if (result != 0) {
Expand All @@ -1846,6 +1854,7 @@ abstract class MultiSelectableSelectionContainerDelegate extends SelectionContai
/// Returns positive if a is lower, negative if a is higher, 0 if their
/// order can't be determine solely by their vertical position.
static int _compareVertically(Rect a, Rect b) {
// The rectangles overlap so defer to horizontal comparison.
if ((a.top - b.top < _kSelectableVerticalComparingThreshold && a.bottom - b.bottom > - _kSelectableVerticalComparingThreshold) ||
(b.top - a.top < _kSelectableVerticalComparingThreshold && b.bottom - a.bottom > - _kSelectableVerticalComparingThreshold)) {
return 0;
Expand All @@ -1863,19 +1872,10 @@ abstract class MultiSelectableSelectionContainerDelegate extends SelectionContai
static int _compareHorizontally(Rect a, Rect b) {
// a encloses b.
if (a.left - b.left < precisionErrorTolerance && a.right - b.right > - precisionErrorTolerance) {
// b ends before a.
if (a.right - b.right > precisionErrorTolerance) {
return 1;
}
return -1;
}

// b encloses a.
if (b.left - a.left < precisionErrorTolerance && b.right - a.right > - precisionErrorTolerance) {
// a ends before b.
if (b.right - a.right > precisionErrorTolerance) {
return -1;
}
return 1;
}
if ((a.left - b.left).abs() > precisionErrorTolerance) {
Expand Down Expand Up @@ -2140,10 +2140,17 @@ abstract class MultiSelectableSelectionContainerDelegate extends SelectionContai
SelectionResult handleSelectWord(SelectWordSelectionEvent event) {
SelectionResult? lastSelectionResult;
for (int index = 0; index < selectables.length; index += 1) {
final Rect localRect = Rect.fromLTWH(0, 0, selectables[index].size.width, selectables[index].size.height);
final Matrix4 transform = selectables[index].getTransformTo(null);
final Rect globalRect = MatrixUtils.transformRect(transform, localRect);
if (globalRect.contains(event.globalPosition)) {
bool globalRectsContainsPosition = false;
if (selectables[index].boundingBoxes.isNotEmpty) {
for (final Rect rect in selectables[index].boundingBoxes) {
final Rect globalRect = MatrixUtils.transformRect(selectables[index].getTransformTo(null), rect);
if (globalRect.contains(event.globalPosition)) {
globalRectsContainsPosition = true;
break;
}
}
}
if (globalRectsContainsPosition) {
final SelectionGeometry existingGeometry = selectables[index].value;
lastSelectionResult = dispatchSelectionEventToChild(selectables[index], event);
if (index == selectables.length - 1 && lastSelectionResult == SelectionResult.next) {
Expand Down
3 changes: 3 additions & 0 deletions packages/flutter/lib/src/widgets/selection_container.dart
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,9 @@ class _SelectionContainerState extends State<SelectionContainer> with Selectable
@override
Size get size => (context.findRenderObject()! as RenderBox).size;

@override
List<Rect> get boundingBoxes => <Rect>[(context.findRenderObject()! as RenderBox).paintBounds];

@override
void dispose() {
if (!widget._disabled) {
Expand Down
2 changes: 1 addition & 1 deletion packages/flutter/test/material/selection_area_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ void main() {
),
);

final TestGesture longpress = await tester.startGesture(const Offset(10, 10));
final TestGesture longpress = await tester.startGesture(tester.getCenter(find.byType(Text)));
addTearDown(longpress.removePointer);
await tester.pump(const Duration(milliseconds: 500));
await longpress.up();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -138,9 +138,14 @@ class RenderSelectionSpy extends RenderProxyBox
Size get size => _size;
Size _size = Size.zero;

@override
List<Rect> get boundingBoxes => _boundingBoxes;
final List<Rect> _boundingBoxes = <Rect>[];

@override
Size computeDryLayout(BoxConstraints constraints) {
_size = Size(constraints.maxWidth, constraints.maxHeight);
_boundingBoxes.add(Rect.fromLTWH(0.0, 0.0, constraints.maxWidth, constraints.maxHeight));
return _size;
}

Expand Down
Loading

0 comments on commit f60e54b

Please sign in to comment.