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

Improve IME area reporting and set IME area in TextArea #785

Merged
merged 3 commits into from
Dec 11, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ tree_arena = { version = "0.1.0", path = "tree_arena" }
vello = "0.3"
wgpu = "22.1.0"
kurbo = "0.11.1"
parley = { git = "https://github.com/linebender/parley", rev = "211878fae20ba8b4bf4dcc39e6bf790eece8aa03", features = [
parley = { git = "https://github.com/linebender/parley", rev = "1a8740d8d86ebf751201e45e89bb71019340137d", features = [
"accesskit",
] }
peniko = "0.2.0"
Expand Down
19 changes: 19 additions & 0 deletions masonry/src/contexts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,25 @@ impl_context_method!(
.expect("get_child_state_mut: child not found");
child_state_mut.item
}

/// Set the IME cursor area.
///
/// When this widget is [focused] and [accepts text input], the reported IME area is sent
/// to the platform. The area can be used by the platform to, for example, place a
/// candidate box near that area, while ensuring the area is not obscured.
///
/// [focused]: EventCtx::request_focus
/// [accepts text input]: Widget::accepts_text_input
pub fn set_ime_area(&mut self, ime_area: Rect) {
self.widget_state.ime_area = Some(ime_area);
}

/// Remove the IME cursor area.
///
/// See [`LayoutCtx::set_ime_area`](LayoutCtx::set_ime_area) for more details.
pub fn clear_ime_area(&mut self) {
self.widget_state.ime_area = None;
}
}
);

Expand Down
8 changes: 1 addition & 7 deletions masonry/src/passes/compose.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use tree_arena::ArenaMut;
use vello::kurbo::Vec2;

use crate::passes::{enter_span_if, recurse_on_children};
use crate::render_root::{RenderRoot, RenderRootSignal, RenderRootState};
use crate::render_root::{RenderRoot, RenderRootState};
use crate::{ComposeCtx, Widget, WidgetState};

// --- MARK: RECURSE ---
Expand Down Expand Up @@ -42,12 +42,6 @@ fn compose_widget(
widget.item.compose(&mut ctx);
}

// TODO - Add unit tests for this.
if moved && state.item.accepts_text_input && global_state.is_focused(state.item.id) {
let ime_area = state.item.get_ime_area();
global_state.emit_signal(RenderRootSignal::new_ime_moved_signal(ime_area));
}

// We need to update the accessibility node's coordinates and repaint it at the new position.
state.item.request_accessibility = true;
state.item.needs_accessibility = true;
Expand Down
5 changes: 0 additions & 5 deletions masonry/src/passes/update.rs
Original file line number Diff line number Diff line change
Expand Up @@ -523,11 +523,6 @@ pub(crate) fn run_update_focus_pass(root: &mut RenderRoot) {
if widget_state.accepts_text_input {
root.global_state.emit_signal(RenderRootSignal::StartIme);
}

root.global_state
.emit_signal(RenderRootSignal::new_ime_moved_signal(
widget_state.get_ime_area(),
));
} else {
root.global_state.is_ime_active = false;
}
Expand Down
34 changes: 33 additions & 1 deletion masonry/src/render_root.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,10 @@ use crate::text::BrushIndex;
use crate::widget::{WidgetArena, WidgetMut, WidgetRef, WidgetState};
use crate::{AccessEvent, Action, CursorIcon, Handled, QueryCtx, Widget, WidgetId, WidgetPod};

/// We ensure that any valid initial IME area is sent to the platform by storing an invalid initial
/// IME area as the `last_sent_ime_area`.
const INVALID_IME_AREA: Rect = Rect::new(f64::NAN, f64::NAN, f64::NAN, f64::NAN);

// --- MARK: STRUCTS ---

