-
Notifications
You must be signed in to change notification settings - Fork 120
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
Re-add IME support #762
Re-add IME support #762
Conversation
I believe the cursor should still land at index `self.buffer.len()` (logically following the last cluster). E.g., in linebender/xilem#762, if used without this change, the selection can't span the last cluster using `cursor_at`.
I believe the cursor should still land at index `self.buffer.len()` (logically following the last cluster). E.g., in linebender/xilem#762, if used without this change, the selection can't span the last cluster using `cursor_at`.
Another regression from #754 ... Needed for #762 --------- Co-authored-by: Tom Churchman <[email protected]>
if !self.editor.is_composing() && self.editor.selected_text().is_some() | ||
{ | ||
// The IME has started composing. Delete the current selection and | ||
// send a TextChange event with the selection removed, but without | ||
// the composing preedit text. | ||
self.editor.transact(fctx, lctx, |txn| { | ||
txn.delete_selection(); | ||
}); | ||
submit_text = Some(self.text().to_string()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note PlainEditorTxn::set_compose
also removes the selection from the buffer when the IME newly starts composing (i.e., the first compose after or after a clear_compose
). That seemed like a useful abstraction for Parley. The Masonry code here as a special-case also removes the selection, because Masonry might not want to send preedit text in TextChange
actions, but does want to notify about the removed selection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works really well for me. It does show that underlines for an emoji is in the wrong place, but that's not the fault of this PR.
Some minor points to be considered. I don't think there's anything blocking, but we should also check-in with Matt.
)); | ||
} else { | ||
// IME indicates nothing is to be selected: collapse the selection to a | ||
// caret just in front of the preedit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd normally expect the cursor to be at the end of the region in this circumstance. Is this working on some prior art?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was more a less an arbitrary choice I lifted from linebender/parley#111, but on my platform, Firefox, Chrome and LibreOffice Writer all place the caret in front of the preedit text (and they appear to never make a spanning selection).
Firefox:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. Is that hitting this condition where the IME doesn't provide a preedit cursor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Performing the same IME actions on Winit as on Firefox, provides a preedit cursor that spans the entire preedit text.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I haven't figured out steps yet with this IME that sends a cursor: None
.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've done a little bit more research into this, and it seems like Winit gets into a borked state. Any choice seems reasonable here. Alternatively, maybe we should just not have a visible cursor if we're asked to display none?
To clarify, I have confirmed that my IME is still sending cursor positions, but winit is sending us None
. I don't know why
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's all slightly messy, with different implementations making different choices. Hiding the cursor completely is what Winit suggests, so perhaps hiding the caret is the best option.
Maybe we should then also call accesskit::node::clear_text_selection
, but I'm not sure if that messes with something.
self.update_layout(); | ||
|
||
self.editor | ||
.set_selection(self.editor.cursor_at(preedit_range.start).into()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is unfortunate in the commit case, because we do a layout here to be able to set the selection, then moments later we change the text, forcing another layout. Practically this is probably fine, but it would be nice to think about whether it can be avoided.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PlainEditor::layout_dirty
gets us part of the way there, but as you mentioned, we can't set the next selection until the layout is updated (it is forced to fit within the current layout). It might work if we have some (private) PlainEditor::defer_set_selection
or something similar, that simply waits with setting the selection until inside the update_layout
method.
} | ||
|
||
fn on_access_event(&mut self, ctx: &mut EventCtx, event: &AccessEvent) { | ||
if event.action == accesskit::Action::SetTextSelection { | ||
if self.editor.is_composing() { | ||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should check with @mwcampbell if this is right. The scenario is:
- The user is inputting some text which uses a pre-edit through their IME
- Through their accessibility tool, they have tried to select some text, without their IME finishing the input
What should we do in that case? This code just ignores the second request, which is the easiest method to reason about. The alternatives are quite messy.
Co-authored-by: Daniel McNab <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A potential simplification of the new pass code.
Either applying this or not, I think we can still land this. The only accessibilty integration problem might occur if there is a pre-edit, and not having IME is a much bigger accessibility fail. I'm unlikely to be able to review again before Monday (at least), but I'm happy for it to land as-is (presuming there aren't any massive new changes, of course)
Co-authored-by: Daniel McNab <[email protected]>
I'm going to defer the decision on cursor hiding to a different PR, as it may hit on AccessKit accessibility, too. |
This is for #192, because of the changes in linebender/xilem#762
This adds IME support back into Masonry. This sticks close to linebender/parley#111, except that during IME compose, this version doesn't allow changing the selection anchor, making the code simpler. For reference, there's also linebender/parley#136.
This tweaks the focus update pass: when a widget with IME is unfocused, Masonry sends the platform's IME disable event to the newly focused widget. As a workaround, we synthesize an IME disable event in the focus pass and send it to the widget that is about to be unfocused. A complication is that the handling of that event can request focus to a different widget, and in particular, can request itself to be focused again. This handles that case, too.
Remaining work is setting the IME candidate region to be near the current selection and to make a decision on cursor/selection hiding when the platform sends a
None
cursor.