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.

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

Reverts part of "fix auto-correction highlight on top left corner (Again)" #45523

Merged
merged 1 commit into from Sep 7, 2023

Conversation

hellohuanlin
Copy link
Contributor

@hellohuanlin hellohuanlin commented Sep 7, 2023

This reverts part of #44779. Verified this to be the regression for flutter/flutter#133908.

This pair of textWill/DidChange calls was added in Beta 1, in order to fix highlight region missing last character (more details here).

However, that fix doesn't work anymore for Beta 7 - it turns out that in Beta 7, the system doesn't rely on the selectionRects to determine the highlight region anymore. Instead, the system relies on firstRectForRange API. So we fixed the system highlight in firstRectForRange in Beta 7.

So even after reverting this change, the system highlight still works correctly in Beta 7. I was initially leaning towards keeping these calls even after Beta 7, because without it, the selectionRects are still out of sync with iOS text input system. (more details here).

However, I was able to verify that it is indeed textWill/DidChange that introduced the regression for IME inputs (It's unclear why they are related, so need further investigation).

In summary, after this revert, system highlights are still in the right place, and with the right length.

List which issues are fixed by this PR. You must list at least one issue.

Fixes flutter/flutter#133908

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

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 and the C++, Objective-C, Java style guides.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

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

@stuartmorgan
Copy link
Contributor

Please update the PR description to clearly explain the behavioral changes that are introduced by this partial revert. Am I correct in thinking we'll have the highlight rect in the right place (so not the upper-left-corner issue) but missing the last letter?

@hellohuanlin
Copy link
Contributor Author

@stuartmorgan updated the description.

Copy link
Contributor

@stuartmorgan stuartmorgan left a comment

Choose a reason for hiding this comment

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

Thanks for that context, it's great that this won't (at least for now) cause any known regressions.

LGTM

@hellohuanlin hellohuanlin added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 7, 2023
@auto-submit auto-submit bot merged commit 554a456 into flutter:main Sep 7, 2023
25 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 7, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Sep 7, 2023
…134249)

flutter/engine@2dba8ce...a828c26

2023-09-07 skia-flutter-autoroll@skia.org Roll ANGLE from 204c07a56b64 to cdbc45a9f37e (1 revision) (flutter/engine#45551)
2023-09-07 skia-flutter-autoroll@skia.org Roll Fuchsia Mac SDK from WbB3tmMXnuwJBAHoi... to EuBfOtm5TZIdgNaQe... (flutter/engine#45550)
2023-09-07 skia-flutter-autoroll@skia.org Roll Skia from ee741e5e8cf3 to ce2c94883cb5 (1 revision) (flutter/engine#45552)
2023-09-07 30870216+gaaclarke@users.noreply.github.com [Impeller] Started tracking the pool with the command buffer. (flutter/engine#45298)
2023-09-07 49699333+dependabot[bot]@users.noreply.github.com Bump actions/checkout from 3.6.0 to 4.0.0 (flutter/engine#45439)
2023-09-07 41930132+hellohuanlin@users.noreply.github.com Reverts part of "fix auto-correction highlight on top left corner (Again)"  (flutter/engine#45523)
2023-09-07 skia-flutter-autoroll@skia.org Roll Skia from 59a2610cd83d to ee741e5e8cf3 (4 revisions) (flutter/engine#45548)
2023-09-07 skia-flutter-autoroll@skia.org Roll ANGLE from 60b56591dee5 to 204c07a56b64 (7 revisions) (flutter/engine#45546)

Also rolling transitive DEPS:
  fuchsia/sdk/core/mac-amd64 from WbB3tmMXnuwJ to EuBfOtm5TZId

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC rmistry@google.com,zra@google.com on the revert to ensure that a human
is aware of the problem.

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

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
@zhangyanzhu88
Copy link

zhangyanzhu88 commented Sep 11, 2023

@hellohuanlin Has this question been corrected?Have you updated to the latest version?

@hellohuanlin
Copy link
Contributor Author

@hellohuanlin Has this question been corrected?Have you updated to the latest version?

Will submit a CP Monday or Tuesday.

@zhangyanzhu88
Copy link

@hellohuanlin this is great, thank you

auto-submit bot pushed a commit that referenced this pull request Sep 12, 2023
… (Again)" (#45677)

Original PR: #45523

*List which issues are fixed by this PR. You must list at least one issue.*

Fixes flutter/flutter#133908

*If you had to change anything in the [flutter/tests] repo, include a link to the migration guide as per the [breaking change policy].*

[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App platform-ios
Projects
None yet
3 participants