pub struct RenderRoot {
Expand Down Expand Up @@ -74,6 +78,10 @@ pub(crate) struct RenderRootState {
pub(crate) text_layout_context: LayoutContext<BrushIndex>,
pub(crate) mutate_callbacks: Vec<MutateCallback>,
pub(crate) is_ime_active: bool,
/// The IME area last sent to the platform.
///
/// This allows only sending the area to the platform when the area has changed.
pub(crate) last_sent_ime_area: Rect,
tomcur marked this conversation as resolved.
Show resolved Hide resolved
pub(crate) scenes: HashMap<WidgetId, Scene>,
/// Whether data set in the pointer pass has been invalidated.
pub(crate) needs_pointer_pass: bool,
Expand Down Expand Up @@ -170,6 +178,7 @@ impl RenderRoot {
text_layout_context: LayoutContext::new(),
mutate_callbacks: Vec::new(),
is_ime_active: false,
last_sent_ime_area: INVALID_IME_AREA,
scenes: HashMap::new(),
needs_pointer_pass: false,
trace: PassTracing::from_env(),
Expand Down Expand Up @@ -263,6 +272,12 @@ impl RenderRoot {
let _span = info_span!("text_event");
let handled = run_on_text_event_pass(self, &event);
run_update_focus_pass(self);

if matches!(event, TextEvent::Ime(winit::event::Ime::Enabled)) {
// Reset the last sent IME area, as the platform reset the IME state and may have
// forgotten it.
self.global_state.last_sent_ime_area = INVALID_IME_AREA;
}
self.run_rewrite_passes();

handled
Expand Down Expand Up @@ -489,6 +504,19 @@ impl RenderRoot {
self.global_state
.emit_signal(RenderRootSignal::RequestRedraw);
}

if self.global_state.is_ime_active {
let widget = self
.global_state
.focused_widget
.expect("IME is active without a focused widget");
let ime_area = self.widget_arena.get_state(widget).item.get_ime_area();
Copy link
Member

Choose a reason for hiding this comment

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

It's conceivable that you have enough context to call a Widget method with a QueryCtx here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that probably is possible. Hmm, my main concern with both using QueryCtx and removing the repeated calls to set the IME area that you mentioned on TextArea::ime_area, would be that the results aren't cached. In general, calculating the area can be a somewhat expensive operation – and certainly in the case of PlainEditor it is somewhat expensive.

I believe the question is then whether WidgetState stores the IME area, which is what Masonry currently does, or whether that field should be removed in favor of a query method on Widget, which moves the responsibility of caching when necessary to the widget. This doesn't solve the issue of the repeated TextArea::ime_area calls.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, that's true, and it's probably too messy. Without having a new invalidation method, it doesn't really work.

if self.global_state.last_sent_ime_area != ime_area {
self.global_state.last_sent_ime_area = ime_area;
self.global_state
.emit_signal(RenderRootSignal::new_ime_moved_signal(ime_area));
}
}
Comment on lines +508 to +519
Copy link
Member Author

Choose a reason for hiding this comment

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

My understanding is that these rewrite passes are intended to make the state consistent, so Masonry will know the correct IME area once this has been done. Some other signals are emitted to the platform here as well. While I believe it makes sense to add this here, I can certainly be convinced otherwise.

Copy link
Member

Choose a reason for hiding this comment

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

This seems valid to me.

}

pub(crate) fn request_render_all(&mut self) {
Expand Down Expand Up @@ -580,6 +608,10 @@ impl RenderRootState {
self.focused_widget != self.next_focused_widget
}

#[expect(
dead_code,
reason = "no longer used, but may be useful again in the future"
)]
pub(crate) fn is_focused(&self, id: WidgetId) -> bool {
self.focused_widget == Some(id)
}
Expand All @@ -594,7 +626,7 @@ impl RenderRootSignal {
RenderRootSignal::ImeMoved(
LogicalPosition {
x: area.origin().x,
y: area.origin().y + area.size().height,
y: area.origin().y,
},
LogicalSize {
width: area.size().width,
Expand Down
26 changes: 25 additions & 1 deletion masonry/src/widget/text_area.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ use parley::layout::Alignment;
use parley::PlainEditor;
use smallvec::SmallVec;
use tracing::{trace_span, Span};
use vello::kurbo::Vec2;
use vello::kurbo::{Rect, Vec2};
use vello::peniko::{Brush, Color, Fill};
use vello::Scene;
use winit::keyboard::{Key, NamedKey};
Expand Down Expand Up @@ -299,6 +299,25 @@ impl<const EDITABLE: bool> TextArea<EDITABLE> {
}
}

// --- MARK: HELPERS ---
impl<const EDITABLE: bool> TextArea<EDITABLE> {
/// Get the IME area from the editor, accounting for padding.
///
/// This should only be called when the editor layout is available.
fn ime_area(&self) -> Rect {
Copy link
Member

Choose a reason for hiding this comment

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

This being repeated so often to me suggests that it should be a method on Widget, so that there is only one source of truth needed. That would be a bit of a bigger change to wire up, and I don't have all the specifics.

That is, this is a non-blocking conern

debug_assert!(
self.editor.try_layout().is_some(),
"TextArea::ime_area should only be called when the editor layout is available"
);
let is_rtl = self
.editor
.try_layout()
.map(|layout| layout.is_rtl())
.unwrap_or(false);
self.editor.ime_cursor_area() + Vec2::new(self.padding.get_left(is_rtl), self.padding.top)
}
}

// --- MARK: WIDGETMUT ---
impl<const EDITABLE: bool> TextArea<EDITABLE> {
/// Set font styling for an active text area.
Expand Down Expand Up @@ -499,6 +518,7 @@ impl<const EDITABLE: bool> Widget for TextArea<EDITABLE> {
let new_generation = self.editor.generation();
if new_generation != self.rendered_generation {
ctx.request_render();
ctx.set_ime_area(self.ime_area());
self.rendered_generation = new_generation;
}
ctx.request_focus();
Expand All @@ -515,6 +535,7 @@ impl<const EDITABLE: bool> Widget for TextArea<EDITABLE> {
let new_generation = self.editor.generation();
if new_generation != self.rendered_generation {
ctx.request_render();
ctx.set_ime_area(self.ime_area());
self.rendered_generation = new_generation;
}
}
Expand All @@ -539,6 +560,7 @@ impl<const EDITABLE: bool> Widget for TextArea<EDITABLE> {
},
);
let (fctx, lctx) = ctx.text_contexts();
// Whether the text was changed.
let mut edited = false;
// Ideally we'd use key_without_modifiers, but that's broken
match &key_event.logical_key {
Expand Down Expand Up @@ -730,6 +752,7 @@ impl<const EDITABLE: bool> Widget for TextArea<EDITABLE> {
ctx.request_layout();
} else {
ctx.request_render();
ctx.set_ime_area(self.ime_area());
}
self.rendered_generation = new_generation;
}
Expand Down Expand Up @@ -851,6 +874,7 @@ impl<const EDITABLE: bool> Widget for TextArea<EDITABLE> {
let layout = self.editor.layout(fctx, lctx);
let text_width = max_advance.unwrap_or(layout.full_width());
let text_size = Size::new(text_width.into(), layout.height().into());
ctx.set_ime_area(self.ime_area());

let area_size = Size {
height: text_size.height + padding_size.height,
Expand Down
Loading