Position accent popup window next to caret #24867

knopp · 2021-03-08T16:22:38Z

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 Hixie said 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.
The reviewer has submitted any presubmit flakes in this PR using the engine presubmit flakes form before re-triggering the failure.

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

flutter-dashboard · 2021-03-08T16:22:40Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

knopp · 2021-03-08T17:06:33Z

flutter/flutter#30660

cbracken · 2021-03-09T22:49:27Z

shell/platform/darwin/macos/framework/Source/FlutterTextInputPlugin.mm

+  } else if ([method isEqualToString:@"TextInput.setEditableSizeAndTransform"]) {
+    NSDictionary* state = call.arguments;
+    [self setEditableTransform:state[@"transform"]];
+  } else if ([method isEqualToString:@"TextInput.setCaretRect"]) {


Added comments on flutter/flutter#77608. I'd have thought that TextInput.setMarkedTextRect should be sufficient here -- despite its name, it should emit the current caret rect when the composing range is empty.

As noted in flutter/flutter#77608, TextInput.setMarkedTextRect only works while compositing. So we need another method for caret position.

shell/platform/darwin/macos/framework/Source/FlutterTextInputPlugin.mm

cbracken

Thanks for sending this! I think if you add a handler for TextInput.setMarkedTextRect you should be able to get away without the new message. You'll want to invalidate/re-cache the transformed rect on each update of either the setMarkedTextRect or setEditableSizeAndTransform message.

chinmaygarde · 2021-03-18T21:38:13Z

@knopp Any updates on @cbracken last comment? If progress is not likely soon, maybe we can close this for now and reopen later?

knopp · 2021-03-18T22:12:47Z

@chinmaygarde, sorry, missed the notifications on this one. I fixed the typo. However this PR does depend on flutter/flutter#77608.

chinmaygarde · 2021-03-25T21:48:02Z

The linked PR has landed. Are there issues with the order in which the commits need to be landed?

knopp · 2021-03-26T16:58:52Z

@cbracken, @chinmaygarde, TextInput.setMarkedTextRect doesn't work because it is only valid during compositing. However we need cared position even outside of compositing. This has been discussed in flutter/flutter#77608. I'm not sure what you mean by the linked PR having landed, this depend on flutter/flutter#77608 which hasn't landed yet.

cbracken · 2021-03-29T19:47:14Z

To be clear -- for future archaeologists s/compositing/composing/ in the discussions above. setMarkedTextRect is used for IME composing mode and is unrelated to compositing.

To summarise previous discussions elsewhere with @knopp and offline with @LongCatIsLooong, this and setMarkedTextRect cover slightly different but overlapping usecases:

When using a composing IME such as for Chinese, Japanese, Korean input, we need the mark rect since the candidates dropdown menu should remain fixed to the bottom-left of the composing region (depending on platform) until the composing text is committed.

This patch covers non-composing use-cases where an OS-provided accent-selection popup is used to inject an accented character (é, è, ê, etc.) and should move with the caret. I'd actually forgotten this use-case existed as I'd apparently disabled it using defaults at some point -- I communicate a fair bit in French but use option-{e,i,u,`} dead-keys for input :(

I did some local testing on macOS, and it appears that when IME composing is active; long-press accent selection is disabled, so there should be no risk of the two behaviours interfering with each other.

cbracken

This LGTM. Apologies for the delay -- this fell off my radar while I dealt with a regression to IME composing on macOS after the key event patches landed.

flutter/flutter#77545

GreenLim · 2021-04-01T02:28:11Z

flutter/flutter#77608 has been merged. so these commits can be landed now? it's import for input.

)

chunhtai · 2021-04-13T22:37:25Z

shell/platform/darwin/macos/framework/Source/FlutterTextInputPlugin.mm

+        CGRectApplyAffineTransform(_caretRect, CATransform3DGetAffineTransform(_editableTransform));
+
+    // flip and convert to screen coordinates
+    double viewHeight = self.flutterViewController.view.bounds.size.height;


self.flutterViewController will be null here ref https://github.com/flutter/engine/pull/23469/files#r612811988

cc @knopp @cbracken

I noticed that :)

Should be fixed in #25524 .

* Position accent popup window next to caret flutter/flutter#77545 * Fix typo

googlebot added the cla: yes label Mar 8, 2021

knopp requested review from cbracken and stuartmorgan-g March 8, 2021 16:22

cbracken reviewed Mar 9, 2021

View reviewed changes

shell/platform/darwin/macos/framework/Source/FlutterTextInputPlugin.mm Outdated Show resolved Hide resolved

cbracken suggested changes Mar 9, 2021

View reviewed changes

stuartmorgan-g removed their request for review March 10, 2021 15:10

cbracken approved these changes Mar 29, 2021

View reviewed changes

knopp added 2 commits March 29, 2021 22:03

Position accent popup window next to caret

c76f83f

flutter/flutter#77545

Fix typo

eaf9587

knopp force-pushed the macos_accent_popup_location branch from fd82053 to eaf9587 Compare March 29, 2021 20:03

knopp merged commit 82e9d8f into flutter:master Apr 1, 2021

engine-flutter-autoroll mentioned this pull request Apr 1, 2021

Roll Engine from 4b52a9f148c6 to 82e9d8f896c6 (1 revision) flutter/flutter#79541

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 1, 2021

82e9d8f Position accent popup window next to caret (flutter/engine#24867

687a60c

)

engine-flutter-autoroll mentioned this pull request Apr 1, 2021

Roll Engine from 4b52a9f148c6 to 686761833a6f (3 revisions) flutter/flutter#79544

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 1, 2021

82e9d8f Position accent popup window next to caret (flutter/engine#24867

afed122

)

chunhtai mentioned this pull request Apr 13, 2021

Hardware Keyboard: macOS #23469

Merged

13 tasks

chunhtai reviewed Apr 13, 2021

View reviewed changes

duanqz pushed a commit to duanqz/engine that referenced this pull request Apr 16, 2021

Position accent popup window next to caret (flutter#24867)

f833c14

* Position accent popup window next to caret flutter/flutter#77545 * Fix typo

knopp deleted the macos_accent_popup_location branch July 17, 2021 09:25

Position accent popup window next to caret #24867

Position accent popup window next to caret #24867

Uh oh!

Conversation

knopp commented Mar 8, 2021

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented Mar 8, 2021

Uh oh!

knopp commented Mar 8, 2021

Uh oh!

cbracken Mar 9, 2021

Choose a reason for hiding this comment

Uh oh!

knopp Mar 18, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cbracken left a comment

Choose a reason for hiding this comment

Uh oh!

chinmaygarde commented Mar 18, 2021

Uh oh!

knopp commented Mar 18, 2021

Uh oh!

chinmaygarde commented Mar 25, 2021

Uh oh!

knopp commented Mar 26, 2021

Uh oh!

cbracken commented Mar 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cbracken left a comment

Choose a reason for hiding this comment

Uh oh!

GreenLim commented Apr 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chunhtai Apr 13, 2021

Choose a reason for hiding this comment

Uh oh!

knopp Apr 13, 2021 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

cbracken commented Mar 29, 2021 •

edited

Loading

GreenLim commented Apr 1, 2021 •

edited

Loading

knopp Apr 13, 2021 •

edited

Loading