Update hasTrailingSpaces by ttorii20 · Pull Request #149698 · flutter/flutter

ttorii20 · 2024-06-05T00:30:44Z

This PR addresses an issue with TextPainter's caret position calculation for text containing full-width spaces. Currently, the caret position is not accurately calculated for strings with full-width spaces. To resolve this, the following changes have been made:

Corrected the logic for caret position calculation when full-width spaces are present in the text.
Added and updated test cases to ensure accurate caret position calculation.
These changes ensure that the caret position for text with full-width spaces is computed correctly.

This issue was introduced by the commit a0a854a.

Related Issue: #149099

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is test-exempt.
I followed the breaking change policy and added Data Driven Fixes where supported.
All existing and new tests are passing.

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

LongCatIsLooong

LGTM.

The whole trailing space check thing is a hack and we should expose the width of a line with trailing spaces instead. But that will be a riskier fix to cherry-pick to the beta / stable channel (if you intend to do so).

LongCatIsLooong · 2024-06-07T17:25:48Z

packages/flutter/lib/src/painting/text_painter.dart

@@ -356,6 +356,7 @@ class _TextLayout {
    // last logical trailing space.


I looked at the SkParagraph code and it looks like the statement here about it treating only " " and "\t" as spaces is not true. It's using code unit properties. Could you update or delete that first paragraph?

LongCatIsLooong · 2024-06-07T17:37:28Z

packages/flutter/test/painting/text_painter_test.dart

+      final TextPainter painter = TextPainter()
+        ..textDirection = TextDirection.ltr;
+
+      const String text = 'A　';


nit: could you use \u{3000} to make it more obvious (or add a comment) that there is a trailing fullwidth space after A?

LongCatIsLooong · 2024-06-07T17:43:47Z

packages/flutter/test/painting/text_painter_test.dart

      painter.dispose();
    });

+    test('TextPainter caret test with trailing full-width space', () {


nit: move this to the "caret" test group?

justinmc

LGTM 👍

But seconding the nits left by @LongCatIsLooong .

justinmc · 2024-06-07T18:52:49Z

packages/flutter/test/painting/text_painter_test.dart

+      painter.dispose();
+    });
+
+


Nit: Too many newlines here, there should only be one.

…b.com/ttorii20/flutter into fix/cursor-alignment-fullwidth-spaces

ttorii20 · 2024-06-08T02:22:13Z

Hi @LongCatIsLooong @justinmc

Thank you both for your review!

I have completed the requested changes:

Updated the SkParagraph white space handling comments.
Merged the trailing full-width space test into the existing caret test.
Please review the latest commit.

LongCatIsLooong

