Skip to content

Conversation

@koji-1009
Copy link
Contributor

@koji-1009 koji-1009 commented Jul 24, 2024

Changed the cursor position to be the same as before flutter 3.22.0 when 17 character codes in the Unicode Zs category are entered into a TextField.

Extend the support for #149698. As a result, #149099 is resolved.

The code for the Unicode-Zs category is based on the following page.
https://www.compart.com/en/unicode/category/Zs

Fixes #149099

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@github-actions github-actions bot added a: text input Entering text in a text field or keyboard related problems framework flutter/packages/flutter repository. See also f: labels. labels Jul 24, 2024
@koji-1009 koji-1009 changed the title Fix cursor position when Unicode Zs category is entered in TextField. Fix cursor position when Unicode Zs category is entered in TextField Jul 24, 2024
@koji-1009 koji-1009 marked this pull request as ready for review July 24, 2024 09:34
0x2009 || // thin space
0x200A || // hair space
0x202F || // narrow no-break space(NNBSP)
0x205F || // medium mathematical space(MMSP)
Copy link
Contributor

@LongCatIsLooong LongCatIsLooong Jul 24, 2024

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Also you could use regex for matching unicode properties, e.g.: RegExp(r'\p{Space_Separator}', unicode: true)

Copy link
Contributor

@LongCatIsLooong LongCatIsLooong left a comment

Choose a reason for hiding this comment

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

It's a bit unfortunate that we have to maintain this to match SkParagraph's definition of trailing spaces. The less hacky solution is exposing the line width (including trailing spaces) from SkParagraph, but that's probably going to take more effort.

0x2009 || // thin space
0x200A || // hair space
0x202F || // narrow no-break space(NNBSP)
0x205F || // medium mathematical space(MMSP)
Copy link
Contributor

Choose a reason for hiding this comment

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

Also you could use regex for matching unicode properties, e.g.: RegExp(r'\p{Space_Separator}', unicode: true)

const String textWithFullWidthSpace = 'A\u{3000}';
checkCaretOffsetsLtr(textWithFullWidthSpace);
painter.text = const TextSpan(text: textWithFullWidthSpace);
text = 'A';
Copy link
Contributor

Choose a reason for hiding this comment

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

These 3 lines are no longer needed it seems?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I see you needed A's horizontal advance. It's 14.0.

Copy link
Contributor Author

@koji-1009 koji-1009 Jul 25, 2024

Choose a reason for hiding this comment

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

Yes. I checked the value at runtime and found it to be 14.0, but since it depends on the style of the painter, I calculated it.

caretOffset = painter.getOffsetForCaret(const ui.TextPosition(offset: textWithFullWidthSpace.length), ui.Rect.zero);
expect(caretOffset.dx, painter.width);
final double textAWidth = painter.width;
void checkUnicodeZsCategory(String charset) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: charset -> character

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I fixed at 1e1d000.

checkCaretOffsetsLtr(text);
painter.text = TextSpan(text: text);
painter.layout();
caretOffset = painter.getOffsetForCaret(const ui.TextPosition(offset: 0), ui.Rect.zero);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you also verify that each input character is indeed a space, by laying out the painter at maxWidth = 14, and checks if there's only one line, and the line width is exactly 14? If one of the characters is not considered a trailing space then there will be 2 lines (or lineWidth would be greater than 14 if the character is not a line breaker).

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 added test case at 9c37308.

expect(caretOffset.dx, painter.width);

painter.layout(maxWidth: textAWidth);
expect(painter.width, textAWidth);
Copy link
Contributor

Choose a reason for hiding this comment

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

painter.width is not the line width. I think this should be:

