Unverified Commit f60e54b2 authored by Renzo Olivares's avatar Renzo Olivares Committed by GitHub

Fix SelectionArea select-word edge cases (#136920)

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.
parent 67edaef9
......@@ -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() {
......
......@@ -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;
......@@ -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,
......@@ -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,
......@@ -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,
),
......@@ -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);
......@@ -1717,16 +1731,18 @@ 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
......@@ -1734,6 +1750,7 @@ class _SelectableFragment with Selectable, ChangeNotifier implements TextLayoutM
assert(wordBoundary.wordStart.offset >= range.start && wordBoundary.wordEnd.offset <= range.end);
_textSelectionStart = wordBoundary.wordStart;
_textSelectionEnd = wordBoundary.wordEnd;
_selectableContainsOriginWord = true;
return SelectionResult.end;
}
......@@ -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
......@@ -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(
......@@ -2000,7 +2035,6 @@ class _SelectableFragment with Selectable, ChangeNotifier implements TextLayoutM
}
return _cachedRect!;
}
Rect? _cachedRect;
void didChangeParagraphLayout() {
_cachedRect = null;
......@@ -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,
......@@ -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,
......@@ -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));
}
}
......@@ -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();
}
......
......@@ -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.
///
......@@ -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) {
......@@ -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;
......@@ -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) {
......@@ -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) {
......
......@@ -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) {
......
......@@ -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();
......
......@@ -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;
}
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment