SemianiakaVY/openpencil
1
0
Fork 0
Code Issues Pull requests Actions 1 Packages Projects Releases Wiki Activity

fix(shell-web): Phase C gate — exception-safe listener reg + key coverage

Browse source 
Two fixes surfaced by the Phase C gate review iterations:

# C-gate R1 BLOCK: partial listener registration not exception-safe

Five `add_listener?` calls in `mount()` were sequenced via the
question-mark operator. If registration #K returned `Err`, the
K-1 already-landed listeners would never be unregistered:
WebShell never reaches `Ok(WebShell { ... })` so its `Drop`
never runs, and the partial `listeners` Vec drops without
calling `remove_event_listener_with_callback` first — the
browser-held DOM callbacks then outlive their Closures, which
is dangling-listener UB at the wasm-bindgen boundary.

Fix: wrap all 5 registrations in an inner closure that consumes
`&mut Vec<Listener>` and returns `Result<(), JsValue>`. On Err
we drain the partial vec and unregister each listener with the
same body Drop uses, then return the error. Comment cites the
codex finding inline.

# C-gate R1 CONCERN + R3 BLOCK: incomplete named-key coverage

`map_key_value` covered Enter/Escape/Tab/Space/Backspace/Delete/
Arrow{Up,Down,Left,Right} only. jian's `NamedKey` enum has 31
variants total — Home, End, PageUp, PageDown, F1..F12, Shift,
Control, Alt, Meta, and CapsLock were all falling through to
`Unidentified(<key>)`.

`map_key_code` similarly missed Home, End, PageUp, PageDown, and
the 8 modifier physical codes (ShiftLeft / ShiftRight / etc).

Fix: extended both tables. After the fix, every NamedKey jian
exposes round-trips through map_key_value, and every
non-Unknown KeyCode variant round-trips through map_key_code
(F1-F12 stay Unknown because jian's KeyCode does NOT have F-key
variants — NamedKey covers them; comment in keyboard.rs
explains).

# Tests added (3 new round-trip tests, 22 → 25)

- `keyboard_navigation_keys_mapped` — Home / End / PageUp /
  PageDown / CapsLock all map to Named(<variant>), not
  Unidentified.
- `keyboard_function_keys_mapped` — F1..F12 round-trip via a
  for-loop.
- `keyboard_modifier_keys_mapped_to_named_values` — Shift /
  Control / Alt / Meta with both Left and Right `code` variants
  assert both Named(<modifier>) on `key` and the matching
  KeyCode::{Mod}{Left,Right} on `code`.

CapsLock specifically was the Round 3 NO-GO discovery — the
only NamedKey variant the Round 1 fix missed; Round 4 added it
+ the round-trip tests above.

Verification:
- `cargo test -p openpencil-shell-web --test dom_event_mapping` —
  25/25 passing
- `cargo build -p openpencil-shell-web --target
  wasm32-unknown-unknown --features skia --release` — green
- `bash tools/check-wasm-bundle.sh` — PASS:
  - 0 env.* imports
  - 621 151 bytes gzip = 59% of 1 MiB ceiling (+0.6 KiB vs
    pre-fix; new key table entries are tiny)
- `bash tools/check-widget-boundary.sh` — PASS

Codex Phase C gate review: 5 rounds (gate-level, not commit-level).
Round 1 BLOCK + CONCERN, Round 2 sandbox-only BLOCK, Round 3
NO-GO (CapsLock + missing test coverage), Round 4 sandbox-only
BLOCK (source verdict clean), Round 5 GO with Phase B5 R2
precedent applied for sandbox-only build evidence.
This commit is contained in:
Kayshen-X 2026-05-09 21:52:00 +08:00
parent a84d1f30e3
commit 40dd271816
3 changed files with 237 additions and 93 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

48
crates/openpencil-shell-web/src/event/keyboard.rs
View file

