Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix cursor jumping when typing some special characters. #7964

Merged
merged 11 commits into from
Mar 1, 2019
Merged

Fix cursor jumping when typing some special characters. #7964

merged 11 commits into from
Mar 1, 2019

Conversation

HelloCore
Copy link
Contributor

@HelloCore HelloCore commented Feb 26, 2019 •
edited
Loading

Trust GlyphPosition instead of the offset that came from Dart because Dart sees a combined character as multiple characters.

For example, when we type สี, it will call GetRectsForRange twice.
Which start = 0, end = 1 and start = 1, end = 2
However, when I looked at GlyphPosition on the second time, I got gp.code_units.start = 0 and gp.code_units.end = 2, which makes it skip the measuring because gp.code_units.start < start.

Relate to:
flutter/flutter#21745

@HelloCore HelloCore changed the title WIP: Fix cursor jumping when typing some special characters. Fix cursor jumping when typing some special characters. Feb 26, 2019
@HelloCore HelloCore changed the title Fix cursor jumping when typing some special characters. WIP: Fix cursor jumping when typing some special characters. Feb 26, 2019
@GaryQian GaryQian added the Work in progress (WIP) Not ready (yet) for review! label Feb 26, 2019
@GaryQian
Copy link
Contributor

Thanks for looking at this! Let me know when this is ready for review!

@HelloCore
Format code
83418cb
@HelloCore
Keep old logic and add exception for this case
419499c 
It caused bug on some cases, so better keep old logic and add exception for this case
@HelloCore HelloCore changed the title WIP: Fix cursor jumping when typing some special characters. Fix cursor jumping when typing some special characters. Feb 27, 2019
@HelloCore HelloCore changed the title Fix cursor jumping when typing some special characters. WIP: Fix cursor jumping when typing some special characters. Feb 27, 2019
@HelloCore HelloCore changed the title WIP: Fix cursor jumping when typing some special characters. Fix cursor jumping when typing some special characters. Feb 27, 2019
@HelloCore
Copy link
Contributor Author

HelloCore commented Feb 27, 2019 •
edited
Loading

Thanks for looking at this! Let me know when this is ready for review!

Thank you, I think it's ready now. 🎉

@GaryQian GaryQian removed the Work in progress (WIP) Not ready (yet) for review! label Feb 27, 2019
@GaryQian
Copy link
Contributor

Thanks for looking into this issue! I left some suggestions, and I'll go ahead and build this and test it locally.

Please make sure this is tested so that we dont break this in the future, (see https://github.com/flutter/flutter/wiki/Tree-hygiene).

Tests for this would belong in paragraph_unittests.cc, you should be able to follow the format of the existing tests for getRectsForRange. Run the tests by building a host_debug_unopt build, going to out/host_debug_unopt and running ./txt_unittests --font-directory=../../flutter/third_party/txt/third_party/fonts (linux)

@HelloCore
Copy link
Contributor Author

HelloCore commented Feb 28, 2019 •
edited
Loading

./txt_unittests --font-directory=../../flutter/third_party/txt/third_party/fonts

Thank you for reviewing the code and your suggestions. I really appreciate it. 🙏

I mainly used OSX, and I found that some tests were failed on OSX.
For example, GetRectsForRangeTight, when comparing boxes[0].rect.top(), I got -0.00015068054 instead of 0, or The difference between boxes[0].rect.top() and 34.5 is 0.0001220703125, which exceeds 0.0001, where boxes[0].rect.top() evaluates to 34.5001220703125

I believe this is correct, but it makes a little bit harder to test because I have to comment it, and make sure that I don't commit it in the future. 😢

Also, I couldn't use font NotoColorEmoji on OSX.
I think it would be better if we have something like DISABLE_TEST_WINDOWS, but for OSX. 😄

@GaryQian
Copy link
Contributor

GaryQian commented Feb 28, 2019 •
edited
Loading

Yeah unfortunately due to platform font loading and interpretation differences, we only run txt_unittests on Linux.

I'll take another look in the morning!

GaryQian
GaryQian approved these changes Feb 28, 2019
View reviewed changes
Copy link
Contributor

@GaryQian GaryQian left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM! Thanks for your contribution! I'll take a look at your other PR too when I get a moment. I hugely appreciate your insight into solving the issue with Thai!

Just a very minor cosmetic comment, and this should be good to go.

@HelloCore
Copy link
Contributor Author

LGTM! Thanks for your contribution! I'll take a look at your other PR too when I get a moment. I hugely appreciate your insight into solving the issue with Thai!

Just a very minor cosmetic comment, and this should be good to go.

Thank you for your effort to review my code!
I really really appreciate it. 🙏

@GaryQian GaryQian merged commit 302e2e9 into flutter:master Mar 1, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 1, 2019
@engine-flutter-autoroll
302e2e9 Fix cursor jumping when typing some special characters. (flut…
d3b6074 
…ter/engine#7964)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Mar 1, 2019
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 1, 2019
@engine-flutter-autoroll
302e2e9 Fix cursor jumping when typing some special characters. (flut…
5fa1660 
…ter/engine#7964)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Mar 1, 2019
Closed
cbracken added a commit to cbracken/flutter that referenced this pull request Mar 1, 2019
@cbracken
Roll engine to 99f3f7a
4568f6a 
flutter/engine@99f3f7a9c Fix incorrect transformation matrix (flutter/engine#8001)
flutter/engine@c88b09710 Roll src/third_party/skia b7b2da871e95..255569187f27 (23 commits) (flutter/engine#8002)
flutter/engine@302e2e9d2 Fix cursor jumping when typing some special characters. (flutter/engine#7964)
flutter/engine@fe15149d1 Android Embedding PR 12: Add lifecycle methods to FlutterActivity. (flutter/engine#7974)
flutter/engine@6145e9046 Android Embedding PR 13: Integrated text input, keyevent input, and some other channel comms in FlutterView. (flutter/engine#7979)
@cbracken cbracken mentioned this pull request Mar 1, 2019
Merged
cbracken added a commit to flutter/flutter that referenced this pull request Mar 1, 2019
@cbracken
Roll engine to 99f3f7a (#28698)
046f960 
flutter/engine@99f3f7a9c Fix incorrect transformation matrix (flutter/engine#8001)
flutter/engine@c88b09710 Roll src/third_party/skia b7b2da871e95..255569187f27 (23 commits) (flutter/engine#8002)
flutter/engine@302e2e9d2 Fix cursor jumping when typing some special characters. (flutter/engine#7964)
flutter/engine@fe15149d1 Android Embedding PR 12: Add lifecycle methods to FlutterActivity. (flutter/engine#7974)
flutter/engine@6145e9046 Android Embedding PR 13: Integrated text input, keyevent input, and some other channel comms in FlutterView. (flutter/engine#7979)
@jason-simmons jason-simmons mentioned this pull request Feb 2, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GaryQian GaryQian GaryQian approved these changes

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@HelloCore @GaryQian @googlebot