Skip to content

Commit

Permalink
RenderParagraphs _SelectableFragment.boundingBoxes should conside…
Browse files Browse the repository at this point in the history 
…r max line height (flutter#155892)

Fixes flutter#133637

This change updates the `_SelectableFragment.boundingBoxes` logic to consider the max line height. Previously we were using boxes that were tightly bound around each glyph, so you would have to click within the bounds of the glyph for double tap to select word to work. This is different than `SelectableText` which considers the max line height, as well as the native web behavior that also considers the max line height.

## Web
https://github.com/user-attachments/assets/4ce8c0ca-ec6f-4969-88b1-baa356be8278

## Flutter SelectableText
https://github.com/user-attachments/assets/54c22ad3-75d7-475b-856b-e9b2dbe09d54

## Flutter Text widget under SelectionArea
https://github.com/user-attachments/assets/27db0e2b-1d19-43cc-8ab3-16050e3a5bc7

After this change, Flutter's Text widget under a SelectionArea now matches the SelectableText and native web behavior.

This change also:
* Invalidates the cached bounding boxes when the paragraph layout changes.
* Updates `textOffsetToPosition` to consider `preferredLineHeight`. In cases when the text wraps, it was sometimes inaccurate.
  • Loading branch information
@Renzo-Olivares @thejitenpatel
Renzo-Olivares authored and thejitenpatel committed Oct 1, 2024
1 parent 711dc06 commit a6141f6
Show file tree
Hide file tree
Showing 2 changed files with 74 additions and 12 deletions.
8 changes: 8 additions & 0 deletions packages/flutter/lib/src/rendering/paragraph.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -701,6 +701,12 @@ class RenderParagraph extends RenderBox with ContainerRenderObjectMixin<RenderBo
.maxIntrinsicWidth;
}

/// An estimate of the height of a line in the text. See [TextPainter.preferredLineHeight].
///
/// This does not require the layout to be updated.
@visibleForTesting
double get preferredLineHeight => _textPainter.preferredLineHeight;

