Unverified Commit 3538e4c7 authored by LongCatIsLooong's avatar LongCatIsLooong Committed by GitHub

Changing `TextPainter.getOffsetForCaret` implementation to remove the logarithmic search (#143281)

The behavior largely remains the same, except:

1. The EOT cursor `(textLength, downstream)` for text ending in the opposite writing direction as the paragraph is now placed at the visual end of the last line. 
  For example, in a LTR paragraph, the EOT cursor for `aA` (lowercase for LTR and uppercase for RTL) is placed to the right of the line: `aA|` (it was `a|A` before). 
  This matches the behavior of most applications that do logical order arrow key navigation instead of visual order navigation. 
  And it makes the navigation order consistent for `aA\naA`:
```
  |aA    =>  aA|  => aA|  => aA  => aA   => aA 
   aA        aA      aA     |aA     aA|     aA|     
   (1)       (2)     (3)    (4)    (5)      (6)
```
This is indeed still pretty confusing as (2) and (3), as well as (5) and (6) are hard to distinguish (when the I beam has a large width they are actually visually distinguishable -- they use the same anchor but one gets painted to the left and the other to the right. I noticed that emacs does the same). 
But logical order navigation will always be confusing in bidi text, in one way or another.

Interestingly there are 3 different behaviors I've observed in chrome:
- the chrome download dialog (which I think uses GTK text widgets but not sure which version) gives me 2 cursors when navigating bidi text, and 
- its HTML fields only show one, and presumably they place the I beam at the **trailing edge** of the character (which makes more sense for backspacing I guess). 
- On the other hand, its (new) omnibar seems to use visual order arrow navigation

Side note: we may need to update the "tap to place the caret here" logic to handle the case where the tap lands outside of the text and the text ends in the opposite writing direction. 

2. Removed the logarithmic search. The same could be done using the characters package but when glyphInfo tells you about the baseline location in the future we probably don't need the `getBoxesForRange` call. This should fix https://github.com/flutter/flutter/issues/123424.

## Internal Tests

This is going to change the image output of some internal golden tests. I'm planning to merge https://github.com/flutter/flutter/pull/143281 before this to avoid updating the same golden files twice for invalid selections.
parent 3ae71f5c
...@@ -46,8 +46,9 @@ void main() { ...@@ -46,8 +46,9 @@ void main() {
const Duration durationBetweenActions = Duration(milliseconds: 20); const Duration durationBetweenActions = Duration(milliseconds: 20);
const String defaultText = 'I am a magnifier, fear me!'; const String defaultText = 'I am a magnifier, fear me!';
Future<void> showMagnifier(WidgetTester tester, String characterToTapOn) async { Future<void> showMagnifier(WidgetTester tester, int textOffset) async {
final Offset tapOffset = _textOffsetToPosition(tester, defaultText.indexOf(characterToTapOn)); assert(textOffset >= 0);
final Offset tapOffset = _textOffsetToPosition(tester, textOffset);
// Double tap 'Magnifier' word to show the selection handles. // Double tap 'Magnifier' word to show the selection handles.
final TestGesture testGesture = await tester.startGesture(tapOffset); final TestGesture testGesture = await tester.startGesture(tapOffset);
...@@ -59,11 +60,11 @@ void main() { ...@@ -59,11 +60,11 @@ void main() {
await testGesture.up(); await testGesture.up();
await tester.pumpAndSettle(); await tester.pumpAndSettle();
final TextSelection selection = tester final TextEditingController controller = tester
.firstWidget<TextField>(find.byType(TextField)) .firstWidget<TextField>(find.byType(TextField))
.controller! .controller!;
.selection;
final TextSelection selection = controller.selection;
final RenderEditable renderEditable = _findRenderEditable(tester); final RenderEditable renderEditable = _findRenderEditable(tester);
final List<TextSelectionPoint> endpoints = _globalize( final List<TextSelectionPoint> endpoints = _globalize(
renderEditable.getEndpointsForSelection(selection), renderEditable.getEndpointsForSelection(selection),
...@@ -86,7 +87,7 @@ void main() { ...@@ -86,7 +87,7 @@ void main() {
testWidgets('should show custom magnifier on drag', (WidgetTester tester) async { testWidgets('should show custom magnifier on drag', (WidgetTester tester) async {
await tester.pumpWidget(const example.TextMagnifierExampleApp(text: defaultText)); await tester.pumpWidget(const example.TextMagnifierExampleApp(text: defaultText));
await showMagnifier(tester, 'e'); await showMagnifier(tester, defaultText.indexOf('e'));
expect(find.byType(example.CustomMagnifier), findsOneWidget); expect(find.byType(example.CustomMagnifier), findsOneWidget);
await expectLater( await expectLater(
...@@ -96,16 +97,15 @@ void main() { ...@@ -96,16 +97,15 @@ void main() {
}, variant: const TargetPlatformVariant(<TargetPlatform>{ TargetPlatform.iOS, TargetPlatform.android })); }, variant: const TargetPlatformVariant(<TargetPlatform>{ TargetPlatform.iOS, TargetPlatform.android }));
for (final TextDirection textDirection in TextDirection.values) { testWidgets('should show custom magnifier in RTL', (WidgetTester tester) async {
testWidgets('should show custom magnifier in $textDirection', (WidgetTester tester) async { const String text = 'أثارت زر';
final String text = textDirection == TextDirection.rtl ? 'أثارت زر' : defaultText; const String textToTapOn = 'ت';
final String textToTapOn = textDirection == TextDirection.rtl ? 'ت' : 'e';
await tester.pumpWidget(example.TextMagnifierExampleApp(textDirection: textDirection, text: text)); await tester.pumpWidget(const example.TextMagnifierExampleApp(textDirection: TextDirection.rtl, text: text));
await showMagnifier(tester, textToTapOn); await showMagnifier(tester, text.indexOf(textToTapOn));
expect(find.byType(example.CustomMagnifier), findsOneWidget);
});
expect(find.byType(example.CustomMagnifier), findsOneWidget);
});
}
} }
...@@ -6941,7 +6941,7 @@ void main() { ...@@ -6941,7 +6941,7 @@ void main() {
// the arrow should not point exactly to the caret because the caret is // the arrow should not point exactly to the caret because the caret is
// too close to the right. // too close to the right.
controller.dispose(); controller.dispose();
controller = TextEditingController(text: List<String>.filled(200, 'a').join()); controller = TextEditingController(text: 'a' * 200);
await tester.pumpWidget( await tester.pumpWidget(
CupertinoApp( CupertinoApp(
debugShowCheckedModeBanner: false, debugShowCheckedModeBanner: false,
...@@ -7002,7 +7002,7 @@ void main() { ...@@ -7002,7 +7002,7 @@ void main() {
// Normal centered collapsed selection. The toolbar arrow should point down, and // Normal centered collapsed selection. The toolbar arrow should point down, and
// it should point exactly to the caret. // it should point exactly to the caret.
controller.dispose(); controller.dispose();
controller = TextEditingController(text: List<String>.filled(200, 'a').join()); controller = TextEditingController(text: 'a' * 200);
addTearDown(controller.dispose); addTearDown(controller.dispose);
await tester.pumpWidget( await tester.pumpWidget(
CupertinoApp( CupertinoApp(
......
...@@ -15222,6 +15222,8 @@ void main() { ...@@ -15222,6 +15222,8 @@ void main() {
bool isWide = false; bool isWide = false;
const double wideWidth = 300.0; const double wideWidth = 300.0;
const double narrowWidth = 200.0; const double narrowWidth = 200.0;
const TextStyle style = TextStyle(fontSize: 10, height: 1.0, letterSpacing: 0.0, wordSpacing: 0.0);
const double caretWidth = 2.0;
final TextEditingController controller = _textEditingController(); final TextEditingController controller = _textEditingController();
await tester.pumpWidget( await tester.pumpWidget(
boilerplate( boilerplate(
...@@ -15234,6 +15236,7 @@ void main() { ...@@ -15234,6 +15236,7 @@ void main() {
key: textFieldKey, key: textFieldKey,
controller: controller, controller: controller,
textDirection: TextDirection.rtl, textDirection: TextDirection.rtl,
style: style,
), ),
); );
}, },
...@@ -15250,15 +15253,17 @@ void main() { ...@@ -15250,15 +15253,17 @@ void main() {
expect(inputWidth, narrowWidth); expect(inputWidth, narrowWidth);
expect(cursorRight, inputWidth - kCaretGap); expect(cursorRight, inputWidth - kCaretGap);
// After entering some text, the cursor remains on the right of the input. const String text = '12345';
await tester.enterText(find.byType(TextField), '12345'); // After entering some text, the cursor is placed to the left of the text
// because the paragraph's writing direction is RTL.
await tester.enterText(find.byType(TextField), text);
await tester.pump(); await tester.pump();
editable = findRenderEditable(tester); editable = findRenderEditable(tester);
cursorRight = editable.getLocalRectForCaret( cursorRight = editable.getLocalRectForCaret(
TextPosition(offset: controller.value.text.length), TextPosition(offset: controller.value.text.length),
).topRight.dx; ).topRight.dx;
inputWidth = editable.size.width; inputWidth = editable.size.width;
expect(cursorRight, inputWidth - kCaretGap); expect(cursorRight, inputWidth - kCaretGap - text.length * 10 - caretWidth);
// Since increasing the width of the input moves its right edge further to // Since increasing the width of the input moves its right edge further to
// the right, the cursor has followed this change and still appears on the // the right, the cursor has followed this change and still appears on the
...@@ -15273,7 +15278,7 @@ void main() { ...@@ -15273,7 +15278,7 @@ void main() {
).topRight.dx; ).topRight.dx;
inputWidth = editable.size.width; inputWidth = editable.size.width;
expect(inputWidth, wideWidth); expect(inputWidth, wideWidth);
expect(cursorRight, inputWidth - kCaretGap); expect(cursorRight, inputWidth - kCaretGap - text.length * 10 - caretWidth);
}); });
testWidgets('Text selection menu hides after select all on desktop', (WidgetTester tester) async { testWidgets('Text selection menu hides after select all on desktop', (WidgetTester tester) async {
......
...@@ -301,9 +301,9 @@ void main() { ...@@ -301,9 +301,9 @@ void main() {
painter.getOffsetForCaret(const TextPosition(offset: 2, affinity: TextAffinity.upstream), Rect.zero), painter.getOffsetForCaret(const TextPosition(offset: 2, affinity: TextAffinity.upstream), Rect.zero),
const Offset(0.0, 10.0), const Offset(0.0, 10.0),
); );
expect( // after the Alef expect( // To the right of the Alef
painter.getOffsetForCaret(const TextPosition(offset: 2), Rect.zero), painter.getOffsetForCaret(const TextPosition(offset: 2), Rect.zero),
const Offset(0.0, 10.0), const Offset(10.0, 10.0),
); );
expect( expect(
......
...@@ -976,7 +976,7 @@ void main() { ...@@ -976,7 +976,7 @@ void main() {
expect(find.byType(EditableText), paints expect(find.byType(EditableText), paints
..rrect( ..rrect(
rrect: RRect.fromRectAndRadius( rrect: RRect.fromRectAndRadius(
const Rect.fromLTRB(193.83334350585938, -0.916666666666668, 196.83334350585938, 19.083333969116211), const Rect.fromLTWH(193.83334350585938, -0.916666666666668, 3.0, 20.0),
const Radius.circular(1.0), const Radius.circular(1.0),
), ),
color: const Color(0xbf2196f3), color: const Color(0xbf2196f3),
...@@ -994,7 +994,7 @@ void main() { ...@@ -994,7 +994,7 @@ void main() {
expect(find.byType(EditableText), paints expect(find.byType(EditableText), paints
..rrect( ..rrect(
rrect: RRect.fromRectAndRadius( rrect: RRect.fromRectAndRadius(
const Rect.fromLTRB(719.3333333333333, -0.9166666666666679, 721.3333333333333, 17.083333333333332), const Rect.fromLTWH(719.3333333333333, -0.9166666666666679, 2.0, 18.0),
const Radius.circular(2.0), const Radius.circular(2.0),
), ),
color: const Color(0xff999999), color: const Color(0xff999999),
......
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