Provide caret rect to text input plugin by knopp · Pull Request #77608 · flutter/flutter

knopp · 2021-03-08T16:10:09Z

Adds required code to provide current caret rectangle to text input plugin. This rect, with addition to current editable transform can be used to position the accent selection menu on macOS similarly to how IME popup is positioned on iOS.

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 feature I am adding, or Hixie said the PR is test-exempt.
All existing and new tests are passing.

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

knopp · 2021-03-08T17:05:49Z

#77052

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

#30660

LongCatIsLooong · 2021-03-08T17:50:00Z

packages/flutter/lib/src/widgets/editable_text.dart

selection can be null or !isValid, in either case there's no caret.

You may also want to make sure selection.isCollapsed is true, because the caret doesn't show up (in addition to the 2 aforementioned cases) if there's an actual non-empty selection.

Addressed here.

Looks like we are skipping reporting the caret rect to the engine when there's no caret. Does the engine need to know that the caret is gone (instead of using the out-of-date caret rect that was previously sent to it)?

The purpose of caret rect is to position the IME candidate popup. The popup is displayed after long pressing a key (or possibly while typing with some languages). So if there is any selection it will be replaced by a character before the popup is displayed. I'm not aware of a way to get the IME popup visible without caret present. And should that ever happen, last caret position seems to me like a good enough fallback.

Wouldn't the accent popup show up at the wrong location if you:

change the selection to somewhere in the document that's far away from the current caret location (the framework won't send anything in this case because it's a non-collapsed selection).

long press a key to trigger the popup

Long pressing key first clears the selection replacing it with pressed character, moving caret to new position. I don't think this would cause any issues. The way this works is that the popup shows the moment character would normally start repeating (after a long key press).

The communication between the text input plugin and the framework is asynchronous. When the software keyboard clears the selection and changes the caret's position, the text input plugin needs to query the new caret coordinates from the framework. There seems to be a chance that the framework can't respond in time, resulting in the outdated caret coordinates being reported in the firstRect method?

I'm not entirely sure what do you mean by software keyboard? if you mean mean the selection popup, that is shown as response to previous keypress (that would clear the selection). The time between the initial key press and showing popup (when long pressing the key) is about a second, plenty of time for framework to notify plugin of cursor location.

ah sorry forgot this is macOS & hardware keyboard, I meant the key event. Sounds good.

LongCatIsLooong · 2021-03-08T17:58:27Z

packages/flutter/lib/src/services/text_input.dart

nit: is this CGRectNull? Could you put a code comment that explains why this invalid rect is specifically chosen?

I wasn't trying to match CGRectNull (i think that's [inf, inf, 0, 0]), just to be consistent with setComposingRect.

LongCatIsLooong · 2021-03-08T22:49:47Z

Looks like @cbracken is about to land a patch that addresses something similar. Maybe we should wait for that patch to land?

I'm not familiar with how accents work on macos. Does long-pressing a key initiate a composing region (that highlights the base character) on macos?

knopp · 2021-03-08T23:14:02Z

Looks like @cbracken is about to land a patch that addresses something similar. Maybe we should wait for that patch to land?

Sure. I wasn't aware @cbracken was working on this when I opened the PR. Don't mean to step on anyone's toes :)

I'm not familiar with how accents work on macos. Does long-pressing a key initiate a composing region (that highlights the base character) on macos?

Not necessarily. The text input controller suppresses normal key-repeat behavior, showing selection popup on first key repeat instead. However it only enters compositing mode after you change the character selection within the popup. Which currently doesn't work in flutter due to way caret movement and arrow events are handled (but you can still pick up option directly by pressing a number, which works). In which case it generates text ~~insertion~~ replacement event, skipping the composition step altogether.

cbracken

Can you provide a bit more context on why this is required? TextInput.setMarkedTextRect should provide the caret position when not composing.

flutter/packages/flutter/lib/src/widgets/editable_text.dart

Lines 2442 to 2463 in 48f5674

    
           // Sends the current composing rect to the iOS text input plugin via the text 
        
           // input channel. We need to keep sending the information even if no text is 
        
           // currently marked, as the information usually lags behind. The text input 
        
           // plugin needs to estimate the composing rect based on the latest caret rect, 
        
           // when the composing rect info didn't arrive in time. 
        
           void _updateComposingRectIfNeeded() { 
        
             final TextRange composingRange = _value.composing; 
        
             if (_hasInputConnection) { 
        
               assert(mounted); 
        
               Rect? composingRect = renderEditable.getRectForComposingRange(composingRange); 
        
               // Send the caret location instead if there's no marked text yet. 
        
               if (composingRect == null) { 
        
                 assert(!composingRange.isValid || composingRange.isCollapsed); 
        
                 final int offset = composingRange.isValid ? composingRange.start : 0; 
        
                 composingRect = renderEditable.getLocalRectForCaret(TextPosition(offset: offset)); 
        
               } 
        
               assert(composingRect != null); 
        
               _textInputConnection!.setComposingRect(composingRect); 
        
               SchedulerBinding.instance! 
        
                   .addPostFrameCallback((Duration _) => _updateComposingRectIfNeeded()); 
        
             } 
        
           }

I'm not aware of any cases where both an IME candidate menu and the accent pop-up are in use at the same time -- in local testing with Japanese and Korean text, I get direct input of the vowel in question (e.g. あ) and no accent menu.

cbracken · 2021-03-09T22:46:44Z

Sure. I wasn't aware @cbracken was working on this when I opened the PR. Don't mean to step on anyone's toes :)

Ah no worries at all! Thanks so much for sending these patches!

I was working on landing TextInput.setMarkedTextRect and TextInput.setEditableSizeAndTransform as part of #30660 for CJK candidates menu positioning, but got yanked into a couple Windows issues the last couple weeks. I hadn't specifically considered the accent menu, which to be honest, I had no idea existed on macOS, despite spending a good chunk of my life typing in French!

Did you try with just the handlers for the above two messages, and if so, what was the result? As noted, despite its unfortunate name, the setMarkedTextRect message should devolve to just the caret rect in the case where the composing range is empty. I'm guessing something there was insufficient though?

knopp · 2021-03-09T23:34:01Z

@cbracken, I did try setMarkedTextRect first, but without compositing it keeps sending Rect.fromLTRB(-1.0, 0.0, 1.0, 19.0) regardless of where the caret is. If that's not correct behavior perhaps setMarkedTextRect should be fixed?

cbracken · 2021-03-10T01:27:30Z

@cbracken, I did try setMarkedTextRect first, but without compositing it keeps sending Rect.fromLTRB(-1.0, 0.0, 1.0, 19.0) regardless of where the caret is.

Ah interesting; yep looking at this line, TextRange.isValid returns false when start/end are -1.

@LongCatIsLooong any concerns with the general approach? From my point of view this seems like a reasonable message to add, and I think this actually solves an additional issue with CJK candidate menu positioning. On some platforms, the candidate dropdown movies with the caret, on others it remains positioned at the bottom left corner of the composing rect. Currently on platforms that track the caret, we track the bottom-right corner of the composing rect as a proxy for that.

An alternative would be to have the setMarkedTextRect message pass the caret even when not composing, but given the message name and likely resulting confusion, I'd really prefer the current approach.

justinmc

I'll defer to @LongCatIsLooong on this, but it seems to make sense to me.

justinmc · 2021-03-10T17:58:29Z

packages/flutter/lib/src/widgets/editable_text.dart

This makes me worry about an infinite loop (well at least doing this unnecessarily once every frame). Are we sure that can never happen? Like for example if the selection is (-1,-1).

TextInputConnection checks if the rect is same, and so skips the message.

I'm not quite sure how infinite loop could happen, it is started in _openInputConnection and repeated every frame for as long as there is input connection. It is exactly the same behavior as _updateComposingRectIfNeeded, so if that doesn't cause infinite loops neither should this.

You're right, _updateComposingRectIfNeeded does seem to do the same thing so I think it's ok 👍

justinmc · 2021-03-10T17:59:30Z

packages/flutter/test/widgets/editable_text_test.dart

Could we test that the Rect that gets passed is the right one somewhere?

LongCatIsLooong

LGTM modulo comments from other reviewers

cbracken · 2021-03-11T18:46:58Z

packages/flutter/lib/src/services/text_input.dart

nit: s/only used/used/ just in case we do start using it in another embedder at some point. IIRC we have similar wording about iOS somewhere that is now out-of-date.

Also super-pedantic nit, but add a period at the end.

I've sent a fix for the other case in #77951.

goderbauer · 2021-03-24T21:59:22Z

(PR triage) @LongCatIsLooong @justinmc @knopp What's the status of this PR?

flutter#77545

knopp · 2021-03-26T17:48:03Z

I rebased the PR and added unit tests (as requested by @justinmc). Anything else needed here?

justinmc

LGTM 👍

The build is currently broken, but this can be merged when it's green.

flutter-dashboard bot added the framework flutter/packages/flutter repository. See also f: labels. label Mar 8, 2021

google-cla bot added the cla: yes label Mar 8, 2021

knopp requested review from LongCatIsLooong and justinmc March 8, 2021 16:10

knopp mentioned this pull request Mar 8, 2021

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

Merged

9 tasks

LongCatIsLooong reviewed Mar 8, 2021

View reviewed changes

LongCatIsLooong requested a review from cbracken March 8, 2021 23:02

cbracken reviewed Mar 9, 2021

View reviewed changes

justinmc reviewed Mar 10, 2021

View reviewed changes

LongCatIsLooong approved these changes Mar 11, 2021

View reviewed changes

cbracken reviewed Mar 11, 2021

View reviewed changes

knopp mentioned this pull request Mar 11, 2021

Update composing rect documentation #77951

Merged

8 tasks

knopp added 5 commits March 26, 2021 18:23

Provide caret rect to text input plugin

2fbc354

flutter#77545

Ensure selection is valid and collapsed

8a7df57

Update tests

2a31a5a

nit

b787417

Add unit tests for setCaretRect

a499cc9

knopp force-pushed the text_input_set_caret_rect branch from e3a1965 to a499cc9 Compare March 26, 2021 17:24

justinmc approved these changes Mar 29, 2021

View reviewed changes

justinmc added the waiting for tree to go green label Mar 29, 2021

fluttergithubbot merged commit 462ad79 into flutter:master Mar 30, 2021

knopp mentioned this pull request Apr 9, 2021

MacOS: Accent popup position is still wrong #80164

Closed

	// Sends the current composing rect to the iOS text input plugin via the text
	// input channel. We need to keep sending the information even if no text is
	// currently marked, as the information usually lags behind. The text input
	// plugin needs to estimate the composing rect based on the latest caret rect,
	// when the composing rect info didn't arrive in time.
	void _updateComposingRectIfNeeded() {
	final TextRange composingRange = _value.composing;
	if (_hasInputConnection) {
	assert(mounted);
	Rect? composingRect = renderEditable.getRectForComposingRange(composingRange);
	// Send the caret location instead if there's no marked text yet.
	if (composingRect == null) {
	assert(!composingRange.isValid \|\| composingRange.isCollapsed);
	final int offset = composingRange.isValid ? composingRange.start : 0;
	composingRect = renderEditable.getLocalRectForCaret(TextPosition(offset: offset));
	}
	assert(composingRect != null);
	_textInputConnection!.setComposingRect(composingRect);
	SchedulerBinding.instance!
	.addPostFrameCallback((Duration _) => _updateComposingRectIfNeeded());
	}
	}

Conversation

knopp commented Mar 8, 2021

Pre-launch Checklist

Uh oh!

knopp commented Mar 8, 2021

Uh oh!

knopp commented Mar 8, 2021

Uh oh!

LongCatIsLooong Mar 8, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LongCatIsLooong commented Mar 8, 2021

Uh oh!

knopp commented Mar 8, 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!

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

Uh oh!

Uh oh!

knopp commented Mar 9, 2021

Uh oh!

cbracken commented Mar 10, 2021

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LongCatIsLooong left a comment

Choose a reason for hiding this comment

Uh oh!

cbracken Mar 11, 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!

cbracken Mar 11, 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!

goderbauer commented Mar 24, 2021

Uh oh!

knopp commented Mar 26, 2021

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

LongCatIsLooong Mar 8, 2021 •

edited

Loading

knopp commented Mar 8, 2021 •

edited

Loading

cbracken commented Mar 9, 2021 •

edited

Loading

cbracken Mar 11, 2021 •

edited

Loading

cbracken Mar 11, 2021 •

edited

Loading