final lines = painter.computeLineMetrics();
expect(lines.length, 1);
expect(lines.width, 14.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 added test case at 86adcc1.

@koji-1009 koji-1009 force-pushed the feat/unicode_zs_category branch from 6dc687e to 86adcc1 Compare July 25, 2024 07:22
@koji-1009 koji-1009 force-pushed the feat/unicode_zs_category branch 2 times, most recently from d374f83 to 080583e Compare July 25, 2024 08:56
@koji-1009 koji-1009 force-pushed the feat/unicode_zs_category branch from 9d1f2d7 to 039c5e9 Compare July 25, 2024 13:56
Copy link
Contributor

@LongCatIsLooong LongCatIsLooong left a comment

Choose a reason for hiding this comment

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

Some of these space characters are actually not considered "space" by SkParagraph (see this comment)

I think you may want to exclude the characters that are not included in u_isWhitespace (or the test will fail), and also exclude the characters that would force a line break (so basically you don't have to include Zl and Zp).

If the web test keeps failing (looks like it will) and other tests are passing then I think we should add , skip: isBrowser && !isSkiaWeb to the test.


painter.layout(maxWidth: 14.0);
final List<ui.LineMetrics> lines = painter.computeLineMetrics();
// If charactor is non-breaking space, the height should be 2 lines. Otherwise, it should be 1 line.
Copy link
Contributor

Choose a reason for hiding this comment

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

All of these should be 1-line. If it is not then the character is not considered a space (during line-breaking) by SkParagraph, so the character must be excluded from the list.

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: "character"

0x3000 || // ideographic space
0x20 => true, // space
_ => false,
final String lastCharactor = rawString[rawString.length - 1];
Copy link
Contributor

@LongCatIsLooong LongCatIsLooong Jul 25, 2024

Choose a reason for hiding this comment

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

This is still checking the last code unit. I don't see the difference between this and the expression before the change?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also I'm curious how the regexp fares with surrogate pairs, could you also test a surrogate pair ("\uD801\uDC37" for example)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I tested locally and it didn't throw. So up to you if you want to skip adding that test.

0x20 => true, // space
_ => false,
final String lastCharactor = rawString[rawString.length - 1];
final bool hasTrailingSpaces = switch (lastCharactor.codeUnitAt(0)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

There's little point in using a switch expression now I think? This could just be an ||.

caretOffset = painter.getOffsetForCaret(ui.TextPosition(offset: text.length), ui.Rect.zero);
expect(caretOffset.dx, painter.width);

void checkUnicodeZsCategory(String character, int expectLines) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Readability nit: it's not immediately obvious what this is checking. This is verifying that:

  1. character is indeed considered a trailing space by SkParagraph
  2. TextPainter's implementation matches the SkParagraph implementation

So consider renaming this function to something like "verifyCharacterIsConsideredTrailingSpaceBySkParagraphAndTextPainter" (or turn the "by skparagraph and textpainter" part to a comment since the name is a bit long).

@LongCatIsLooong
Copy link
Contributor

re:

If the web test keeps failing (looks like it will) and other tests are passing then I think we should add , skip: isBrowser && !isSkiaWeb to the test.

That is because the HTML renderer (isBrowser && !isSkiaWeb) uses a different text layout library, while everything else uses SkParagraph under the hood.

@koji-1009
Copy link
Contributor Author

@LongCatIsLooong
Thanks for your patient review. Let me ask you one question.

All of these should be 1-line. If it is not then the character is not considered a space (during line-breaking) by SkParagraph, so the character must be excluded from the list.

Is there a problem with the current implementation where computeLineMetrics returns 2 lines when u{00A0} is set? Running the following test code against the flutter:master branch will fail.

void verifyCharacterIsConsideredTrailingSpaceBySkParagraphAndTextPainter(String character) {
  final String reason = 'character: ${character.codeUnitAt(0).toRadixString(16)}';

  text = 'A$character';
  checkCaretOffsetsLtr(text);
  painter.text = TextSpan(text: text);
  painter.layout();
  caretOffset = painter.getOffsetForCaret(const ui.TextPosition(offset: 0), ui.Rect.zero);
  expect(caretOffset.dx, 0.0, reason: reason);
  caretOffset = painter.getOffsetForCaret(const ui.TextPosition(offset: 1), ui.Rect.zero);
  expect(caretOffset.dx, 14.0, reason: reason);
  caretOffset = painter.getOffsetForCaret(ui.TextPosition(offset: text.length), ui.Rect.zero);
  expect(caretOffset.dx, painter.width, reason: reason);

  painter.layout(maxWidth: 14.0);
  final List<ui.LineMetrics> lines = painter.computeLineMetrics();
  expect(lines.length, 1, reason: reason);
  expect(lines.first.width, 14.0, reason: reason);
}

// Test with trailing space
verifyCharacterIsConsideredTrailingSpaceBySkParagraphAndTextPainter('\u{0020}');

// Test with trailing no-break space
verifyCharacterIsConsideredTrailingSpaceBySkParagraphAndTextPainter('\u{00A0}');

// Test with trailing full-width space
verifyCharacterIsConsideredTrailingSpaceBySkParagraphAndTextPainter('\u{3000}');

@LongCatIsLooong
Copy link
Contributor

LongCatIsLooong commented Jul 26, 2024

@LongCatIsLooong Thanks for your patient review. Let me ask you one question.

All of these should be 1-line. If it is not then the character is not considered a space (during line-breaking) by SkParagraph, so the character must be excluded from the list.

Is there a problem with the current implementation where computeLineMetrics returns 2 lines when u{00A0} is set? Running the following test code against the flutter:master branch will fail.

I think that's because U+00A0 is not considered a whitespace character by skparagraph. see #152215 (comment)
(also this comment: #152215 (review)).

The full list of whitespace characters that skparagraph recognizes is here.

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 28, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 28, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 29, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 29, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 29, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 29, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 29, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 29, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 30, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 30, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 30, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 30, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 31, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 31, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 31, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 31, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 31, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 31, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 31, 2024
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Aug 1, 2024
Manual roll requested by [email protected]

flutter/flutter@031dc3d...4d12197

2024-07-26 [email protected] further shard Mac tool_integration_tests from 4 to 5 shards (flutter/flutter#152399)
2024-07-26 [email protected] Change flutter_build_with_compilation_error_test to check stdout or stderr (flutter/flutter#152404)
2024-07-26 [email protected] [cupertino/icons.dart] Replace ligature references with characters corresponding to codepoints (flutter/flutter#152387)
2024-07-26 [email protected] Update minimum macOS version as needed in Swift package (flutter/flutter#152347)
2024-07-26 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 3.25.14 to 3.25.15 (flutter/flutter#152401)
2024-07-26 49699333+dependabot[bot]@users.noreply.github.com Bump ossf/scorecard-action from 2.3.3 to 2.4.0 (flutter/flutter#152400)
2024-07-26 [email protected] Update text_painter.dart (flutter/flutter#152398)
2024-07-26 [email protected] Fix some tests that fail with Swift Package Manager enabled (flutter/flutter#152267)
2024-07-26 [email protected] Reland "Launch DDS from Dart SDK and prepare to serve DevTools from DDS (#146593)" (flutter/flutter#152386)
2024-07-26 [email protected] Make `DragGestureRecognizer` abstract methods public (flutter/flutter#151627)
2024-07-26 [email protected] Fix cursor position when Unicode Zs category is entered in TextField (flutter/flutter#152215)
2024-07-26 [email protected] Roll Flutter Engine from 354abf2800a0 to e28f8755e25b (2 revisions) (flutter/flutter#152388)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages
Please CC [email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
TytaniumDev pushed a commit to TytaniumDev/flutter that referenced this pull request Aug 7, 2024
…lutter#152215)

Changed the cursor position to be the same as before flutter 3.22.0 when 17 character codes in the Unicode Zs category are entered into a TextField.

Extend the support for flutter#149698. As a result, flutter#149099 is resolved.

The code for the Unicode-Zs category is based on the following page.
https://www.compart.com/en/unicode/category/Zs

Fixes flutter#149099
Buchimi pushed a commit to Buchimi/flutter that referenced this pull request Sep 2, 2024
…lutter#152215)

Changed the cursor position to be the same as before flutter 3.22.0 when 17 character codes in the Unicode Zs category are entered into a TextField.

Extend the support for flutter#149698. As a result, flutter#149099 is resolved.

The code for the Unicode-Zs category is based on the following page.
https://www.compart.com/en/unicode/category/Zs

Fixes flutter#149099
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a: text input Entering text in a text field or keyboard related problems autosubmit Merge PR when tree becomes green via auto submit App framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Cursor does not move on entering full-width space in TextField (3.22.0)

4 participants