double _computeIntrinsicHeight(double width) {
return (_textIntrinsics
..setPlaceholderDimensions(layoutInlineChildren(width, ChildLayoutHelper.dryLayoutChild, ChildLayoutHelper.getDryBaseline))
Expand Down Expand Up @@ -2997,6 +3003,7 @@ class _SelectableFragment with Selectable, Diagnosticable, ChangeNotifier implem
if (_cachedBoundingBoxes == null) {
final List<TextBox> boxes = paragraph.getBoxesForSelection(
TextSelection(baseOffset: range.start, extentOffset: range.end),
boxHeightStyle: ui.BoxHeightStyle.max,
);
if (boxes.isNotEmpty) {
_cachedBoundingBoxes = <Rect>[];
Expand Down Expand Up @@ -3034,6 +3041,7 @@ class _SelectableFragment with Selectable, Diagnosticable, ChangeNotifier implem

void didChangeParagraphLayout() {
_cachedRect = null;
_cachedBoundingBoxes = null;
}

@override
Expand Down
78 changes: 66 additions & 12 deletions packages/flutter/test/widgets/selectable_region_test.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@ import 'semantics_tester.dart';

Offset textOffsetToPosition(RenderParagraph paragraph, int offset) {
const Rect caret = Rect.fromLTWH(0.0, 0.0, 2.0, 20.0);
final Offset localOffset = paragraph.getOffsetForCaret(TextPosition(offset: offset), caret);
return paragraph.localToGlobal(localOffset);
final Offset localOffset = paragraph.getOffsetForCaret(TextPosition(offset: offset), caret) + Offset(0.0, paragraph.preferredLineHeight);
return paragraph.localToGlobal(localOffset) + const Offset(kIsWeb ? 1.0 : 0.0, -2.0);
}

Offset globalize(Offset point, RenderBox box) {
Expand Down Expand Up @@ -1316,6 +1316,68 @@ void main() {
skip: kIsWeb, // https://github.com/flutter/flutter/issues/125582.
);

testWidgets('RenderParagraph should invalidate cached bounding boxes', (WidgetTester tester) async {
final UniqueKey outerText = UniqueKey();
final FocusNode focusNode = FocusNode();
addTearDown(focusNode.dispose);
addTearDown(tester.view.reset);

await tester.pumpWidget(
MaterialApp(
home: SelectableRegion(
focusNode: focusNode,
selectionControls: materialTextSelectionControls,
child: Scaffold(
body: Center(
child: Text(
'How are you doing today? Good, and you?',
key: outerText,
),
),
),
),
),
);
final RenderParagraph paragraph = tester.renderObject<RenderParagraph>(find.descendant(of: find.byKey(outerText), matching: find.byType(RichText)).first);
final SelectableRegionState state =
tester.state<SelectableRegionState>(find.byType(SelectableRegion));

// Double click to select word at position.
final TestGesture gesture = await tester.startGesture(textOffsetToPosition(paragraph, 27), kind: PointerDeviceKind.mouse);
addTearDown(gesture.removePointer);
await tester.pump();
await gesture.up();
await tester.pump();
await gesture.down(textOffsetToPosition(paragraph, 27));
await tester.pump();
await gesture.up();
await tester.pumpAndSettle();

// Should select "Good".
expect(paragraph.selections[0], const TextSelection(baseOffset: 25, extentOffset: 29));

// Change the size of the window.
tester.view.physicalSize = const Size(800.0, 400.0);
await tester.pumpAndSettle();
state.clearSelection();
await tester.pumpAndSettle(kDoubleTapTimeout);
expect(paragraph.selections.isEmpty, isTrue);

// Double click at the same position.
await gesture.down(textOffsetToPosition(paragraph, 27));
await tester.pump();
await gesture.up();
await tester.pump();
await gesture.down(textOffsetToPosition(paragraph, 27));
await tester.pump();
await gesture.up();
await tester.pumpAndSettle();

// Should select "Good" again.
expect(paragraph.selections.isEmpty, isFalse);
expect(paragraph.selections[0], const TextSelection(baseOffset: 25, extentOffset: 29));
}, skip: kIsWeb); // https://github.com/flutter/flutter/issues/125582.

testWidgets('mouse can select single text on desktop platforms', (WidgetTester tester) async {
final FocusNode focusNode = FocusNode();
addTearDown(focusNode.dispose);
Expand Down Expand Up @@ -3398,12 +3460,8 @@ void main() {
);
final RenderParagraph paragraph = tester.renderObject<RenderParagraph>(find.descendant(of: find.byKey(outerText), matching: find.byType(RichText)).first);

// Adjust `textOffsetToPosition` result because it returns the wrong vertical position (wrong line).
// TODO(bleroux): Remove when https://github.com/flutter/flutter/issues/133637 is fixed.
final Offset gestureOffset = textOffsetToPosition(paragraph, 125).translate(0, 10);

// Right click to select word at position.
final TestGesture gesture = await tester.startGesture(gestureOffset, kind: PointerDeviceKind.mouse, buttons: kSecondaryMouseButton);
final TestGesture gesture = await tester.startGesture(textOffsetToPosition(paragraph, 125), kind: PointerDeviceKind.mouse, buttons: kSecondaryMouseButton);
addTearDown(gesture.removePointer);
await tester.pump();
await gesture.up();
Expand Down Expand Up @@ -3448,12 +3506,8 @@ void main() {
);
final RenderParagraph paragraph = tester.renderObject<RenderParagraph>(find.descendant(of: find.byKey(outerText), matching: find.byType(RichText)).first);

// Adjust `textOffsetToPosition` result because it returns the wrong vertical position (wrong line).
// TODO(bleroux): Remove when https://github.com/flutter/flutter/issues/133637 is fixed.
final Offset gestureOffset = textOffsetToPosition(paragraph, 125).translate(0, 10);

// Right click to select word at position.
final TestGesture gesture = await tester.startGesture(gestureOffset, kind: PointerDeviceKind.mouse, buttons: kSecondaryMouseButton);
final TestGesture gesture = await tester.startGesture(textOffsetToPosition(paragraph, 125), kind: PointerDeviceKind.mouse, buttons: kSecondaryMouseButton);
addTearDown(gesture.removePointer);
await tester.pump();
await gesture.up();
Expand Down

0 comments on commit a6141f6

Please sign in to comment.