Could you delete the sentence that says "SkParagraph currently treats " ", "\t", and "\u{3000}" as white spaces."? (since it's still a bit misleading as SkParagraph treats a wider range of codepoints as spaces when doing line breaking).

LongCatIsLooong · 2024-06-08T02:32:09Z

Also if you would like to cherry-pick the commit (after the PR is merged), feel free to create a CP request using the automated workflow.

…b.com/ttorii20/flutter into fix/cursor-alignment-fullwidth-spaces

LongCatIsLooong · 2024-06-08T02:43:08Z

LGTM thank you for the fix!

ttorii20 · 2024-06-08T02:44:54Z

LGTM thank you for the fix!

Thank you for the review!

auto-submit · 2024-06-08T03:41:26Z

auto label is removed for flutter/flutter/149698, due to - The status or check suite Linux web_long_running_tests_1_5 has failed. Please fix the issues identified (or deflake) before re-applying this label.

flutter/flutter@32081aa...14df7be 2024-06-11 [email protected] Revert "Add tests for scaffold drawer and end drawer" (flutter/flutter#150045) 2024-06-11 [email protected] Add tests for scaffold drawer and end drawer (flutter/flutter#149383) 2024-06-11 [email protected] Add high-contrast theme (flutter/flutter#149779) 2024-06-11 [email protected] Manual Pub Roll (flutter/flutter#150025) 2024-06-10 [email protected] [docs] Per-platform desktop triage instructions (flutter/flutter#150019) 2024-06-10 [email protected] Fix copy-paste-o in MethodChannel.invokeListMethod doc (flutter/flutter#149976) 2024-06-10 [email protected] Unpin `camera_android` and remove its only usage (flutter/flutter#150017) 2024-06-10 [email protected] Fixes a bug where NavigatorState.pop does not consider any possible sâ�¦ (flutter/flutter#150014) 2024-06-10 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Reland: [CupertinoActionSheet] Match colors to native (#149568) (#150015)" (flutter/flutter#150021) 2024-06-10 [email protected] Reland: [CupertinoActionSheet] Match colors to native (#149568) (flutter/flutter#150015) 2024-06-10 [email protected] Temporarily run Mac_arm64 framework_tests_misc on only Mac-13 (flutter/flutter#150009) 2024-06-10 [email protected] Fixes TextField hinttext in a11y_assessment (flutter/flutter#150007) 2024-06-10 [email protected] Use const bool.fromEnvironment("dart.tool.dart2wasm") to detect dart2wasm (flutter/flutter#149996) 2024-06-10 [email protected] Roll Packages from 8a2c4e4 to e95fe4a (3 revisions) (flutter/flutter#149997) 2024-06-10 [email protected] [web] Notify engine of handled PointerScrollEvents. (flutter/flutter#145500) 2024-06-10 [email protected] Cut no-longer-accurate microtask reference in finalizeTree doc (flutter/flutter#149941) 2024-06-10 [email protected] Update hasTrailingSpaces (flutter/flutter#149698) 2024-06-10 [email protected] [web] Change `--web-renderer` default from `auto` to `canvaskit` (flutter/flutter#149773) 2024-06-10 [email protected] Retain the toString method for subclasses of Key in profile/release mode (flutter/flutter#149926) 2024-06-10 [email protected] Remove package:platform from issue template (flutter/flutter#149995) 2024-06-10 [email protected] Revert "[CupertinoActionSheet] Match colors to native (#149568)" (flutter/flutter#149998) 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],[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

This PR addresses an issue with TextPainter's caret position calculation for text containing full-width spaces. Currently, the caret position is not accurately calculated for strings with full-width spaces. To resolve this, the following changes have been made: Corrected the logic for caret position calculation when full-width spaces are present in the text. Added and updated test cases to ensure accurate caret position calculation. These changes ensure that the caret position for text with full-width spaces is computed correctly. This issue was introduced by the commit [a0a854a](flutter@a0a854a). Related Issue: [flutter#149099](flutter#149099)

…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 #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

…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

update hasTrailingSpaces

893c0c6

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 Jun 5, 2024

add test with trailing full-width space

0326332

ttorii20 marked this pull request as ready for review June 6, 2024 11:43

Merge branch 'master' into fix/cursor-alignment-fullwidth-spaces

0ed261a

justinmc requested a review from LongCatIsLooong June 6, 2024 21:39

LongCatIsLooong approved these changes Jun 7, 2024

View reviewed changes

justinmc approved these changes Jun 7, 2024

View reviewed changes

ttorii20 added 2 commits June 8, 2024 10:30

Fix: Update SkParagraph handling and consolidate caret tests

352ef10

Merge branch 'fix/cursor-alignment-fullwidth-spaces' of https://githu…

eb72d04

…b.com/ttorii20/flutter into fix/cursor-alignment-fullwidth-spaces

Merge branch 'master' into fix/cursor-alignment-fullwidth-spaces

773ef36

LongCatIsLooong reviewed Jun 8, 2024

View reviewed changes

ttorii20 added 2 commits June 8, 2024 11:37

Correction of comments on white space.

d44c2b8

Merge branch 'fix/cursor-alignment-fullwidth-spaces' of https://githu…

1695815

…b.com/ttorii20/flutter into fix/cursor-alignment-fullwidth-spaces

LongCatIsLooong added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 8, 2024

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jun 8, 2024

Merge branch 'master' into fix/cursor-alignment-fullwidth-spaces

f4ad9dd

LongCatIsLooong added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 10, 2024

auto-submit bot merged commit 739e3bd into flutter:master Jun 10, 2024

engine-flutter-autoroll mentioned this pull request Jun 11, 2024

Roll Flutter from 32081aab69f8 to aad70e5cdb8e (20 revisions) flutter/packages#6906

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 11, 2024

739e3bd Update hasTrailingSpaces (flutter/flutter#149698)

6f9179a

engine-flutter-autoroll mentioned this pull request Jun 11, 2024

Roll Flutter from 32081aab69f8 to 14df7be3f947 (21 revisions) flutter/packages#6907

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 11, 2024

739e3bd Update hasTrailingSpaces (flutter/flutter#149698)

35d31f6

yqritc mentioned this pull request Jul 19, 2024

[CP] Fix TextPainter's caret position calculation for text containing full-width spaces #152016

Closed

yqritc mentioned this pull request Jul 19, 2024

[CP] Fix TextPainter's caret position calculation for text containing full-width #152021

Closed

koji-1009 mentioned this pull request Jul 24, 2024

Fix cursor position when Unicode Zs category is entered in TextField #152215

Merged

9 tasks

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 6, 2024

739e3bd Update hasTrailingSpaces (flutter/flutter#149698)

df03373

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update hasTrailingSpaces#149698

Update hasTrailingSpaces#149698
auto-submit[bot] merged 9 commits intoflutter:masterfrom
ttorii20:fix/cursor-alignment-fullwidth-spaces

ttorii20 commented Jun 5, 2024 •

edited

Loading

Uh oh!

LongCatIsLooong left a comment

Uh oh!

LongCatIsLooong Jun 7, 2024 •

edited

Loading

Uh oh!

LongCatIsLooong Jun 7, 2024

Uh oh!

LongCatIsLooong Jun 7, 2024

Uh oh!

justinmc left a comment

Uh oh!

justinmc Jun 7, 2024

Uh oh!

ttorii20 commented Jun 8, 2024

Uh oh!

LongCatIsLooong left a comment •

edited

Loading

Uh oh!

LongCatIsLooong commented Jun 8, 2024 •

edited

Loading

Uh oh!

LongCatIsLooong commented Jun 8, 2024

Uh oh!

ttorii20 commented Jun 8, 2024

Uh oh!

auto-submit bot commented Jun 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		@@ -356,6 +356,7 @@ class _TextLayout {
		// last logical trailing space.

Conversation

ttorii20 commented Jun 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

LongCatIsLooong left a comment

Choose a reason for hiding this comment

Uh oh!

LongCatIsLooong Jun 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LongCatIsLooong Jun 7, 2024

Choose a reason for hiding this comment

Uh oh!

LongCatIsLooong Jun 7, 2024

Choose a reason for hiding this comment

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

justinmc Jun 7, 2024

Choose a reason for hiding this comment

Uh oh!

ttorii20 commented Jun 8, 2024

Uh oh!

LongCatIsLooong left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LongCatIsLooong commented Jun 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LongCatIsLooong commented Jun 8, 2024

Uh oh!

ttorii20 commented Jun 8, 2024

Uh oh!

auto-submit bot commented Jun 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ttorii20 commented Jun 5, 2024 •

edited

Loading

LongCatIsLooong Jun 7, 2024 •

edited

Loading

LongCatIsLooong left a comment •

edited

Loading

LongCatIsLooong commented Jun 8, 2024 •

edited

Loading