Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Paragraph.GetWordBoundary respects text affinity #36176

Merged
auto-submit merged 4 commits into from
Sep 19, 2022
Merged

Paragraph.GetWordBoundary respects text affinity #36176

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
734d1b0
Paragraph.GetWordBoundary respects text affinity
chunhtai Sep 15, 2022
ff0a5d1
addressing comment
chunhtai Sep 16, 2022
12af947
Update lib/ui/text.dart
chunhtai Sep 19, 2022
57af725
Update lib/ui/text.dart
chunhtai Sep 19, 2022
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
28 changes: 21 additions & 7 deletions lib/ui/text.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2238,13 +2238,12 @@ enum TextAffinity {

/// A position in a string of text.
///
/// A TextPosition can be used to locate a position in a string in code (using
/// the [offset] property), and it can also be used to locate the same position
/// visually in a rendered string of text (using [offset] and, when needed to
/// resolve ambiguity, [affinity]).
/// A TextPosition can be used to describe a caret position in between
/// characters. The [offset] points to the position between `offset - 1` and
/// `offset` characters of the string, and the [affinity] is used to describe
/// which character this position affiliates with.
///
/// The location of an offset in a rendered string is ambiguous in two cases.
/// One happens when rendered text is forced to wrap. In this case, the offset
/// One use case is when rendered text is forced to wrap. In this case, the offset
/// where the wrap occurs could visually appear either at the end of the first
/// line or the beginning of the second line. The second way is with
/// bidirectional text. An offset at the interface between two different text
Expand Down Expand Up @@ -2829,8 +2828,23 @@ class Paragraph extends NativeFieldWrapperClass1 {
/// have word breaks on both sides. In such cases, this method will return
/// [offset, offset+1]. Word boundaries are defined more precisely in Unicode
/// Standard Annex #29 http://www.unicode.org/reports/tr29/#Word_Boundaries
///
/// The [TextPosition] is treated as caret position, its [TextPosition.affinity]
/// is used to determine which character this position points to. For example,
/// the word boundary at `TextPosition(offset: 5, affinity: TextPosition.upstream)`
/// of the `string = 'Hello word'` will return range (0, 5) because the position
/// points to the character 'o' instead of the space.
TextRange getWordBoundary(TextPosition position) {
final List<int> boundary = _getWordBoundary(position.offset);
final int characterPosition;
Copy link
Contributor

@LongCatIsLooong LongCatIsLooong Sep 15, 2022

Choose a reason for hiding this comment

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

TextPosition's documentation says

/// A TextPosition can be used to locate a position in a string in code (using
/// the [offset] property), and it can also be used to locate the same position
/// visually in a rendered string of text (using [offset] and, when needed to
/// resolve ambiguity, [affinity]).

so that seems to indicate that the same TextPosition can either be interpreted as an int which points to a code unit in a UTF16 string, or a place to draw the caret in the rendered string.

This works but it should be stated in the documentation that TextPosition here is a caret location instead of an offset (but I feel it's easier to reason about word boundaries using code unit offsets instead of using caret locations). I think i'd prefer we use TextPosition exclusively for caret positions, and let getWordBoundary take an int instead of TextPosition to avoid ambiguity.

Copy link
Contributor

@LongCatIsLooong LongCatIsLooong Sep 15, 2022

Choose a reason for hiding this comment

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

Looks like it was initially an int and was changed in https://github.com/flutter/engine/pull/13765/files. @gspencergoog is there a reason for using TextPosition over int here?

Copy link
Contributor

@yjbanov yjbanov Sep 15, 2022

Choose a reason for hiding this comment

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

I feel like some of the text in this thread should become code comments for future reference.

Copy link
Contributor

@gspencergoog gspencergoog Sep 15, 2022

Choose a reason for hiding this comment

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

It was three years ago, so it's hazy, but I think it had to do with being able to move line by line. Here's some additional context: #13727

Copy link
Contributor Author

@chunhtai chunhtai Sep 15, 2022

Choose a reason for hiding this comment

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

I looked at the previous pr it looks like it just part of moving TextRange and related class to ui to fix some line boundary issue, and the getWordBoundary just some how refactor to matches the API.

Copy link
Contributor Author

@chunhtai chunhtai Sep 15, 2022

Choose a reason for hiding this comment

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

I think it is really down to when we want to make the switch from caret position to code unit position. In my opinion it should be as late as possible in case the a later step may want to use the caret position and they have to do some other hack to restore it back. WDYT @LongCatIsLooong

I think we should always treat TextPosition as a caret position, and use int if it meant to be a code unit position. Otherwise, it will create convoluted code. I will update the document

Copy link
Contributor

@LongCatIsLooong LongCatIsLooong Sep 15, 2022

Choose a reason for hiding this comment

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

#28008 made getLineBoundary TextAffinity aware. I'm ok with making them consistent and document that the parameter is based on the caret location instead of the character location. But still word boundaries generally don't require layout and the caller may just want to tokenize the text without even putting the text on screen. Talking about "caret position" could be confusing in this context.

Copy link
Contributor Author

@chunhtai chunhtai Sep 15, 2022

Choose a reason for hiding this comment

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

the input may come from a getPositionForOffset from a touch event, which is affinity aware. I will keep it as is and document it. Thanks for the input.

switch (position.affinity) {
case TextAffinity.upstream:
characterPosition = position.offset - 1;
break;
case TextAffinity.downstream:
characterPosition = position.offset;
break;
}
final List<int> boundary = _getWordBoundary(characterPosition);
return TextRange(start: boundary[0], end: boundary[1]);
}

Expand Down
11 changes: 10 additions & 1 deletion lib/web_ui/lib/src/engine/canvaskit/text.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -775,7 +775,16 @@ class CkParagraph extends SkiaObject<SkParagraph> implements ui.Paragraph {
@override
ui.TextRange getWordBoundary(ui.TextPosition position) {
final SkParagraph paragraph = _ensureInitialized(_lastLayoutConstraints!);
final SkTextRange skRange = paragraph.getWordBoundary(position.offset);
final int characterPosition;
switch (position.affinity) {
case ui.TextAffinity.upstream:
characterPosition = position.offset - 1;
break;
case ui.TextAffinity.downstream:
characterPosition = position.offset;
break;
}
final SkTextRange skRange = paragraph.getWordBoundary(characterPosition);
return ui.TextRange(start: skRange.start, end: skRange.end);
}

Expand Down
16 changes: 16 additions & 0 deletions lib/web_ui/test/canvaskit/canvaskit_api_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1342,6 +1342,22 @@ void _canvasTests() {
);
});

test('Paragraph converts caret position to charactor position', () {
final CkParagraphBuilder builder = CkParagraphBuilder(
CkParagraphStyle(),
);
builder.addText('Hello there');
final CkParagraph paragraph = builder.build();
paragraph.layout(const ui.ParagraphConstraints(width: 100));
ui.TextRange range = paragraph.getWordBoundary(const ui.TextPosition(offset: 5, affinity: ui.TextAffinity.upstream));
expect(range.start, 0);
expect(range.end, 5);

range = paragraph.getWordBoundary(const ui.TextPosition(offset: 5));
expect(range.start, 5);
expect(range.end, 6);
});

test('Paragraph dispose', () {
final CkParagraphBuilder builder = CkParagraphBuilder(
CkParagraphStyle(),
Expand Down
24 changes: 24 additions & 0 deletions testing/dart/text_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,6 +288,29 @@ void testFontVariation() {
});
}

void testGetWordBoundary() {
test('GetWordBoundary', () async {
final Uint8List fontData = await readFile('RobotoSlab-VariableFont_wght.ttf');
await loadFontFromList(fontData, fontFamily: 'RobotoSerif');

final ParagraphBuilder builder = ParagraphBuilder(ParagraphStyle(
fontFamily: 'RobotoSerif',
fontSize: 40.0,
));
builder.addText('Hello team');
final Paragraph paragraph = builder.build();
paragraph.layout(const ParagraphConstraints(width: double.infinity));

TextRange range = paragraph.getWordBoundary(const TextPosition(offset: 5, affinity: TextAffinity.upstream));
expect(range.start, 0);
expect(range.end, 5);

range = paragraph.getWordBoundary(const TextPosition(offset: 5));
expect(range.start, 5);
expect(range.end, 6);
});
}

void main() {
testFontWeight();
testParagraphStyle();
Expand All @@ -297,4 +320,5 @@ void main() {
testLoadFontFromList();
testFontFeatureClass();
testFontVariation();
testGetWordBoundary();
}