@ -56,6 +56,34 @@ fn map_key_value(key: &str) -> KeyValue {
"ArrowDown" => KeyValue::Named(NamedKey::ArrowDown),
"ArrowLeft" => KeyValue::Named(NamedKey::ArrowLeft),
"ArrowRight" => KeyValue::Named(NamedKey::ArrowRight),
// Navigation extensions (codex Phase C gate CONCERN):
"Home" => KeyValue::Named(NamedKey::Home),
"End" => KeyValue::Named(NamedKey::End),
"PageUp" => KeyValue::Named(NamedKey::PageUp),
"PageDown" => KeyValue::Named(NamedKey::PageDown),
// Function keys F1-F12. jian's NamedKey covers them but
// KeyCode does not — `map_key_code` falls through to
// Unknown(code) for these on purpose.
"F1" => KeyValue::Named(NamedKey::F1),
"F2" => KeyValue::Named(NamedKey::F2),
"F3" => KeyValue::Named(NamedKey::F3),
"F4" => KeyValue::Named(NamedKey::F4),
"F5" => KeyValue::Named(NamedKey::F5),
"F6" => KeyValue::Named(NamedKey::F6),
"F7" => KeyValue::Named(NamedKey::F7),
"F8" => KeyValue::Named(NamedKey::F8),
"F9" => KeyValue::Named(NamedKey::F9),
"F10" => KeyValue::Named(NamedKey::F10),
"F11" => KeyValue::Named(NamedKey::F11),
"F12" => KeyValue::Named(NamedKey::F12),
// Modifier keys themselves (W3C `key` is just "Shift" etc;
// `code` is "ShiftLeft" / "ShiftRight" — handled in
// `map_key_code`).
"Shift" => KeyValue::Named(NamedKey::Shift),
"Control" => KeyValue::Named(NamedKey::Control),
"Alt" => KeyValue::Named(NamedKey::Alt),
"Meta" => KeyValue::Named(NamedKey::Meta),
"CapsLock" => KeyValue::Named(NamedKey::CapsLock),
_ => {
// `KeyValue::Char(char)` covers single-codepoint keys
// (e.g. "a", "1", "你"). Anything else (multi-codepoint
@ -118,6 +146,26 @@ fn map_key_code(code: &str) -> KeyCode {
"ArrowDown" => KeyCode::ArrowDown,
"ArrowLeft" => KeyCode::ArrowLeft,
"ArrowRight" => KeyCode::ArrowRight,
// Navigation extensions (codex Phase C gate CONCERN):
"Home" => KeyCode::Home,
"End" => KeyCode::End,
"PageUp" => KeyCode::PageUp,
"PageDown" => KeyCode::PageDown,
// Modifier physical codes — distinguish left vs right per
// jian KeyCode. Note: "MetaLeft" / "MetaRight" cover the
// Cmd key on macOS, the Win key on Windows, and the Super
// key on Linux (per Jian Modifiers::CMD).
"ShiftLeft" => KeyCode::ShiftLeft,
"ShiftRight" => KeyCode::ShiftRight,
"ControlLeft" => KeyCode::ControlLeft,
"ControlRight" => KeyCode::ControlRight,
"AltLeft" => KeyCode::AltLeft,
"AltRight" => KeyCode::AltRight,
"MetaLeft" => KeyCode::MetaLeft,
"MetaRight" => KeyCode::MetaRight,
// F1-F12 KeyCode variants don't exist in jian (NamedKey
// covers them); fall to Unknown(code) for now. Phase D+
// may upstream F-key codes if a widget needs them.
_ => KeyCode::Unknown(code.to_string()),
}
}

212
crates/openpencil-shell-web/src/lib.rs
View file

@ -240,100 +240,126 @@ pub fn mount(canvas_id: &str) -> Result<WebShell, JsValue> {
// which point the registration target gets parameterized.
let win_target: web_sys::EventTarget = window.clone().into();
// ----- keyboard: keydown + keyup → WidgetHost::apply_key -----
{
let inner_kd = inner.clone();
add_listener::<KeyboardEvent, _>(
&win_target,
"keydown",
&mut listeners,
move |evt: KeyboardEvent| {
let key_event = keyboard::map_keyboard_parts(
&evt.key(),
&evt.code(),
evt.location(),
evt.repeat(),
true, // pressed
modifiers_from_keyboard(&evt),
evt.is_composing(),
);
let mut inner = inner_kd.borrow_mut();
inner.host.apply_key(&key_event);
let _ = inner.repaint();
},
)?;
}
{
let inner_ku = inner.clone();
add_listener::<KeyboardEvent, _>(
&win_target,
"keyup",
&mut listeners,
move |evt: KeyboardEvent| {
let key_event = keyboard::map_keyboard_parts(
&evt.key(),
&evt.code(),
evt.location(),
evt.repeat(),
false, // released
modifiers_from_keyboard(&evt),
evt.is_composing(),
);
let mut inner = inner_ku.borrow_mut();
inner.host.apply_key(&key_event);
let _ = inner.repaint();
},
)?;
}
// Codex Phase C gate BLOCK: registering N listeners with `?`
// is NOT exception-safe — if registration #K fails, the
// partially-built `listeners` vec drops without our `Drop for
// WebShell` ever firing (we never reach the `Ok(WebShell {
// ... })` line), and the K-1 already-registered DOM callbacks
// outlive their wasm-bindgen Closures. Wrap all registrations
// in an inner closure that, on Err, drains the partial vec
// and unregisters everything we managed to land before
// surfacing the error. The unregister loop is the same one
// `Drop for WebShell` runs.
let registration: Result<(), JsValue> = (|listeners: &mut Vec<Listener>| {
// ----- keyboard: keydown + keyup → WidgetHost::apply_key -----
{
let inner_kd = inner.clone();
add_listener::<KeyboardEvent, _>(
&win_target,
"keydown",
listeners,
move |evt: KeyboardEvent| {
let key_event = keyboard::map_keyboard_parts(
&evt.key(),
&evt.code(),
evt.location(),
evt.repeat(),
true, // pressed
modifiers_from_keyboard(&evt),
evt.is_composing(),
);
let mut inner = inner_kd.borrow_mut();
inner.host.apply_key(&key_event);
let _ = inner.repaint();
},
)?;
}
{
let inner_ku = inner.clone();
add_listener::<KeyboardEvent, _>(
&win_target,
"keyup",
listeners,
move |evt: KeyboardEvent| {
let key_event = keyboard::map_keyboard_parts(
&evt.key(),
&evt.code(),
evt.location(),
evt.repeat(),
false, // released
modifiers_from_keyboard(&evt),
evt.is_composing(),
);
let mut inner = inner_ku.borrow_mut();
inner.host.apply_key(&key_event);
let _ = inner.repaint();
},
)?;
}
// ----- IME: compositionstart / update / end → WidgetHost::apply_ime -----
// Phase C1 ime mappers do the UTF-16→UTF-8 selection remap when
// the browser supplies an IME-highlighted segment via
// `getTargetRanges()`; current browsers expose that range via
// a method we cannot call on `CompositionEvent` directly through
// web-sys 0.3.94 without an extra raw `Reflect::get` shim.
// For Step 1b we forward `data` only; selection lands in Phase D
// alongside the DOM mirror that already needs Reflect::get.
{
let inner_cs = inner.clone();
add_listener::<CompositionEvent, _>(
&win_target,
"compositionstart",
&mut listeners,
move |_evt: CompositionEvent| {
let mut inner = inner_cs.borrow_mut();
inner.host.apply_ime(&ime::composition_start());
let _ = inner.repaint();
},
)?;
}
{
let inner_cu = inner.clone();
add_listener::<CompositionEvent, _>(
&win_target,
"compositionupdate",
&mut listeners,
move |evt: CompositionEvent| {
let text = evt.data().unwrap_or_default();
let mut inner = inner_cu.borrow_mut();
inner.host.apply_ime(&ime::composition_update(text, None));
let _ = inner.repaint();
},
)?;
}
{
let inner_ce = inner.clone();
add_listener::<CompositionEvent, _>(
&win_target,
"compositionend",
&mut listeners,
move |evt: CompositionEvent| {
let text = evt.data().unwrap_or_default();
let mut inner = inner_ce.borrow_mut();
inner.host.apply_ime(&ime::composition_end(text));
let _ = inner.repaint();
},
)?;
// ----- IME: compositionstart / update / end → WidgetHost::apply_ime -----
// Phase C1 ime mappers do the UTF-16→UTF-8 selection remap when
// the browser supplies an IME-highlighted segment via
// `getTargetRanges()`; current browsers expose that range via
// a method we cannot call on `CompositionEvent` directly through
// web-sys 0.3.94 without an extra raw `Reflect::get` shim.
// For Step 1b we forward `data` only; selection lands in Phase D
// alongside the DOM mirror that already needs Reflect::get.
{
let inner_cs = inner.clone();
add_listener::<CompositionEvent, _>(
&win_target,
"compositionstart",
listeners,
move |_evt: CompositionEvent| {
let mut inner = inner_cs.borrow_mut();
inner.host.apply_ime(&ime::composition_start());
let _ = inner.repaint();
},
)?;
}
{
let inner_cu = inner.clone();
add_listener::<CompositionEvent, _>(
&win_target,
"compositionupdate",
listeners,
move |evt: CompositionEvent| {
let text = evt.data().unwrap_or_default();
let mut inner = inner_cu.borrow_mut();
inner.host.apply_ime(&ime::composition_update(text, None));
let _ = inner.repaint();
},
)?;
}
{
let inner_ce = inner.clone();
add_listener::<CompositionEvent, _>(
&win_target,
"compositionend",
listeners,
move |evt: CompositionEvent| {
let text = evt.data().unwrap_or_default();
let mut inner = inner_ce.borrow_mut();
inner.host.apply_ime(&ime::composition_end(text));
let _ = inner.repaint();
},
)?;
}
Ok(())
})(&mut listeners);
if let Err(e) = registration {
// Unwind partial registration. Same body as `Drop for
// WebShell`; kept inline rather than refactored into a free
// function because the lifetimes only line up when the
// `Listener` vec is local to mount().
for l in listeners.drain(..) {
let _ = l
.target
.remove_event_listener_with_callback(l.name, l.closure.as_ref().unchecked_ref());
}
return Err(e);
}
Ok(WebShell { inner, listeners })

70
crates/openpencil-shell-web/tests/dom_event_mapping.rs
View file

@ -93,6 +93,76 @@ fn keyboard_unidentified_for_multi_char_keys() {
assert_eq!(event.code, KeyCode::Unknown("Pause".to_string()));
}
#[test]
fn keyboard_navigation_keys_mapped() {
// Codex Phase C gate Round 3 BLOCK: every NamedKey/KeyCode variant
// jian exposes must round-trip through the mapper rather than
// falling to Unidentified / Unknown(...).
for (k, expected) in [
("Home", NamedKey::Home),
("End", NamedKey::End),
("PageUp", NamedKey::PageUp),
("PageDown", NamedKey::PageDown),
("CapsLock", NamedKey::CapsLock),
] {
let event = map_keyboard_parts(k, k, 0, false, true, Modifiers::empty(), false);
assert_eq!(event.key, KeyValue::Named(expected), "key {k}");
}
}
#[test]
fn keyboard_function_keys_mapped() {
use openpencil_shell_core::NamedKey::*;
for (k, expected) in [
("F1", F1),
("F2", F2),
("F3", F3),
("F4", F4),
("F5", F5),
("F6", F6),
("F7", F7),
("F8", F8),
("F9", F9),
("F10", F10),
("F11", F11),
("F12", F12),
] {
let event = map_keyboard_parts(k, k, 0, false, true, Modifiers::empty(), false);
assert_eq!(event.key, KeyValue::Named(expected), "fn-key {k}");
}
}
#[test]
fn keyboard_modifier_keys_mapped_to_named_values() {
// W3C `key` for modifier keys themselves is just the bare name;
// `code` carries left/right ("ShiftLeft" etc) which map_key_code
// covers. Verify both halves round-trip.
for (k, c, expected_key, expected_code) in [
("Shift", "ShiftLeft", NamedKey::Shift, KeyCode::ShiftLeft),
("Shift", "ShiftRight", NamedKey::Shift, KeyCode::ShiftRight),
(
"Control",
"ControlLeft",
NamedKey::Control,
KeyCode::ControlLeft,
),
(
"Control",
"ControlRight",
NamedKey::Control,
KeyCode::ControlRight,
),
("Alt", "AltLeft", NamedKey::Alt, KeyCode::AltLeft),
("Alt", "AltRight", NamedKey::Alt, KeyCode::AltRight),
("Meta", "MetaLeft", NamedKey::Meta, KeyCode::MetaLeft),
("Meta", "MetaRight", NamedKey::Meta, KeyCode::MetaRight),
] {
let event = map_keyboard_parts(k, c, 0, false, true, Modifiers::empty(), false);
assert_eq!(event.key, KeyValue::Named(expected_key), "mod key {k}");
assert_eq!(event.code, expected_code, "mod code {c}");
}
}
// ---------------------------------------------------------------------
// IME
// ---------------------------------------------------------------------