-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Fix cursor position when Unicode Zs category is entered in TextField #152215
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
Fix cursor position when Unicode Zs category is entered in TextField #152215
Conversation
| 0x2009 || // thin space | ||
| 0x200A || // hair space | ||
| 0x202F || // narrow no-break space(NNBSP) | ||
| 0x205F || // medium mathematical space(MMSP) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FTR, this is how SkParagraph determines whether if a cluster is a space (or break).
https://github.com/google/skia/blob/8b2f189cfa59d0c69b700bbe8587f52f5bd2ad5c/modules/skparagraph/src/ParagraphImpl.cpp#L357-L381
With the ICU4C backend, ::kPartOfWhiteSpaceBreak is
https://github.com/unicode-org/icu/blob/23d9628f88a2d0127c564ad98297061c36d3ce77/icu4c/source/common/unicode/uchar.h#L3388-L3425, so this list should probably be a bit different from the Zs category.
There was a problem hiding this comment.
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)
LongCatIsLooong
left a comment
There was a problem hiding this 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) |
There was a problem hiding this comment.
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'; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: charset -> character
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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);There was a problem hiding this comment.
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.
6dc687e to
86adcc1
Compare
d374f83 to
080583e
Compare
9d1f2d7 to
039c5e9
Compare
LongCatIsLooong
left a comment
There was a problem hiding this 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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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]; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
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)) { |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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:
characteris indeed considered a trailing space by SkParagraph- 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).
|
re:
That is because the HTML renderer ( |
|
@LongCatIsLooong
Is there a problem with the current implementation where 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}'); |
I think that's because The full list of whitespace characters that skparagraph recognizes is here. |
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
…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
…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
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.