这是indexloc提供的服务,不要输入任何密码
Skip to content

RenderParagraphs _SelectableFragment.boundingBoxes should consider max line height #155892

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Sep 30, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions packages/flutter/lib/src/rendering/paragraph.dart
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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add @visibleForTest?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RenderEditable has this API public, I don't have a strong preference either way, what do you think?

Copy link
Contributor

@chunhtai chunhtai Sep 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will add it just to be conservative, we can always move to public in the future, but not backward.

and if someone does need it, we can then ask for use case and figure out whether this is really appropriate

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me!


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
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);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I borrowed this from

return endpoints[0].point + const Offset(kIsWeb? 1.0 : 0.0, -2.0);
, I couldn't really track down why this change was needed but it originally came from #4397

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, well I guess we should keep it around 🤷 .

}

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