From d5ce10f0bed8abceff386956ccc1075aaacc65d4 Mon Sep 17 00:00:00 2001 From: Filip Strajnar Date: Wed, 28 Oct 2026 21:42:17 +0100 Subject: [PATCH 1/6] M8: repair and library manifests check membership by set lookups (#1078) Repair re-listed unlisted pages with a scan of the page list per page, and reading a published library kept manifest styles, collections and variables with a scan of the snapshot's lists per entry: a file with many pages or a long manifest made both quadratic. Both now look ids up in a set built once. validate() already indexed its parent/child edges and page list. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01LvG6E2qDnjtMzfPVuEP7iH --- crates/doc/src/handoff_tests.rs | 20 ++++++++++++++++++++ crates/doc/src/lib.rs | 4 +++- crates/format/src/library.rs | 10 +++++++--- 3 files changed, 30 insertions(+), 4 deletions(-) diff --git a/crates/doc/src/handoff_tests.rs b/crates/doc/src/handoff_tests.rs index 79b97194..8a18151d 100644 --- a/crates/doc/src/handoff_tests.rs +++ b/crates/doc/src/handoff_tests.rs @@ -85,3 +85,23 @@ fn validate_is_linear_in_a_wide_parent() { doc.get_mut(page).unwrap().children.retain(|c| *c != kid); assert!(doc.validate().iter().any(|m| m.contains("not listed in parent"))); } + +/// Repair lists every unlisted page once, by set lookups rather than a scan of the page list per +/// page (#1078: a file with many pages made repair quadratic). +#[test] +fn repair_relists_many_unlisted_pages_once_each() { + let mut doc = Document::new("d"); + for i in 0..5_000 { + doc.add_page(&format!("P{i}")); + } + let all: Vec<_> = doc.pages.clone(); + // Half the pages fall off the list. + doc.pages.retain(|p| p.0 % 2 == 0); + doc.repair(); + let mut listed = doc.pages.clone(); + listed.sort_unstable(); + let mut want = all; + want.sort_unstable(); + assert_eq!(listed, want, "every page is listed again, once"); + assert!(doc.validate().is_empty()); +} diff --git a/crates/doc/src/lib.rs b/crates/doc/src/lib.rs index caf0c861..7d1f6928 100644 --- a/crates/doc/src/lib.rs +++ b/crates/doc/src/lib.rs @@ -1143,8 +1143,10 @@ impl Document { Arc::make_mut(&mut self.libraries).store = store; } self.pages.retain(|p| self.nodes.get(p).is_some_and(|n| n.is_page()) && Some(*p) != store); + // Unlisted pages join the list; a set keeps many pages from making this quadratic (#1078). + let mut listed: std::collections::BTreeSet = self.pages.iter().copied().collect(); for (k, n) in self.nodes.iter() { - if n.is_page() && !self.pages.contains(k) && Some(*k) != store { + if n.is_page() && Some(*k) != store && listed.insert(*k) { self.pages.push(*k); } } diff --git a/crates/format/src/library.rs b/crates/format/src/library.rs index daec05e2..12d38e94 100644 --- a/crates/format/src/library.rs +++ b/crates/format/src/library.rs @@ -90,9 +90,13 @@ pub(crate) fn read( } // Keep what the snapshot really holds; clip what people see. manifest.components.retain(|a| doc.node(a.id).is_some_and(|n| matches!(n.kind, NodeKind::Component { .. } | NodeKind::ComponentSet { .. }))); - manifest.styles.retain(|s| doc.styles.iter().any(|x| x.id == s.id)); - manifest.collections.retain(|c| doc.variables.collections.iter().any(|x| x.id == c.id)); - manifest.variables.retain(|v| doc.variables.variables.iter().any(|x| x.id == v.id && x.collection == v.collection)); + // Membership by set lookups: a long manifest against a large snapshot stays linear (#1078). + let styles: BTreeSet<&str> = doc.styles.iter().map(|x| x.id.as_str()).collect(); + let collections: BTreeSet<&str> = doc.variables.collections.iter().map(|x| x.id.as_str()).collect(); + let variables: BTreeSet<(&str, &str)> = doc.variables.variables.iter().map(|x| (x.id.as_str(), x.collection.as_str())).collect(); + manifest.styles.retain(|s| styles.contains(s.id.as_str())); + manifest.collections.retain(|c| collections.contains(c.id.as_str())); + manifest.variables.retain(|v| variables.contains(&(v.id.as_str(), v.collection.as_str()))); manifest.name = manifest.name.chars().take(MAX_NAME).collect(); manifest.description = manifest.description.chars().take(MAX_DESCRIPTION).collect(); Ok(Some(PublishedLibrary { manifest, doc: Arc::new(doc) })) From 8d7c09422420809ee5c9edbbce47b1189e097656 Mon Sep 17 00:00:00 2001 From: Filip Strajnar Date: Wed, 28 Oct 2026 21:42:25 +0100 Subject: [PATCH 2/6] M1: the stroke weight keeps room for its value beside the variable button (#1452) The stroke's position dropdown took half the row and the weight field what was left after the per-side and Advanced buttons: about 40 pt at the panel's default width, so the variable button sat on the weight's icon and hid its value, and the row ran 3 pt past the panel's edge. The position and the weight now share what the buttons leave: the panel's two columns for layers without per-side strokes, and about 66 pt each with them. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01LvG6E2qDnjtMzfPVuEP7iH --- crates/ui-egui/src/lib.rs | 2 + crates/ui-egui/src/panels/right/paints.rs | 13 ++-- crates/ui-egui/src/stroke_row_tests.rs | 82 +++++++++++++++++++++++ 3 files changed, 92 insertions(+), 5 deletions(-) create mode 100644 crates/ui-egui/src/stroke_row_tests.rs diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index 4c1ed337..ef55faf8 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -187,6 +187,8 @@ mod shortcut_keys_tests; #[cfg(test)] mod shortcuts_tests; #[cfg(test)] +mod stroke_row_tests; +#[cfg(test)] mod style_folder_rename_tests; #[cfg(test)] mod text_enter_tests; diff --git a/crates/ui-egui/src/panels/right/paints.rs b/crates/ui-egui/src/panels/right/paints.rs index b947e296..182116fe 100644 --- a/crates/ui-egui/src/panels/right/paints.rs +++ b/crates/ui-egui/src/panels/right/paints.rs @@ -348,7 +348,12 @@ pub(super) fn paints(app: &mut FigApp, ui: &mut Ui, doc: &Document, sel: &[NodeI if !fills && !empty { let weight = common(doc, sel, |n| n.stroke.weight); let align = common(doc, sel, |n| n.stroke.align); - let half = (ui.available_width() - PAD * 2.0 - 8.0) / 2.0; + let sided = sel.iter().all(|i| doc.node(*i).is_some_and(|n| n.kind.is_frame_like() || matches!(n.kind, NodeKind::Rectangle))); + // The position and the weight share what the buttons (Advanced, and per-side strokes for + // layers with sides) leave: halving the whole row left the weight too narrow for its value + // beside the variable button and ran the row past the panel's edge (#1452). + let buttons = if sided { 2.0 } else { 1.0 } * (24.0 + 8.0); + let field_w = ((ui.available_width() - PAD * 2.0 - 8.0 - buttons) / 2.0).max(40.0); row(ui, 32.0, |ui| { let resp = widgets::dropdown( ui, @@ -358,7 +363,7 @@ pub(super) fn paints(app: &mut FigApp, ui: &mut Ui, doc: &Document, sel: &[NodeI Some(StrokeAlign::Outside) => tr("Outside"), None => tr("Mixed"), }, - half, + field_w, None, ); menu_popup(&resp).show(|ui| { @@ -369,8 +374,6 @@ pub(super) fn paints(app: &mut FigApp, ui: &mut Ui, doc: &Document, sel: &[NodeI } } }); - let sided = sel.iter().all(|i| doc.node(*i).is_some_and(|n| n.kind.is_frame_like() || matches!(n.kind, NodeKind::Rectangle))); - let weight_w = half - if sided { 60.0 } else { 32.0 }; let e = crate::variable_ui::number_field_mixed( app, ui, @@ -380,7 +383,7 @@ pub(super) fn paints(app: &mut FigApp, ui: &mut Ui, doc: &Document, sel: &[NodeI Lead::Icon(Icon::Menu, tr("Stroke weight")), weight, "", - weight_w, + field_w, ); app.field_of(&ids, "strokeWeight", e, |n| n.stroke.weight, |v| json!(v.max(0.0))); if sided { diff --git a/crates/ui-egui/src/stroke_row_tests.rs b/crates/ui-egui/src/stroke_row_tests.rs new file mode 100644 index 00000000..18ed7a00 --- /dev/null +++ b/crates/ui-egui/src/stroke_row_tests.rs @@ -0,0 +1,82 @@ +//! The stroke's position / weight row in the Design panel, driven headless at the panel's default +//! width: the weight field keeps room for its value beside the variable button, whose click opens +//! the variable picker, and the row's buttons stay inside the panel (#1452: the weight field was +//! so narrow that the variable button sat on its icon and hid the value). + +use egui::accesskit::Role; +use egui::{Id, Pos2, Rect}; +use figcraft_engine::Session; +use serde_json::json; + +use crate::layers_drag_tests::Ui; + +/// A stroked rectangle (`rect`) or ellipse, selected. +fn scene(kind: &str) -> Ui { + let mut s = Session::new(); + let made = s.execute(&format!("shape.{kind}"), &json!({"x": 0, "y": 0, "width": 200, "height": 120})).unwrap(); + let id = made["id"].as_u64().unwrap(); + s.execute("stroke.add", &json!({"ids": [id]})).unwrap(); + s.execute("select.set", &json!({"ids": [id]})).unwrap(); + let mut u = Ui::new(s); + u.frame(vec![]); + u +} + +/// The area of part `part` ("lead", "field" or "bind") of the stroke weight field. +fn weight(u: &Ui, part: &str) -> Rect { + u.ctx.read_response(Id::unique("stroke_w").with(part)).unwrap_or_else(|| panic!("the weight field's {part} shows")).rect +} + +/// The bounds of the button named `label`, found in the AccessKit tree. +fn named_button(u: &mut Ui, label: &str) -> Rect { + u.ctx.enable_accesskit(); + u.frame(vec![]); + let out = u.frame_output(vec![]); + let nodes = out.platform_output.accesskit_update.expect("an AccessKit update").nodes; + let n = nodes.iter().map(|(_, n)| n).find(|n| n.role() == Role::Button && n.label() == Some(label)).expect(label); + let b = n.bounds().expect("bounds"); + Rect::from_min_max(Pos2::new(b.x0 as f32, b.y0 as f32), Pos2::new(b.x1 as f32, b.y1 as f32)) +} + +/// The variable button clears the weight's icon and leaves the value room for a few digits, for +/// layers with per-side strokes (one more button in the row) and without. +#[test] +fn weight_field_keeps_its_value_beside_the_variable_button() { + for kind in ["rectangle", "ellipse"] { + let u = scene(kind); + let (lead, field, bind) = (weight(&u, "lead"), weight(&u, "field"), weight(&u, "bind")); + assert!(bind.min.x >= lead.max.x + 4.0, "{kind}: the variable button {bind:?} sits on the icon {lead:?}"); + assert!(bind.min.x - field.min.x >= 16.0, "{kind}: no room for the value left of the variable button in {field:?} ({bind:?})"); + } +} + +/// The row's last button (Advanced) stays inside the panel's padding, in line with the panel's +/// icon column, also with the per-side button beside the weight. +#[test] +fn stroke_row_buttons_stay_inside_the_panel() { + for kind in ["rectangle", "ellipse"] { + let mut u = scene(kind); + let advanced = named_button(&mut u, "Advanced stroke"); + let panel_right = u.ctx.content_rect().max.x - crate::widgets::PAD; + assert!(advanced.max.x <= panel_right + 0.5, "{kind}: Advanced at {advanced:?} runs past the panel's edge {panel_right}"); + if kind == "rectangle" { + let sides = named_button(&mut u, "Individual strokes"); + assert!(sides.min.x >= weight(&u, "field").max.x, "{kind}: the per-side button {sides:?} overlaps the weight"); + assert!(sides.max.x <= advanced.min.x, "{kind}: the per-side button {sides:?} overlaps Advanced {advanced:?}"); + } + } +} + +/// A click on the variable button opens the variable picker for the weight. +#[test] +fn variable_button_opens_the_picker() { + let mut u = scene("rectangle"); + let field = weight(&u, "field"); + let at = weight(&u, "bind").center(); + // The button shows while the pointer is over the field. + u.frame(vec![egui::Event::PointerMoved(field.center())]); + u.button(at, true); + u.button(at, false); + u.frame(vec![]); + assert!(crate::variable_ui::picker_open(&u.ctx), "the variable picker opened"); +} From 62c69ba7ab1bed1e6cbf941a6aeeb7734d0e4465 Mon Sep 17 00:00:00 2001 From: Filip Strajnar Date: Wed, 28 Oct 2026 21:59:47 +0100 Subject: [PATCH 3/6] M8: normalising after a command runs under the panic guard (part of #1085) Session::execute ran a command under guard::run but normalised what it changed afterwards, outside it: a panic in normalisation escaped execute instead of becoming EngineError::Internal with the edit rolled back. Inside a batch it unwound through Session::grouped, which then never restored the undo limit it had lifted or ended the group. Normalising is now the last step of the guarded result chain, so a panic there fails the command like one in the command itself (and a batch step fails like any other failed step); live text previews normalise under the guard too. The other half of #1085, restoring grouped()'s state when its closure panics, is left alone: #1419 rewrites that function. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01LvG6E2qDnjtMzfPVuEP7iH --- crates/engine/src/canvas/textpreview.rs | 9 ++- crates/engine/src/derived.rs | 19 ++++++ crates/engine/src/lib.rs | 2 + crates/engine/src/normalize_guard_tests.rs | 72 ++++++++++++++++++++++ crates/engine/src/session/exec.rs | 31 +++++++--- 5 files changed, 123 insertions(+), 10 deletions(-) create mode 100644 crates/engine/src/normalize_guard_tests.rs diff --git a/crates/engine/src/canvas/textpreview.rs b/crates/engine/src/canvas/textpreview.rs index da101d5e..07a09a15 100644 --- a/crates/engine/src/canvas/textpreview.rs +++ b/crates/engine/src/canvas/textpreview.rs @@ -39,7 +39,14 @@ pub fn show(s: &mut Session, command: &str, params: &Value) -> Result<()> { ds.text_edit = text_edit; match result { Ok(_) if !Arc::ptr_eq(&ds.doc, &base) => { - normalize(ds.doc_mut(), &base); + // A panic while normalising shows no preview (#1085). + if let Err(e) = guard::run(command, || { + normalize(ds.doc_mut(), &base); + Ok(()) + }) { + ds.doc = base; + return Err(e); + } ds.text_preview = Some(base); ds.revision += 1; Ok(()) diff --git a/crates/engine/src/derived.rs b/crates/engine/src/derived.rs index b6a001b8..c618269f 100644 --- a/crates/engine/src/derived.rs +++ b/crates/engine/src/derived.rs @@ -10,9 +10,28 @@ pub fn changed_nodes(doc: &Document, before: &Document) -> Vec { .collect() } +#[cfg(test)] +thread_local! { + /// Tests: normalising panics, standing for a bug in it (a command's edit is rolled back). + pub(crate) static PANIC: std::cell::Cell = const { std::cell::Cell::new(false) }; +} + +/// Tests: panic when [`PANIC`] is set, as a bug in normalising would. +#[cfg(test)] +fn test_panic() { + if PANIC.with(std::cell::Cell::get) { + panic!("normalize: test panic"); + } +} + +/// Shipped builds: nothing (the test hook compiles away). +#[cfg(not(test))] +fn test_panic() {} + /// Keep derived state consistent after edits: text boxes resize to their content, groups wrap /// their children, instances follow their main components, auto layout re-flows. pub fn normalize(doc: &mut Document, before: &Document) { + test_panic(); // Only what was touched since the last normalisation (edited, added or removed). let mut touched: BTreeSet = doc.take_dirty(); // A variable or style edit changes no layer directly, but bound and linked layers must follow it. diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index ea135ca1..beaf989c 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -346,6 +346,8 @@ mod nested_override_tests; #[cfg(test)] mod node_set_refusals_tests; #[cfg(test)] +mod normalize_guard_tests; +#[cfg(test)] mod online_tests; #[cfg(test)] mod open_issues_tests; diff --git a/crates/engine/src/normalize_guard_tests.rs b/crates/engine/src/normalize_guard_tests.rs new file mode 100644 index 00000000..c4ad7c08 --- /dev/null +++ b/crates/engine/src/normalize_guard_tests.rs @@ -0,0 +1,72 @@ +//! A panic while normalising what a command changed is caught like one in the command itself +//! (#1085): the command fails with `EngineError::Internal`, the document is as it was, no undo step +//! is pushed, and the session keeps working. + +use serde_json::json; + +use crate::{EngineError, Session, derived}; + +/// Run `f` with normalisation panicking. +fn panicking(f: impl FnOnce() -> T) -> T { + derived::PANIC.with(|p| p.set(true)); + let out = f(); + derived::PANIC.with(|p| p.set(false)); + out +} + +/// The layer id a creating command returned. +fn id(v: &serde_json::Value) -> u64 { + v["id"].as_u64().unwrap() +} + +/// A command whose edit panics while normalising fails with `EngineError::Internal`, leaves the +/// document and the undo history as they were, and the same command then runs as usual. +#[test] +fn a_panic_while_normalising_rolls_the_command_back() { + let mut s = Session::new(); + let r = id(&s.execute("shape.rectangle", &json!({"x": 0, "y": 0, "width": 10, "height": 10})).unwrap()); + let before = s.doc().unwrap().clone(); + let undo = s.undo_depth(); + let result = panicking(|| s.execute("node.set", &json!({"ids": [r], "props": {"name": "Renamed"}}))); + assert!(matches!(result, Err(EngineError::Internal { .. })), "{result:?}"); + assert_eq!(s.doc().unwrap(), &before, "the edit is rolled back"); + assert_eq!(s.undo_depth(), undo, "no undo step for the failed command"); + // The next command runs as usual. + s.execute("node.set", &json!({"ids": [r], "props": {"name": "Renamed"}})).unwrap(); + assert_eq!(s.doc().unwrap().node(figcraft_doc::NodeId(r)).unwrap().name, "Renamed"); + assert_eq!(s.undo_depth().0, undo.0 + 1); +} + +/// The step fails like any other failed step: the batch's group restores what it set aside for +/// its steps (the history limit, the group depth), which a panic unwinding through it skipped. +#[test] +fn a_panic_while_normalising_a_batch_step_rolls_the_batch_back() { + let mut s = Session::new(); + let before = s.doc().unwrap().clone(); + let undo = s.undo_depth(); + let limit = s.ds().unwrap().history.limit; + let result = panicking(|| { + s.execute("command.batch", &json!({"commands": [{"command": "shape.rectangle", "params": {"x": 0, "y": 0, "width": 10, "height": 10}}]})) + }); + assert!(result.is_err(), "{result:?}"); + assert_eq!(s.doc().unwrap(), &before); + assert_eq!(s.undo_depth(), undo); + assert_eq!(s.ds().unwrap().history.limit, limit, "the undo limit is back"); + assert_eq!(s.group_depth, 0, "the group ended"); + s.execute("shape.rectangle", &json!({"x": 0, "y": 0, "width": 10, "height": 10})).unwrap(); +} + +/// A live text preview whose normalising panics fails and shows nothing: the document is as it +/// was, and the same preview then shows. +#[test] +fn a_panic_while_normalising_a_text_preview_shows_none() { + let mut s = Session::new(); + let t = id(&s.execute("text.create", &json!({"x": 0, "y": 0, "text": "Preview"})).unwrap()); + let before = s.doc().unwrap().clone(); + let result = panicking(|| s.execute("text.preview", &json!({"command": "text.style", "params": {"ids": [t], "fontSize": 32}}))); + assert!(matches!(result, Err(EngineError::Internal { .. })), "{result:?}"); + assert_eq!(s.doc().unwrap(), &before); + assert!(!crate::textpreview::active(&s), "no preview shows"); + s.execute("text.preview", &json!({"command": "text.style", "params": {"ids": [t], "fontSize": 32}})).unwrap(); + assert!(crate::textpreview::active(&s)); +} diff --git a/crates/engine/src/session/exec.rs b/crates/engine/src/session/exec.rs index 00798b2c..f5743d5d 100644 --- a/crates/engine/src/session/exec.rs +++ b/crates/engine/src/session/exec.rs @@ -51,7 +51,14 @@ impl Session { .and_then(|v| self.check_instance_structure(id, &before).map(|()| v)) .and_then(|v| self.check_preview(id, &before).map(|()| v)) .and_then(|v| self.check_live_access(id, &before).map(|()| v)) - .and_then(|v| self.check_finite_geometry(id, &before).map(|()| v)); + .and_then(|v| self.check_finite_geometry(id, &before).map(|()| v)) + // Normalising is part of the command: a panic in it rolls the edit back too (#1085). + .and_then(|v| { + guard::run(id, || { + self.normalize_edit(spec, &before); + Ok(v) + }) + }); match &result { Ok(_) => { self.after_command(spec, before); @@ -141,7 +148,19 @@ impl Session { .map_err(|m| EngineError::Disabled(id.to_string(), m)) } - /// Bookkeeping after a successful command: normalise what changed, push undo, bump revision. + /// Normalise what a command changed in the document it ran on (text boxes, groups, instances, + /// auto layout…). + fn normalize_edit(&mut self, spec: &CommandSpec, before: &Option<(Arc, Vec, NodeId, u64)>) { + let Some((bdoc, _, _, uid)) = before else { return }; + let Some(ds) = self.docs.iter_mut().find(|d| d.uid == *uid) else { return }; + // Saving may adopt the chosen filename as the document title. The encoded snapshot is + // already final; normalising it again would detach it from the saved baseline. + if !Arc::ptr_eq(&ds.doc, bdoc) && !matches!(spec.id, "file.save" | "file.saveAs") { + normalize(ds.doc_mut(), bdoc); + } + } + + /// Bookkeeping after a successful command (normalised already): push undo, bump revision. fn after_command(&mut self, spec: &CommandSpec, before: Option<(Arc, Vec, NodeId, u64)>) { let Some((bdoc, bsel, bpage, uid)) = before else { if let Ok(ds) = self.ds_mut() { @@ -150,13 +169,7 @@ impl Session { return; }; let Some(ds) = self.docs.iter_mut().find(|d| d.uid == uid) else { return }; - let changed = !Arc::ptr_eq(&ds.doc, &bdoc); - // Saving may adopt the chosen filename as the document title. The encoded snapshot is - // already final; normalising it again would detach it from the saved baseline. - if changed && !matches!(spec.id, "file.save" | "file.saveAs") { - normalize(ds.doc_mut(), &bdoc); - } - let edit = changed && ds.interaction.is_none(); + let edit = !Arc::ptr_eq(&ds.doc, &bdoc) && ds.interaction.is_none(); ds.clean_selection(); ds.revision += 1; if edit { From 652bf35b196624c9881c797b986b5be4c90f9a90 Mon Sep 17 00:00:00 2001 From: Filip Strajnar Date: Wed, 28 Oct 2026 21:58:16 +0100 Subject: [PATCH 4/6] M1: page ruler guides survive save and reopen (#1476) A page kept its ruler guides in `NodeKind::Page`, and since frame guides (#70) every layer also has its own `guides` field. Both are written as `guides`, and on reading the layer's own field takes the key, so: - a reopened page's guides landed in the layer field and were not drawn; - saving that page wrote `guides` twice (the page kind's list, always written, then the layer's), and the file no longer opened ("duplicate field `guides`"). Every layer, pages included, now keeps its guides in the one layer field; the key in the file is unchanged, so existing files open with their guides. The field reads every `guides` list a layer has, so files already saved with the key twice open again with both lists, in file order. This also covers collab `Create` payloads from older peers. Regressions: JSON round trips of page and frame guides, a page with two lists, a bad list still refused (doc); save, reopen, save, reopen through `.figcraft` and a document.json with the key twice (format). Checked in the app over the control channel: guides drawn after two save/reopen cycles, and a file with the key twice opens with all its guides. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01LvG6Dyzof21BCrUsByhx2w --- ROADMAP.md | 4 +- crates/doc/src/client_id_tests.rs | 4 +- crates/doc/src/guides_tests.rs | 61 +++++++++++++++++ crates/doc/src/lib.rs | 11 ++-- crates/doc/src/library.rs | 2 +- crates/doc/src/node.rs | 91 ++++++++++++++++++++++---- crates/engine/src/cmd/page.rs | 8 ++- crates/engine/src/tests.rs | 6 +- crates/engine/src/theme_pages_tests.rs | 11 +++- crates/format/src/fig/export/layer.rs | 4 +- crates/format/src/guides_tests.rs | 45 +++++++++++++ crates/format/src/lib.rs | 2 + 12 files changed, 215 insertions(+), 34 deletions(-) create mode 100644 crates/doc/src/guides_tests.rs create mode 100644 crates/format/src/guides_tests.rs diff --git a/ROADMAP.md b/ROADMAP.md index 74a617f3..662c2e3d 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -2,7 +2,7 @@ **Stage: alpha** · next: beta, ~26 points and ~162–292 h away (one Opus 5.5 agent; ~33–67 h wall-clock with 5 agents) -> **Last reviewed:** 2026-10-10 · **Last updated:** 2026-10-26 · **Change:** minor (colour picker LCH and OKLCH models; non-ASCII layer names transliterate in code export (#1235); branches in live sessions and branch history entries, gap 17; plugin widgets take text input and a property menu (plugin API 4); boards: templates and tables; boards: a shared timer and voting sessions; boards: the dot grid, connector ends dragged to re-attach, curved connectors; Installed weights of an embedded font family (#1246); `.sketch` import, best effort (Sketch 48 and newer); CJK line breaking: JIS X 4051's line-start and line-end sets completed with the halfwidth small kana, the halfwidth and spacing (semi-)voiced sound marks, the iteration marks 〱–〵 and the halfwidth parentheses, with behaviour cases for each; Adobe Fonts found on macOS (#298); egui 0.37 hidden-capture deadline contract; Dev Mode measurements kept on the canvas and a read-only layers panel; boards: a board area row and progress entry; community fixes in #507, #510, #529 and #530: auto-save, a text box by dragging, batch step references, removing someone from a live session, Russian and Turkish catalogs reviewed, Czech and Indonesian drafts, the Japanese draft completed, zoom previews, the web clipboard, Alt-drag snapping and Cmd+D repeat, recovery copies, crash and rollback fixes, colour emoji, `.fig` import fixes; earlier changes are in the revision history below) · **Target:** Figma Design (desktop app 126.10.7) +> **Last reviewed:** 2026-10-10 · **Last updated:** 2026-10-28 · **Change:** minor (page ruler guides survive save and reopen (#1476); colour picker LCH and OKLCH models; non-ASCII layer names transliterate in code export (#1235); branches in live sessions and branch history entries, gap 17; plugin widgets take text input and a property menu (plugin API 4); boards: templates and tables; boards: a shared timer and voting sessions; boards: the dot grid, connector ends dragged to re-attach, curved connectors; Installed weights of an embedded font family (#1246); `.sketch` import, best effort (Sketch 48 and newer); CJK line breaking: JIS X 4051's line-start and line-end sets completed with the halfwidth small kana, the halfwidth and spacing (semi-)voiced sound marks, the iteration marks 〱–〵 and the halfwidth parentheses, with behaviour cases for each; Adobe Fonts found on macOS (#298); egui 0.37 hidden-capture deadline contract; Dev Mode measurements kept on the canvas and a read-only layers panel; boards: a board area row and progress entry; community fixes in #507, #510, #529 and #530: auto-save, a text box by dragging, batch step references, removing someone from a live session, Russian and Turkish catalogs reviewed, Czech and Indonesian drafts, the Japanese draft completed, zoom previews, the web clipboard, Alt-drag snapping and Cmd+D repeat, recovery copies, crash and rollback fixes, colour emoji, `.fig` import fixes; earlier changes are in the revision history below) · **Target:** Figma Design (desktop app 126.10.7) FigCraft is a clean-room, open-source, pure-Rust reimplementation of the Figma design workflow, native on macOS, Windows, Linux and FreeBSD and in the browser, fully drivable by agents through @@ -132,6 +132,7 @@ Ranked; detail and milestones in [`docs/roadmap.md`](docs/roadmap.md). ## Progress log +- **2026-10-28** · M1, #1476: page ruler guides survive save and reopen. A page kept its guides in the page kind while the layer's own `guides` field (frame guides, #70) took that key when a file was read, so a reopened page's guides vanished, and saving it wrote `guides` twice, which then refused to open. Every layer now keeps its guides in the one field, and a page with two `guides` lists opens with both. Regressions in the doc and format crates. Numbers unchanged. - **2026-10-26** · Non-ASCII layer names transliterate in code export (M6, #1235): `class_name` runs the name through `deunicode` before its ASCII filter, so 로그인 화면 → `rogeuin-hwamyeon`, кнопка → `knopka`, über → `uber` in the HTML/CSS/React/Leptos class names, component roots and `data-*` variant attributes (SwiftUI/Compose type names unchanged); a name left with nothing (symbols only) takes the node's kind (`.text`, `.frame`) instead of `layer-`, and the 40-character cut lands on a word boundary. New dependency: deunicode (pure Rust tables, zero deps, BSD-3-Clause, the `slug` crate's base). `html_codegen_tests`. Numbers unchanged. - **2026-10-25** · egui 0.37 capture lifecycle: screenshots with hidden or unknown visibility queue, request a hidden paint, and expire after 60 seconds if no capture arrives (headless-tested contract). Logic-only edits, saves and inspection continue while hidden. Native visible GUI and forced hidden-window paint, browser and Wayland behavior remain unqualified; parity percentages are unchanged. - **2026-10-24** · Open, Save and the folder dialogs start in the folder of the last file or folder picked in the run (#1290): the Linux portal does not remember it between dialogs as the macOS and Windows panels do. A folder deleted since falls back to the system's choice. No numbers change. @@ -348,6 +349,7 @@ Ranked; detail and milestones in [`docs/roadmap.md`](docs/roadmap.md). | Date | Change | Summary | |---|---|---| +| 2026-10-28 | minor | M1, #1476: page ruler guides survive save and reopen; files written with a page's `guides` key twice open again with both lists. Numbers unchanged. | | 2026-10-26 | minor | Non-ASCII layer names transliterate in code export (M6, #1235): `class_name` transliterates through the new `deunicode` dependency (pure Rust tables, zero deps, BSD-3-Clause), so Korean, Cyrillic, Latin-diacritic and CJK names become readable ASCII classes (`rogeuin-hwamyeon`, `knopka`, `uber`, `an-niu`) in HTML/CSS/React/Leptos output, component roots and `data-*` variant attributes; symbols-only names take the node's kind (`.text`) instead of `layer-`, and the 40-character cut lands on a word boundary. `html_codegen_tests` updated and extended. Numbers unchanged | | 2026-10-25 | minor | egui 0.37 capture lifecycle: screenshots with hidden or unknown visibility queue, request a hidden paint, and expire after 60 seconds if no capture arrives (headless-tested contract). Logic-only edits, saves and inspection continue while hidden. Native visible GUI and forced hidden-window paint, browser and Wayland behavior remain unqualified; parity percentages are unchanged. | | 2026-10-24 | minor | Server memory (#1134): when the last person leaves a file nobody edited, figcraft-server keeps the client ids it issued in the log instead of rewriting the snapshot (compaction still follows `compact_after`/`compact_mb` and edits), and a snapshot is encoded straight into its file buffer, without a second copy of the body; the file format is unchanged. Numbers unchanged | diff --git a/crates/doc/src/client_id_tests.rs b/crates/doc/src/client_id_tests.rs index 520895fb..94b676c2 100644 --- a/crates/doc/src/client_id_tests.rs +++ b/crates/doc/src/client_id_tests.rs @@ -104,13 +104,15 @@ fn place_keeps_the_local_transform_and_refuses_bad_moves() { assert!(d.validate().is_empty()); } +/// A page-less document takes pages inserted with their own ids, refuses a duplicate or a layer +/// that isn't a page, and repairs into a file with a page. #[test] fn pages_can_be_inserted_onto_a_page_less_base() { let mut d = Document::without_pages("snapshot"); assert!(d.pages.is_empty() && d.node_count() == 0); let p = Node::new( NodeId(C3 | 1), - NodeKind::Page { background: figcraft_color::Color::DEFAULT_PAGE, flows: vec![], prototype: Default::default(), guides: vec![] }, + NodeKind::Page { background: figcraft_color::Color::DEFAULT_PAGE, flows: vec![], prototype: Default::default() }, "P", 0.0, 0.0, diff --git a/crates/doc/src/guides_tests.rs b/crates/doc/src/guides_tests.rs new file mode 100644 index 00000000..82926e5d --- /dev/null +++ b/crates/doc/src/guides_tests.rs @@ -0,0 +1,61 @@ +//! Ruler guides survive a save and reopen (#1476). + +use crate::{Document, Guide, Node, NodeKind}; + +/// A guide: vertical (at an x offset) or horizontal (at a y offset). +fn guide(vertical: bool, offset: f64) -> Guide { + Guide { vertical, offset } +} + +/// A page's guides are written once and read back onto the page. +#[test] +fn page_guides_round_trip_through_json() { + let mut d = Document::new("a"); + let page = d.first_page().unwrap(); + d.node_mut(page).unwrap().guides_mut().push(guide(true, 120.0)); + let json = serde_json::to_string(d.node(page).unwrap()).unwrap(); + assert_eq!(json.matches("\"guides\"").count(), 1, "{json}"); + let back: Node = serde_json::from_str(&json).unwrap(); + assert_eq!(back.guides(), &[guide(true, 120.0)]); + assert_eq!(back, *d.node(page).unwrap()); +} + +/// A frame's guides round trip too, and a layer without guides writes no `guides` key. +#[test] +fn frame_guides_round_trip_and_none_are_left_out() { + let mut d = Document::new("a"); + let page = d.first_page().unwrap(); + let frame = d.create(page, NodeKind::Frame, "F", 0.0, 0.0, 100.0, 100.0).unwrap(); + for id in [page, frame] { + let json = serde_json::to_value(d.node(id).unwrap()).unwrap(); + assert!(json.get("guides").is_none(), "{json}"); + } + d.node_mut(frame).unwrap().guides_mut().push(guide(false, 40.0)); + let json = serde_json::to_string(d.node(frame).unwrap()).unwrap(); + assert_eq!(json.matches("\"guides\"").count(), 1, "{json}"); + let back: Node = serde_json::from_str(&json).unwrap(); + assert_eq!(back, *d.node(frame).unwrap()); +} + +/// A page saved with two `guides` keys reads both lists, in file order, and writes them once. +#[test] +fn a_page_written_with_two_guides_lists_opens_with_both() { + // How a page was saved after its file had been reopened: the page kind's list (guides added + // since), then the layer's (the ones the file had). + let json = r#"{"id": 1, "name": "Page 1", "type": "page", "guides": [{"vertical": true, "offset": 5.0}], + "width": 0.0, "height": 0.0, "guides": [{"vertical": false, "offset": 7.0}]}"#; + let n: Node = serde_json::from_str(json).unwrap(); + assert!(matches!(n.kind, NodeKind::Page { .. })); + assert_eq!(n.guides(), &[guide(true, 5.0), guide(false, 7.0)]); + let again: Node = serde_json::from_str(&serde_json::to_string(&n).unwrap()).unwrap(); + assert_eq!(again.guides(), n.guides()); +} + +/// A `guides` value that isn't a list of guides is still refused. +#[test] +fn a_bad_guides_list_is_still_an_error() { + for bad in [r#"3"#, r#"[{"vertical": "no"}]"#, r#"{"offset": 1}"#] { + let json = format!(r#"{{"id": 1, "name": "F", "type": "frame", "width": 1.0, "height": 1.0, "guides": {bad}}}"#); + assert!(serde_json::from_str::(&json).is_err(), "{bad}"); + } +} diff --git a/crates/doc/src/lib.rs b/crates/doc/src/lib.rs index caf0c861..1214e3cf 100644 --- a/crates/doc/src/lib.rs +++ b/crates/doc/src/lib.rs @@ -407,13 +407,8 @@ impl Document { /// Add a page at the end. pub fn add_page(&mut self, name: &str) -> NodeId { let id = self.alloc_id(); - let node = Node::new( - id, - NodeKind::Page { background: Color::DEFAULT_PAGE, flows: Vec::new(), prototype: Default::default(), guides: Vec::new() }, - name, - 0.0, - 0.0, - ); + let node = + Node::new(id, NodeKind::Page { background: Color::DEFAULT_PAGE, flows: Vec::new(), prototype: Default::default() }, name, 0.0, 0.0); self.nodes.insert(id, Arc::new(node)); self.dirty.touch(id); self.pages.push(id); @@ -1258,6 +1253,8 @@ mod diff_tests; #[cfg(test)] mod effects_tests; #[cfg(test)] +mod guides_tests; +#[cfg(test)] mod handoff_tests; #[cfg(test)] mod ids_tests; diff --git a/crates/doc/src/library.rs b/crates/doc/src/library.rs index 35994bb0..fe53ec29 100644 --- a/crates/doc/src/library.rs +++ b/crates/doc/src/library.rs @@ -260,7 +260,7 @@ impl Document { let id = self.alloc_id(); let node = Node::new( id, - NodeKind::Page { background: figcraft_color::Color::DEFAULT_PAGE, flows: Vec::new(), prototype: Default::default(), guides: Vec::new() }, + NodeKind::Page { background: figcraft_color::Color::DEFAULT_PAGE, flows: Vec::new(), prototype: Default::default() }, STORE_NAME, 0.0, 0.0, diff --git a/crates/doc/src/node.rs b/crates/doc/src/node.rs index 3146047a..bcf8f09d 100644 --- a/crates/doc/src/node.rs +++ b/crates/doc/src/node.rs @@ -66,6 +66,9 @@ pub enum PropDef { } /// What a node is, with per-type data. +// Text sets the size; the lint counts the gap to the next largest variant, which grew only because +// Page's guides moved to the node (#1476). +#[allow(clippy::large_enum_variant)] #[derive(Clone, Debug, PartialEq, Serialize, Deserialize)] #[serde(tag = "type", rename_all = "camelCase")] pub enum NodeKind { @@ -76,8 +79,6 @@ pub enum NodeKind { flows: Vec, #[serde(default)] prototype: crate::proto::PrototypeSettings, - #[serde(default)] - guides: Vec, }, Frame, Group, @@ -472,8 +473,8 @@ pub struct Node { pub mask_type: MaskType, #[serde(default, skip_serializing_if = "is_default")] pub grids: Vec, - /// A frame's ruler guides, in its own coordinates (a page's are in [`NodeKind::Page`]). - #[serde(default, skip_serializing_if = "is_default")] + /// A page's or frame's ruler guides, in its own coordinates (see [`guides_key`]). + #[serde(flatten, with = "guides_key")] pub guides: Vec, #[serde(default, skip_serializing_if = "is_default")] pub exports: Vec, @@ -578,6 +579,77 @@ fn is_identity(v: &Affine) -> bool { *v == identity() } +/// A layer's `guides` key: left out when there are none, and every `guides` list the layer has is +/// read. Pages used to keep their guides in [`NodeKind::Page`] while the layer field took the +/// `guides` key on reading, so a reopened page lost its guides, and saving it then wrote two +/// `guides` keys, which a plain field refuses (#1476). Both lists are kept, in file order. +mod guides_key { + use super::Guide; + use serde::de::{IgnoredAny, MapAccess, Visitor}; + use serde::ser::SerializeMap; + use serde::{Deserialize, Deserializer, Serializer}; + use std::fmt; + + const KEY: &str = "guides"; + + /// Write `guides` as the layer's `guides` key, or nothing when there are none. + pub fn serialize(guides: &[Guide], s: S) -> Result { + let mut map = s.serialize_map(None)?; + if !guides.is_empty() { + map.serialize_entry(KEY, guides)?; + } + map.end() + } + + /// Read every `guides` list among the layer's fields, joined in file order. + pub fn deserialize<'de, D: Deserializer<'de>>(d: D) -> Result, D::Error> { + d.deserialize_map(Lists) + } + + /// Reads the `guides` lists of a layer's fields and skips the other fields. + struct Lists; + + impl<'de> Visitor<'de> for Lists { + type Value = Vec; + fn expecting(&self, f: &mut fmt::Formatter) -> fmt::Result { + f.write_str("a layer's fields") + } + /// Every `guides` value joined, in order; a malformed one is an error. + fn visit_map>(self, mut map: A) -> Result, A::Error> { + let mut guides = Vec::new(); + while let Some(key) = map.next_key::()? { + if key.0 { + guides.extend(map.next_value::>()?); + } else { + map.next_value::()?; + } + } + Ok(guides) + } + } + + /// Whether a field name is `guides` (without copying the name). + struct Key(bool); + + impl<'de> Deserialize<'de> for Key { + /// Compare a field name with `guides` as it is read. + fn deserialize>(d: D) -> Result { + struct Name; + impl Visitor<'_> for Name { + type Value = Key; + fn expecting(&self, f: &mut fmt::Formatter) -> fmt::Result { + f.write_str("a field name") + } + /// `guides` or another field. + fn visit_str(self, s: &str) -> Result { + Ok(Key(s == KEY)) + } + } + d.deserialize_identifier(Name) + } + } +} + impl Node { /// A node with Figma's defaults for its kind. pub fn new(id: NodeId, kind: NodeKind, name: impl Into, width: f64, height: f64) -> Self { @@ -712,17 +784,12 @@ impl Node { /// The ruler guides on this page or frame, in its own coordinates. pub fn guides(&self) -> &[Guide] { - match &self.kind { - NodeKind::Page { guides, .. } => guides, - _ => &self.guides, - } + &self.guides } + /// The ruler guides on this page or frame, to edit. pub fn guides_mut(&mut self) -> &mut Vec { - match &mut self.kind { - NodeKind::Page { guides, .. } => guides, - _ => &mut self.guides, - } + &mut self.guides } /// Children in paint order, bottom to top: the child list, reversed in an auto layout frame diff --git a/crates/engine/src/cmd/page.rs b/crates/engine/src/cmd/page.rs index 44c7b367..fefd8c73 100644 --- a/crates/engine/src/cmd/page.rs +++ b/crates/engine/src/cmd/page.rs @@ -33,6 +33,7 @@ fn page_target(s: &Session, cmd: &str, p: &Value) -> crate::Result { Ok(target) } +/// The page and ruler guide commands. pub fn specs() -> Vec { vec![ spec!("page.add", "Add page", "{name?} (a name starting with a hyphen adds a divider)", true, |s, p| { @@ -106,8 +107,10 @@ pub fn specs() -> Vec { copies.extend(doc.subtree(*c).into_iter().zip(doc.subtree(*copy))); } // The copy keeps the page's colour, flows (pointing at the copied frames), prototype settings and guides. - if let NodeKind::Page { background, flows, prototype, guides } = doc.get(src)?.kind.clone() - && let NodeKind::Page { background: b, flows: f, prototype: pr, guides: g } = &mut doc.get_mut(np)?.kind + let guides = doc.get(src)?.guides.clone(); + doc.get_mut(np)?.guides = guides; + if let NodeKind::Page { background, flows, prototype } = doc.get(src)?.kind.clone() + && let NodeKind::Page { background: b, flows: f, prototype: pr } = &mut doc.get_mut(np)?.kind { *b = background; *f = flows @@ -115,7 +118,6 @@ pub fn specs() -> Vec { .filter_map(|fl| copies.iter().find(|(old, _)| *old == fl.start).map(|(_, new)| Flow { start: *new, ..fl })) .collect(); *pr = prototype; - *g = guides; } // Keep the new page right after the source. if let Some(si) = doc.pages.iter().position(|x| *x == src) { diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 811724f2..b330b8e4 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -616,13 +616,11 @@ fn document_colors_are_counted_per_page() { assert!(v["colors"].as_array().unwrap().is_empty()); } +/// `guide.add`, `guide.move` and `guide.remove` edit the current page's guides by index. #[test] fn guides_add_move_remove() { let mut s = sess(); - let guides = |s: &Session| match &s.doc().unwrap().node(s.page().unwrap()).unwrap().kind { - NodeKind::Page { guides, .. } => guides.clone(), - _ => vec![], - }; + let guides = |s: &Session| s.doc().unwrap().node(s.page().unwrap()).unwrap().guides.clone(); let r = s.execute("guide.add", &json!({"vertical": true, "offset": 120})).unwrap(); assert_eq!(r["index"], json!(0)); s.execute("guide.add", &json!({"vertical": false, "offset": 40.5})).unwrap(); diff --git a/crates/engine/src/theme_pages_tests.rs b/crates/engine/src/theme_pages_tests.rs index 5172bb4b..1ebd78b0 100644 --- a/crates/engine/src/theme_pages_tests.rs +++ b/crates/engine/src/theme_pages_tests.rs @@ -183,6 +183,8 @@ fn text_drawn_on_a_dark_page_is_white_and_on_a_light_page_black() { assert_eq!(s.doc().unwrap().node(made).unwrap().fills, vec![Paint::solid(Color::WHITE)]); } +/// `page.duplicate` copies the page's flows (pointing at the copied frames), prototype settings +/// and guides. #[test] fn a_copy_of_a_page_keeps_its_flows_prototype_settings_and_guides() { let mut s = Session::new(); @@ -195,9 +197,12 @@ fn a_copy_of_a_page_keeps_its_flows_prototype_settings_and_guides() { let source = s.page().unwrap(); let copy = id_of(&ex(&mut s, "page.duplicate", json!({}))["id"]).unwrap(); let frames = s.doc().unwrap().node(copy).unwrap().children.clone(); - let state = |s: &Session, id: NodeId| match &s.doc().unwrap().node(id).unwrap().kind { - NodeKind::Page { flows, prototype, guides, .. } => (flows.clone(), prototype.clone(), guides.clone()), - _ => panic!("expected a page"), + let state = |s: &Session, id: NodeId| { + let n = s.doc().unwrap().node(id).unwrap(); + match &n.kind { + NodeKind::Page { flows, prototype, .. } => (flows.clone(), prototype.clone(), n.guides.clone()), + _ => panic!("expected a page"), + } }; let (flows, prototype, guides) = state(&s, copy); let (src_flows, src_prototype, src_guides) = state(&s, source); diff --git a/crates/format/src/fig/export/layer.rs b/crates/format/src/fig/export/layer.rs index 55fd6dd9..4a8604fc 100644 --- a/crates/format/src/fig/export/layer.rs +++ b/crates/format/src/fig/export/layer.rs @@ -19,11 +19,11 @@ impl<'d> Exporter<'d> { ("phase", e("CREATED")), ("parentIndex", o(vec![("guid", parent_guid), ("position", s(&position(index)))])), ]; - let children = if let NodeKind::Page { background, guides, prototype, .. } = &n.kind { + let children = if let NodeKind::Page { background, prototype, .. } = &n.kind { f.extend([("type", e("CANVAS")), ("name", s(&n.name))]); f.extend(root_fields()); f.extend([("backgroundColor", color(*background)), ("backgroundOpacity", fl(background.a.into())), ("backgroundEnabled", b(true))]); - if !guides.is_empty() { + if !n.guides.is_empty() { self.warn("ruler guides were left out"); } if *prototype != Default::default() { diff --git a/crates/format/src/guides_tests.rs b/crates/format/src/guides_tests.rs new file mode 100644 index 00000000..7fcbc8f1 --- /dev/null +++ b/crates/format/src/guides_tests.rs @@ -0,0 +1,45 @@ +//! Ruler guides in `.figcraft` files: a reopened page keeps its guides, and files saved with a +//! page's guides written twice open again (#1476). + +use figcraft_doc::{Document, Guide, NodeKind}; + +use super::*; + +/// A guide: vertical (at an x offset) or horizontal (at a y offset). +fn guide(vertical: bool, offset: f64) -> Guide { + Guide { vertical, offset } +} + +/// Page and frame guides survive saving, reopening, saving the reopened file and opening it again. +#[test] +fn page_and_frame_guides_survive_save_and_reopen() { + let mut d = Document::new("ruler"); + let page = d.first_page().unwrap(); + let frame = d.create(page, NodeKind::Frame, "F", 0.0, 0.0, 100.0, 100.0).unwrap(); + d.node_mut(page).unwrap().guides_mut().extend([guide(true, 120.0), guide(false, -30.5)]); + d.node_mut(frame).unwrap().guides_mut().push(guide(true, 12.0)); + let (once, issues) = load_bytes(&save_bytes(&d, None).unwrap()).unwrap(); + assert!(issues.is_empty(), "{issues:?}"); + assert_eq!(once.node(page).unwrap().guides(), d.node(page).unwrap().guides()); + assert_eq!(once.node(frame).unwrap().guides(), d.node(frame).unwrap().guides()); + // Saving the reopened file and opening it again (where the file stopped opening). + let text = serde_json::to_string(&to_json(&once).unwrap()).unwrap(); + assert_eq!(text.matches("\"guides\"").count(), 2, "one list on the page, one on the frame: {text}"); + let (twice, _) = load_bytes(&save_bytes(&once, None).unwrap()).unwrap(); + assert_eq!(twice.node(page).unwrap().guides(), d.node(page).unwrap().guides()); +} + +/// A `document.json` with a page's `guides` key twice (as saving a reopened page wrote it) opens +/// with both lists. +#[test] +fn a_file_with_a_page_guides_list_written_twice_opens() { + let mut d = Document::new("twice"); + let page = d.first_page().unwrap(); + d.node_mut(page).unwrap().guides_mut().push(guide(false, 7.0)); + let text = serde_json::to_string(&to_json(&d).unwrap()).unwrap(); + // What saving a reopened page wrote: the guides added since, then the ones the file had. + let broken = text.replacen("\"guides\":[", "\"guides\":[{\"vertical\":true,\"offset\":5.0}],\"guides\":[", 1); + assert_eq!(broken.matches("\"guides\"").count(), 2); + let (back, _) = from_json(&broken).unwrap(); + assert_eq!(back.node(page).unwrap().guides(), &[guide(true, 5.0), guide(false, 7.0)]); +} diff --git a/crates/format/src/lib.rs b/crates/format/src/lib.rs index 30f6e548..634d0363 100644 --- a/crates/format/src/lib.rs +++ b/crates/format/src/lib.rs @@ -977,6 +977,8 @@ pub fn load_file(path: &std::path::Path) -> Result<(Document, Vec), Form #[cfg(test)] mod branch_tests; #[cfg(test)] +mod guides_tests; +#[cfg(test)] mod image_budget_tests; #[cfg(test)] mod image_tests; From 53f47706ecee78284d7545c461edec587c848d9d Mon Sep 17 00:00:00 2001 From: Christopher Linnett Date: Thu, 29 Oct 2026 00:37:22 +0800 Subject: [PATCH 5/6] fix(present): prototype links open only behind the notice or a viewer's click (#1096) An Open link action carries a URL from the document and fires from whatever trigger the prototype sets, so an unattended presentation could open web links and mail messages one after another: a mail link bypassed the leaving-the- prototype notice entirely, and a skipped notice let any trigger open any number of links. - finish() routes every openable URL, mail included, through the notice. - Skipping the notice ("Don't show this again") opens at once only a link the viewer's own click, press, drag or key fired, one per frame. A delay, the pointer passing over a layer (enter, leave, hover) and a timed mouse trigger that fires after the gesture is over still show the notice, which then offers no checkbox: it couldn't skip that notice anyway. - The player tags each queued link with whether the viewer fired it (Presenter::fire_from); an OpenUrl outside an interaction defaults to not. - The notice covers mail links (new locale string, mail wording). - Engine prefs/command doc text follows the rule; behavior untouched there. - present_links_tests: shared setup helpers for the link tests; new tests for the skip rule (delay, hover, delayed mouse-up, key, one link per click, checkbox only for clicked links); the scheme-filter test now clicks with the notice skipped so the filter alone keeps those URLs out. - docs/prototyping-parity.md row, status line and revision history updated. --- crates/engine/src/cmd/proto.rs | 2 +- crates/engine/src/prefs.rs | 6 +- crates/ui-egui/locales/en.toml | 1 + crates/ui-egui/src/present.rs | 12 +- crates/ui-egui/src/present/actions.rs | 27 ++- crates/ui-egui/src/present/chrome.rs | 39 ++-- crates/ui-egui/src/present_links_tests.rs | 211 +++++++++++++++++++--- docs/prototyping-parity.md | 5 +- 8 files changed, 248 insertions(+), 55 deletions(-) diff --git a/crates/engine/src/cmd/proto.rs b/crates/engine/src/cmd/proto.rs index c451eeb2..d0822db9 100644 --- a/crates/engine/src/cmd/proto.rs +++ b/crates/engine/src/cmd/proto.rs @@ -161,7 +161,7 @@ pub fn specs() -> Vec { spec!( "proto.linkNotice", "Notice before leaving for a link", - "{show? (default true; false skips it)} whether a presented prototype's Open link to a website first says that it leaves the prototype, with Open link and Cancel; kept with the preferences → {shown}", + "{show? (default true; false skips it)} whether a presented prototype's Open link the viewer clicked (or fired by a key) first says that it leaves the prototype, with Open link and Cancel (a link a delay or hover fired always does); kept with the preferences → {shown}", false, |s, p| { let show = boolean(p, "show").unwrap_or(true); diff --git a/crates/engine/src/prefs.rs b/crates/engine/src/prefs.rs index ced1793f..35a26bc9 100644 --- a/crates/engine/src/prefs.rs +++ b/crates/engine/src/prefs.rs @@ -82,8 +82,10 @@ pub struct Prefs { /// The Getting started tips on the home screen were closed (Help ▸ Getting started shows them /// again). pub getting_started_dismissed: bool, - /// A prototype's Open link goes to a website without first telling the viewer that it leaves - /// the prototype (the notice's "Don't show this again"; `proto.linkNotice` shows it again). + /// A prototype's Open link — to a website or to the mail app — that the viewer's own click, + /// press, drag or key fired opens without first telling them that it leaves the prototype + /// (the notice's "Don't show this again"; `proto.linkNotice` shows it again). A link a delay + /// or the pointer passing over a layer fired still shows the notice (#1096). pub link_notice_skipped: bool, /// How each home screen section shows its files, by section id (`"recents"`): `"grid"` (cards /// with the picture each file carries) or `"list"` (a table). Missing: the grid. diff --git a/crates/ui-egui/locales/en.toml b/crates/ui-egui/locales/en.toml index b0afcdf7..ce98cf75 100644 --- a/crates/ui-egui/locales/en.toml +++ b/crates/ui-egui/locales/en.toml @@ -1487,6 +1487,7 @@ name = "English" "This frame is new on the branch." = "This frame is new on the branch." "This frame isn't on the branch." = "This frame isn't on the branch." "This link opens a website outside the prototype:" = "This link opens a website outside the prototype:" +"This link sends a mail message through your mail app:" = "This link sends a mail message through your mail app:" "This server is open to the public: no key needed." = "This server is open to the public: no key needed." "This server needs its key: add ?key=… to the address (`figcraft-server key` prints it on the server's machine)." = "This server needs its key: add ?key=… to the address (`figcraft-server key` prints it on the server's machine)." "This server takes files only with its key: see Advanced." = "This server takes files only with its key: see Advanced." diff --git a/crates/ui-egui/src/present.rs b/crates/ui-egui/src/present.rs index 64a3b70a..8e394a86 100644 --- a/crates/ui-egui/src/present.rs +++ b/crates/ui-egui/src/present.rs @@ -299,10 +299,14 @@ pub struct Presenter { clock: f64, /// The layer the Inspect panel shows (the last one under the pointer). inspected: Option, - urls: Vec, - /// A link to a website waiting for the viewer to open it ([`link_notice`]), and whether "Don't - /// show this again" is ticked. - link_notice: Option<(String, bool)>, + /// The links the last frame's actions opened, and whether the viewer's own click, press, drag + /// or key fired each; the frame-end queues them behind the notice, but for one the viewer + /// fired while choosing to skip the notice (#1096). + urls: Vec<(String, bool)>, + /// A link waiting for the viewer to open it ([`link_notice`]), and whether "Don't show this + /// again" is ticked; `None` when the notice doesn't offer it (the viewer didn't fire the link, + /// so skipping the notice wouldn't cover it, or they skip it already). + link_notice: Option<(String, Option)>, /// The editor's document as last copied (the player copies it again when it changes). source: Option>, /// Scrolling by dragging: what it scrolls and its offset when the drag began. diff --git a/crates/ui-egui/src/present/actions.rs b/crates/ui-egui/src/present/actions.rs index ad2c646e..5cd4e03e 100644 --- a/crates/ui-egui/src/present/actions.rs +++ b/crates/ui-egui/src/present/actions.rs @@ -36,14 +36,23 @@ impl Presenter { self.waiting.retain(|w| w.root == root); let mut did = false; for w in due.into_iter().filter(|w| w.root == root) { - // An earlier one may have left the screen: the rest are dropped with it. - did = did || self.fire(w.node, w.k, w.rect, now); + // An earlier one may have left the screen: the rest are dropped with it. The gesture + // is long over: what these open is not the viewer's doing (#1096). + did = did || self.fire_from(w.node, w.k, w.rect, now, false); } did } /// Run the actions of interaction `k` on `node`. `rect` is the layer's screen rect. pub(crate) fn fire(&mut self, node: NodeId, k: usize, rect: Option, now: f64) -> bool { + self.fire_from(node, k, rect, now, true) + } + + /// [`fire`](Self::fire); `live`: the viewer's input fired it just now (not a delay it waited + /// out). The links it opens are the viewer's own only when live from a click, press, drag or + /// key: a delay, the pointer passing over a layer and a timed mouse trigger fire without the + /// viewer asking for anything ([`Presenter::urls`], #1096). + fn fire_from(&mut self, node: NodeId, k: usize, rect: Option, now: f64, live: bool) -> bool { let Some(it) = self.doc.as_ref().and_then(|d| d.node(node)).and_then(|n| n.interactions.get(k)).cloned() else { return false }; let dur = it.easing.seconds(it.duration_ms); let temp = match it.trigger { @@ -53,9 +62,18 @@ impl Presenter { }; let snap = if temp.is_some() && self.temp.is_none() { self.snapshot() } else { None }; let mut did = false; + let first_link = self.urls.len(); for a in &it.actions { did |= self.perform(a, node, it.transition, dur, it.easing, rect, now); } + let asked = live + && matches!( + it.trigger, + Trigger::OnClick | Trigger::OnPress | Trigger::OnDrag | Trigger::MouseDown | Trigger::MouseUp | Trigger::KeyPress { .. } + ); + for link in self.urls.iter_mut().skip(first_link) { + link.1 = asked; + } if did && let (Some(press), Some(snap), Some(rect)) = (temp, snap, rect) { self.temp = Some(Temp { rect, press, snap, transition: it.transition, dur, easing: it.easing }); } @@ -178,9 +196,10 @@ impl Presenter { Action::CloseOverlay => self.close_overlay(now), Action::OpenUrl { url } => { // The URL comes from the document and this action fires from whatever trigger the - // prototype sets, including a delay, so no click is implied: only a web link opens. + // prototype sets, including a delay, so no click is implied: only a web or mail + // link is kept, as not the viewer's doing until `fire_from` says otherwise. if figcraft_engine::doc_urls::openable(url) { - self.urls.push(url.clone()); + self.urls.push((url.clone(), false)); } true } diff --git a/crates/ui-egui/src/present/chrome.rs b/crates/ui-egui/src/present/chrome.rs index eeaba1ce..12399ced 100644 --- a/crates/ui-egui/src/present/chrome.rs +++ b/crates/ui-egui/src/present/chrome.rs @@ -239,15 +239,20 @@ fn poll_pads(app: &mut FigApp, p: &mut Presenter, ctx: &egui::Context) { crate::repaint_in(ctx, PAD_POLL_SECS); } -/// After a player's frame: the links it opens and the widget clicks it took. A link to a website -/// waits in [`link_notice`] unless the viewer chose to skip the notice; a second link to a website -/// that fires while the notice shows is dropped. +/// After a player's frame: the links it opens and the widget clicks it took. Every link waits in +/// [`link_notice`] — to a website or to the mail app (#1096). Skipping the notice opens at once +/// only a link the viewer's own click, press, drag or key fired, one per frame: a delay or the +/// pointer passing over a layer never opens anything unasked. A further link that fires while the +/// notice shows is dropped. fn finish(app: &mut FigApp, p: &mut Presenter) { - for u in p.urls.drain(..) { - if app.session.prefs.link_notice_skipped || !goes_to_a_website(&u) { + let skipped = app.session.prefs.link_notice_skipped; + let mut opened = false; + for (u, asked) in p.urls.drain(..) { + if skipped && asked && !opened { app.services.open_url(&u); + opened = true; } else if p.link_notice.is_none() { - p.link_notice = Some((u, false)); + p.link_notice = Some((u, (asked && !skipped).then_some(false))); } } if let Some(e) = p.click_widgets(&app.session) { @@ -255,24 +260,32 @@ fn finish(app: &mut FigApp, p: &mut Presenter) { } } -/// Whether `url` is an `http` or `https` link (a `mailto:` link opens the mail app instead). +/// Whether `url` is an `http` or `https` link (of the openable URLs, the others are `mailto:`; +/// [`figcraft_engine::doc_urls`]). fn goes_to_a_website(url: &str) -> bool { url.trim().split_once(':').is_some_and(|(scheme, _)| scheme.eq_ignore_ascii_case("http") || scheme.eq_ignore_ascii_case("https")) } -/// The notice before a link leaves the prototype for a website: the link, "Don't show this -/// again" (saved with the preferences when Open link is pressed), Open link and Cancel. Escape or -/// the close button cancels. +/// The notice before a link leaves the prototype — for a website, or for the mail app: the link, +/// "Don't show this again" for a link the viewer fired (saved with the preferences when Open link +/// is pressed), Open link and Cancel. Escape or the close button cancels. fn link_notice(app: &mut FigApp, p: &mut Presenter, ctx: &egui::Context, id: Id) { let Some((url, skip)) = p.link_notice.as_mut() else { return }; let t = Tokens::get(ctx); let (mut open, mut go, mut cancel) = (true, false, false); crate::dialogs::window(tr("Leaving the prototype"), ctx).id(id).open(&mut open).show(ctx, |ui| { ui.set_width(340.0); - widgets::label(ui, tr("This link opens a website outside the prototype:"), t.text); + let what = if goes_to_a_website(url) { + tr("This link opens a website outside the prototype:") + } else { + tr("This link sends a mail message through your mail app:") + }; + widgets::label(ui, what, t.text); ui.add(egui::Label::new(egui::RichText::new(url.as_str()).font(regular(11.0)).color(t.text_secondary)).wrap()); ui.add_space(8.0); - if widgets::checkbox(ui, *skip, tr("Don't show this again")) { + if let Some(skip) = skip.as_mut() + && widgets::checkbox(ui, *skip, tr("Don't show this again")) + { *skip = !*skip; } ui.add_space(12.0); @@ -284,7 +297,7 @@ fn link_notice(app: &mut FigApp, p: &mut Presenter, ctx: &egui::Context, id: Id) cancel |= !open || ctx.input_mut(|i| i.consume_key(egui::Modifiers::NONE, egui::Key::Escape)); if go { let Some((url, skip)) = p.link_notice.take() else { return }; - if skip { + if skip == Some(true) { app.run("proto.linkNotice", json!({"show": false})); app.services.save_prefs(&app.session.prefs); } diff --git a/crates/ui-egui/src/present_links_tests.rs b/crates/ui-egui/src/present_links_tests.rs index a2b571af..03c82552 100644 --- a/crates/ui-egui/src/present_links_tests.rs +++ b/crates/ui-egui/src/present_links_tests.rs @@ -1,9 +1,13 @@ //! A prototype's Open link action carries a URL from the document and fires from whatever trigger //! the prototype sets, including a delay, so the player hands the opener only a web link //! ([`figcraft_engine::doc_urls`]). Without that, a saved document opens `javascript:` in the web -//! build's own origin (stored XSS) and `smb:`/`ms-…:` through the desktop's handlers. A link to a -//! website then waits behind a notice that it leaves the prototype, unless the viewer chose to skip -//! the notice. +//! build's own origin (stored XSS) and `smb:`/`ms-…:` through the desktop's handlers. Every +//! openable link waits behind a notice that it leaves the prototype — to a website or to the +//! mail app (#1096). Skipping the notice opens at once only a link the viewer's own click, press, +//! drag or key fired, one per frame; a delay or the pointer passing over a layer never opens +//! anything unasked. Before, a mail link opened at once however it was fired, and with the notice +//! skipped any trigger opened any number of links: an unattended presentation could open links and +//! mail messages one after another. use std::sync::{Arc, Mutex}; @@ -35,46 +39,76 @@ fn ex(s: &mut Session, cmd: &str, p: Value) -> Value { s.execute(cmd, &p).unwrap_or_else(|e| panic!("{cmd} {p}: {e}")) } -/// A presented frame whose layer opens `url` 100ms after the screen appears, with no click -/// anywhere, and with the notice skipped or not. Returns the harness and what the player opened. +/// Frame A presented by `cmd` (`proto.present` or `proto.preview`) with the notice skipped or +/// not, its `interaction` (a trigger and maybe `delayMs`) opening each of `urls`. Returns the +/// harness and what the player opened. +fn playing(cmd: &str, skip_notice: bool, interaction: Value, urls: &[&str]) -> (Harness, Opened) { + let mut s = Session::new(); + s.prefs.link_notice_skipped = skip_notice; + let f = id(&ex(&mut s, "frame.create", json!({"x": 0, "y": 0, "width": 300, "height": 400, "name": "A"}))); + let mut it = interaction; + it["id"] = json!(f); + it["actions"] = urls.iter().map(|u| json!({"type": "openUrl", "url": u})).collect(); + it["transition"] = json!({"type": "instant"}); + ex(&mut s, "proto.interaction.add", it); + present(cmd, s, f) +} + +/// `s` presented by `cmd` from frame `start`, with the opener and the preferences recorded. +fn present(cmd: &str, s: Session, start: figcraft_doc::NodeId) -> (Harness, Opened) { + let seen = Opened::default(); + let mut u = Harness::new(s); + u.app.services = Box::new(Opened(seen.0.clone(), seen.1.clone())); + u.ctx.enable_accesskit(); + u.run(cmd, json!({"start": start})); + u.frames(2, Vec::new); + (u, seen) +} + +/// A presented frame that opens `url` 100ms after the screen appears, with no click anywhere, +/// and with the notice skipped or not. Returns the harness and what the player opened. fn presented(url: &str, skip_notice: bool) -> (Harness, Opened) { played("proto.present", url, skip_notice) } /// [`presented`], played by `cmd` (`proto.present` or `proto.preview`). fn played(cmd: &str, url: &str, skip_notice: bool) -> (Harness, Opened) { - let mut s = Session::new(); - s.prefs.link_notice_skipped = skip_notice; - let f = id(&ex(&mut s, "frame.create", json!({"x": 0, "y": 0, "width": 300, "height": 400, "name": "A"}))); - let layer = id(&ex(&mut s, "shape.rectangle", json!({"x": 10, "y": 10, "width": 30, "height": 30, "parent": f}))); - ex( - &mut s, - "proto.interaction.add", - json!({"id": layer, "trigger": {"type": "afterDelay", "ms": 100}, "action": {"type": "openUrl", "url": url}, "transition": {"type": "instant"}}), - ); - let seen = Opened::default(); - let mut u = Harness::new(s); - u.app.services = Box::new(Opened(seen.0.clone(), seen.1.clone())); - u.ctx.enable_accesskit(); - u.run(cmd, json!({"start": f})); + let (mut u, seen) = playing(cmd, skip_notice, json!({"trigger": {"type": "afterDelay", "ms": 100}}), &[url]); // Well past the delay. u.frames(40, Vec::new); (u, seen) } +/// A click on the presented frame, and a few frames for what it fires. +fn click_the_frame(u: &mut Harness) { + let at = egui::pos2(700.0, 290.0); + u.button(at, true); + u.button(at, false); + u.frames(3, Vec::new); +} + +/// What a click on a frame whose click opens `url` opens, with the notice skipped. +fn opens_on_a_click(url: &str) -> Vec { + let (mut u, seen) = playing("proto.present", true, json!({"trigger": {"type": "onClick"}}), &[url]); + click_the_frame(&mut u); + opened(&seen) +} + fn opened(seen: &Opened) -> Vec { seen.0.lock().map(|v| v.clone()).unwrap_or_default() } -fn opens_after_a_delay(url: &str) -> Vec { - opened(&presented(url, true).1) -} - /// The URL the presenter's notice waits to open. fn notice(u: &Harness) -> Value { u.app.inspect()["presenter"]["linkNotice"].clone() } +/// The labels of the controls drawn now. +fn labels(u: &mut Harness) -> Vec { + let tree = u.frame_output(vec![]).platform_output.accesskit_update.unwrap().nodes; + tree.into_iter().filter_map(|(_, n)| n.label().map(str::to_string)).collect() +} + /// Clicks the control labeled `label`. fn click(u: &mut Harness, label: &str) { let tree = u.frame_output(vec![]).platform_output.accesskit_update.unwrap().nodes; @@ -87,10 +121,128 @@ fn click(u: &mut Harness, label: &str) { #[test] fn a_prototype_opens_web_links() { - assert_eq!(opens_after_a_delay("https://example.com/a"), ["https://example.com/a"]); - assert_eq!(opens_after_a_delay("mailto:a@b.c"), ["mailto:a@b.c"]); - // A mail link opens the mail app without the notice. - assert_eq!(opened(&presented("mailto:a@b.c", false).1), ["mailto:a@b.c"]); + // The viewer chose to skip the notice: their yes, kept, for the links they click. + assert_eq!(opens_on_a_click("https://example.com/a"), ["https://example.com/a"]); + assert_eq!(opens_on_a_click("mailto:a@b.c"), ["mailto:a@b.c"]); + // With the notice on a mail link waits behind it like a website: nothing reaches the mail + // app, or any opener, unasked (#1096). + let (mut u, seen) = presented("mailto:a@b.c", false); + assert!(opened(&seen).is_empty(), "a mail link fired without a click waits behind the notice"); + assert_eq!(notice(&u), "mailto:a@b.c"); + click(&mut u, "Open link"); + assert_eq!(opened(&seen), ["mailto:a@b.c"]); + assert!(notice(&u).is_null()); +} + +/// Skipping the notice covers what the viewer does, not what the prototype does by itself: a link +/// on a delay still waits behind the notice, which then offers no "Don't show this again", as +/// that couldn't cover it (#1096). +#[test] +fn a_skipped_notice_still_shows_for_a_link_no_click_fired() { + for url in ["https://example.com/a", "mailto:a@b.c"] { + let (mut u, seen) = presented(url, true); + assert!(opened(&seen).is_empty(), "{url}: a delay opened it with the notice skipped"); + assert_eq!(notice(&u), url); + assert!(!labels(&mut u).iter().any(|l| l == "Don't show this again"), "{url}: the checkbox can't skip this notice"); + click(&mut u, "Open link"); + assert_eq!(opened(&seen), [url]); + } +} + +/// A clicked link the notice first shows for offers "Don't show this again" (#1096). +#[test] +fn the_notice_for_a_clicked_link_offers_to_skip_it() { + let (mut u, _) = playing("proto.present", false, json!({"trigger": {"type": "onClick"}}), &["https://example.com/a"]); + click_the_frame(&mut u); + assert_eq!(notice(&u), "https://example.com/a"); + assert!(labels(&mut u).iter().any(|l| l == "Don't show this again")); +} + +/// With the notice skipped one click opens one link, however many its interaction opens: the +/// next waits behind the notice and the rest are dropped (#1096). +#[test] +fn a_click_opens_one_link_with_the_notice_skipped() { + let urls = ["https://example.com/1", "https://example.com/2", "mailto:a@b.c"]; + let (mut u, seen) = playing("proto.present", true, json!({"trigger": {"type": "onClick"}}), &urls); + click_the_frame(&mut u); + assert_eq!(opened(&seen), ["https://example.com/1"]); + assert_eq!(notice(&u), "https://example.com/2"); +} + +/// The pointer passing over a layer is not the viewer asking for a link: with the notice skipped, +/// a mouse enter's link still waits behind it (#1096). +#[test] +fn a_link_the_pointer_fires_by_passing_waits_with_the_notice_skipped() { + let (mut u, seen) = playing("proto.present", true, json!({"trigger": {"type": "mouseEnter"}}), &["https://example.com/hover"]); + u.frame(vec![egui::Event::PointerMoved(egui::pos2(700.0, 290.0))]); + u.frames(3, Vec::new); + assert!(opened(&seen).is_empty(), "passing over the layer opened it"); + assert_eq!(notice(&u), "https://example.com/hover"); +} + +/// A presentation whose prototype queues several mail links on delays opens none of them by +/// itself: the first waits behind the notice and the rest are dropped with it (#1096). +#[test] +fn delayed_opens_do_not_pile_up_unattended() { + let mut s = Session::new(); + s.prefs.link_notice_skipped = true; + let f = id(&ex(&mut s, "frame.create", json!({"x": 0, "y": 0, "width": 300, "height": 400, "name": "A"}))); + for ms in [100u32, 200, 300] { + let layer = id(&ex(&mut s, "shape.rectangle", json!({"x": 10, "y": 10, "width": 30, "height": 30, "parent": f}))); + ex( + &mut s, + "proto.interaction.add", + json!({"id": layer, "trigger": {"type": "afterDelay", "ms": ms}, "action": {"type": "openUrl", "url": format!("mailto:victim@example.com?subject=spam{ms}")}}), + ); + } + let (mut u, seen) = present("proto.present", s, f); + u.frames(40, Vec::new); // no click, no key: the presentation alone + assert!(opened(&seen).is_empty(), "a presentation by itself opens nothing, the notice skipped or not"); + assert_eq!(notice(&u), "mailto:victim@example.com?subject=spam100", "the first waits, the rest are dropped"); + click(&mut u, "Open link"); + assert_eq!(opened(&seen), ["mailto:victim@example.com?subject=spam100"]); +} + +/// A mail link the viewer clicks is still leaving the prototype: it waits behind the notice like +/// every other link (#1096). Before, a mail link opened straight away, trigger or no trigger. +#[test] +fn a_clicked_mail_link_waits_behind_the_notice() { + let (mut u, seen) = playing("proto.present", false, json!({"trigger": {"type": "onClick"}}), &["mailto:a@b.c?subject=hello"]); + click_the_frame(&mut u); + assert!(opened(&seen).is_empty(), "the notice comes first, whatever fired the link"); + assert_eq!(notice(&u), "mailto:a@b.c?subject=hello"); + click(&mut u, "Open link"); + assert_eq!(opened(&seen), ["mailto:a@b.c?subject=hello"]); +} + +/// A mouse-up whose actions run on a delay fires once the click is long over; its link waits +/// behind the notice like a timed one does, the notice skipped or not (#1096). +#[test] +fn a_link_a_click_queued_to_fire_later_waits_behind_the_notice() { + for skip in [false, true] { + let (mut u, seen) = playing("proto.present", skip, json!({"trigger": {"type": "mouseUp"}, "delayMs": 150}), &["https://example.com/late"]); + click_the_frame(&mut u); + u.frames(20, Vec::new); // well past the 150 ms the click queued + assert!(opened(&seen).is_empty(), "skip {skip}: the actions fire after the click is over: not a gesture"); + assert_eq!(notice(&u), "https://example.com/late"); + } +} + +/// A link a key trigger fires waits behind the notice too, and opens at once with it skipped: a +/// key is the viewer's own doing (#1096). +#[test] +fn a_link_fired_by_a_key_waits_behind_the_notice() { + for skip in [false, true] { + let (mut u, seen) = playing("proto.present", skip, json!({"trigger": {"type": "keyPress", "key": "K"}}), &["https://example.com/keyed"]); + u.key(egui::pos2(700.0, 450.0), egui::Key::K); + u.frames(3, Vec::new); + if skip { + assert_eq!(opened(&seen), ["https://example.com/keyed"]); + } else { + assert!(opened(&seen).is_empty(), "a key is not a click"); + assert_eq!(notice(&u), "https://example.com/keyed"); + } + } } #[test] @@ -192,7 +344,8 @@ fn the_prototype_takes_no_controller_buttons_while_the_notice_shows() { #[test] fn dont_show_this_again_is_saved_with_the_preferences() { - let (mut u, seen) = presented("https://example.com/a", false); + let (mut u, seen) = playing("proto.present", false, json!({"trigger": {"type": "onClick"}}), &["https://example.com/a"]); + click_the_frame(&mut u); click(&mut u, "Don't show this again"); click(&mut u, "Open link"); assert_eq!(opened(&seen), ["https://example.com/a"]); @@ -216,7 +369,7 @@ fn a_prototype_cannot_open_a_script_or_handler_url() { "ms-msdt:/id PCWDiagnostic", "figcraft://join?host=127.0.0.1:47900&file=f&token=t", ] { - assert!(opens_after_a_delay(url).is_empty(), "{url:?} must never reach the opener"); + assert!(opens_on_a_click(url).is_empty(), "{url:?} must never reach the opener"); } } diff --git a/docs/prototyping-parity.md b/docs/prototyping-parity.md index 6c7b2437..ec2daf04 100644 --- a/docs/prototyping-parity.md +++ b/docs/prototyping-parity.md @@ -1,6 +1,6 @@ # Prototyping parity -> **Last reviewed:** 2026-10-11 · **Last updated:** 2026-10-24 · **Change:** patch (overlay positions relative to the layer that opens them; the Triggers row no longer lists delays on mouse triggers as missing; top-level frames bigger than the device's screen scroll in the player; Change to, set variable and set variable mode lay out what they change in the player, as the canvas does; presentation tabs, one per page of a file; a Present tab with device and scale controls (#537); core editor leftovers: delays on mouse triggers and manual overlay offsets; controller buttons on key/gamepad triggers, widgets that react in prototypes, `proto.evaluate` and the mode lookup rules pinned) · **Target:** Figma Design (desktop app 126.10.7) +> **Last reviewed:** 2026-10-11 · **Last updated:** 2026-10-29 · **Change:** patch (Open link notices now cover mail links, and skipping them covers only links the viewer fired (#1096); overlay positions relative to the layer that opens them; the Triggers row no longer lists delays on mouse triggers as missing; top-level frames bigger than the device's screen scroll in the player; Change to, set variable and set variable mode lay out what they change in the player, as the canvas does; presentation tabs, one per page of a file; a Present tab with device and scale controls (#537); core editor leftovers: delays on mouse triggers and manual overlay offsets; controller buttons on key/gamepad triggers, widgets that react in prototypes, `proto.evaluate` and the mode lookup rules pinned) · **Target:** Figma Design (desktop app 126.10.7) **Breadth ~92%, ready ~74% (estimated).** Every Figma trigger except media, every action except media control, every transition and easing exist; key/gamepad triggers take controller buttons, @@ -94,7 +94,7 @@ tool; opt-in), **silent** (the documentation doesn't say; the case pins the curr | Hotspot hints, inspect panel in the presenter, comments in the presenter | done | | | Preview pane following edits live | done | | | Edit interactions on several layers at once | done | | -| Open link: a notice before a website opens, with a checkbox to skip it on this device | done | `http` and `https` links (a mail link opens at once); Open link opens it, Cancel or Escape drops it; the desktop app saves the checkbox with its preferences, and the web build keeps it until the page reloads; `proto.linkNotice {show}` turns it on or off (`present_links_tests`) | +| Open link: a notice before a link opens, with a checkbox to skip it on this device | done | every openable link — `http`, `https` and `mailto` — and whatever fired it, a click, a delay, a key (a further link that fires while the notice shows is dropped); skipping it opens at once only a link the viewer's own click, press, drag or key fired, one per frame — a delay, the pointer passing over a layer or a timed mouse trigger still shows the notice, without the checkbox; Open link opens it, Cancel or Escape drops it; the desktop app saves the checkbox with its preferences, and the web build keeps it until the page reloads; `proto.linkNotice {show}` turns it on or off (`present_links_tests`) | | Opening a link in place | missing | | | Controllers on macOS, Linux and FreeBSD | missing | the usual Rust controller library links the system's libudev on Linux; the pure-Rust desktop builds read none there yet | | Prototype in a separate window; mirror to a phone | missing | see [hardware-parity](hardware-parity.md) | @@ -105,6 +105,7 @@ tool; opt-in), **silent** (the documentation doesn't say; the case pins the curr | Date | Change | Summary | |---|---|---| +| 2026-10-29 | patch | The Open link notice covers mail links, and skipping it covers only links the viewer fired: before, a mail link opened the mail app at once, also from an "After delay" trigger, and while the notice was skipped any trigger opened any number of web links. Now every openable link waits behind the notice; with it skipped, a link the viewer's own click, press, drag or key fired opens at once, one per frame, while a delay, the pointer passing over a layer or a timed mouse trigger still shows the notice, which then offers no "Don't show this again" (#1096, `crates/ui-egui/src/present_links_tests.rs`). Numbers unchanged | | 2026-10-24 | patch | The Triggers row's Missing column no longer lists delays on mouse enter, leave, down and up, which landed on 2026-10-13 (`crates/engine/src/proto_delay_tests.rs`, `crates/ui-egui/src/present/delay_tests.rs`). Numbers unchanged | | 2026-10-24 | patch | Open link to a website first shows a notice that it leaves the prototype, with the link, Open link, Cancel and "Don't show this again", which the desktop app saves with its preferences and the web build keeps until the page reloads (`proto.linkNotice` turns the notice back on); a second link to a website that fires while the notice shows is dropped, and a mail link still opens. Before, the player opened the link at once, also from an After delay trigger. Numbers unchanged | | 2026-10-23 | patch | A top-level frame taller (or wider) than the device's screen scrolls with no overflow setting, as Figma's documentation says; before, "No scrolling" frames such as the landing page template's Phone frame only clipped. Content past a frame's own edges still scrolls only as its overflow allows. Checked in the app on macOS. Numbers unchanged | From 7660877845327f0ab089b2cd87cb2e41da3dc3c9 Mon Sep 17 00:00:00 2001 From: Christopher Linnett Date: Thu, 29 Oct 2026 08:50:00 +0800 Subject: [PATCH 6/6] fix(present): nothing opens or releases behind an open link notice (#1096) - finish(): a viewer-fired link opens directly (notice skipped) only while no notice shows; any other link that fires while one shows is dropped. - finish(): raising a notice ends the press under way, so its release doesn't fire the prototype's mouse-up actions (a link, a navigation) behind the notice, which takes no input while it shows. - The mail notice says the link opens the mail app to write a message (opening a mailto: link doesn't send anything). - Tests: nothing_opens_while_a_notice_shows_with_the_notice_skipped, a_press_that_raised_a_notice_doesnt_release_behind_it. - docs/prototyping-parity.md row and revision history. --- crates/ui-egui/locales/en.toml | 2 +- crates/ui-egui/src/present/chrome.rs | 13 +++-- crates/ui-egui/src/present_links_tests.rs | 60 +++++++++++++++++++++++ docs/prototyping-parity.md | 4 +- 4 files changed, 71 insertions(+), 8 deletions(-) diff --git a/crates/ui-egui/locales/en.toml b/crates/ui-egui/locales/en.toml index ce98cf75..f84cd45b 100644 --- a/crates/ui-egui/locales/en.toml +++ b/crates/ui-egui/locales/en.toml @@ -1487,7 +1487,7 @@ name = "English" "This frame is new on the branch." = "This frame is new on the branch." "This frame isn't on the branch." = "This frame isn't on the branch." "This link opens a website outside the prototype:" = "This link opens a website outside the prototype:" -"This link sends a mail message through your mail app:" = "This link sends a mail message through your mail app:" +"This link opens your mail app to write a message:" = "This link opens your mail app to write a message:" "This server is open to the public: no key needed." = "This server is open to the public: no key needed." "This server needs its key: add ?key=… to the address (`figcraft-server key` prints it on the server's machine)." = "This server needs its key: add ?key=… to the address (`figcraft-server key` prints it on the server's machine)." "This server takes files only with its key: see Advanced." = "This server takes files only with its key: see Advanced." diff --git a/crates/ui-egui/src/present/chrome.rs b/crates/ui-egui/src/present/chrome.rs index 12399ced..bd74b5ac 100644 --- a/crates/ui-egui/src/present/chrome.rs +++ b/crates/ui-egui/src/present/chrome.rs @@ -241,18 +241,21 @@ fn poll_pads(app: &mut FigApp, p: &mut Presenter, ctx: &egui::Context) { /// After a player's frame: the links it opens and the widget clicks it took. Every link waits in /// [`link_notice`] — to a website or to the mail app (#1096). Skipping the notice opens at once -/// only a link the viewer's own click, press, drag or key fired, one per frame: a delay or the -/// pointer passing over a layer never opens anything unasked. A further link that fires while the -/// notice shows is dropped. +/// only a link the viewer's own click, press, drag or key fired, one per frame, and never while a +/// notice shows: a delay or the pointer passing over a layer never opens anything unasked. A +/// further link that fires while the notice shows is dropped, whatever fired it. fn finish(app: &mut FigApp, p: &mut Presenter) { let skipped = app.session.prefs.link_notice_skipped; let mut opened = false; for (u, asked) in p.urls.drain(..) { - if skipped && asked && !opened { + if skipped && asked && !opened && p.link_notice.is_none() { app.services.open_url(&u); opened = true; } else if p.link_notice.is_none() { p.link_notice = Some((u, (asked && !skipped).then_some(false))); + // A press under way ends here: its release would otherwise reach the prototype + // behind the notice, which takes no input while it shows. + p.press = None; } } if let Some(e) = p.click_widgets(&app.session) { @@ -278,7 +281,7 @@ fn link_notice(app: &mut FigApp, p: &mut Presenter, ctx: &egui::Context, id: Id) let what = if goes_to_a_website(url) { tr("This link opens a website outside the prototype:") } else { - tr("This link sends a mail message through your mail app:") + tr("This link opens your mail app to write a message:") }; widgets::label(ui, what, t.text); ui.add(egui::Label::new(egui::RichText::new(url.as_str()).font(regular(11.0)).color(t.text_secondary)).wrap()); diff --git a/crates/ui-egui/src/present_links_tests.rs b/crates/ui-egui/src/present_links_tests.rs index 03c82552..39137fca 100644 --- a/crates/ui-egui/src/present_links_tests.rs +++ b/crates/ui-egui/src/present_links_tests.rs @@ -404,3 +404,63 @@ fn the_prototype_takes_no_clicks_while_the_notice_shows() { u.frames(5, Vec::new); assert_eq!(u.app.inspect()["presenter"]["current"], json!(b), "with the notice gone the same click navigates"); } + +/// While a notice shows nothing else opens, even a link the viewer fired with the notice skipped: +/// a press that opened one link and put the next behind the notice then releases over a +/// mouse-up link, which is dropped with the notice up (#1096). +#[test] +fn nothing_opens_while_a_notice_shows_with_the_notice_skipped() { + let mut s = Session::new(); + s.prefs.link_notice_skipped = true; + let f = id(&ex(&mut s, "frame.create", json!({"x": 0, "y": 0, "width": 300, "height": 400, "name": "A"}))); + let open = |u: &str| json!({"type": "openUrl", "url": u}); + ex( + &mut s, + "proto.interaction.add", + json!({"id": f, "trigger": {"type": "mouseDown"}, "actions": [open("https://example.com/1"), open("https://example.com/2")], "transition": {"type": "instant"}}), + ); + ex( + &mut s, + "proto.interaction.add", + json!({"id": f, "trigger": {"type": "mouseUp"}, "actions": [open("https://example.com/3")], "transition": {"type": "instant"}}), + ); + let (mut u, seen) = present("proto.present", s, f); + let at = egui::pos2(700.0, 290.0); + u.button(at, true); + u.frames(2, Vec::new); + assert_eq!(opened(&seen), ["https://example.com/1"]); + assert_eq!(notice(&u), "https://example.com/2"); + u.button(at, false); + u.frames(3, Vec::new); + assert_eq!(opened(&seen), ["https://example.com/1"], "the release opened a link with the notice up"); + assert_eq!(notice(&u), "https://example.com/2"); +} + +/// The prototype takes no input while a notice shows, including the release of a press that +/// began before it: a mouse-up that navigates doesn't run behind the notice (#1096). +#[test] +fn a_press_that_raised_a_notice_doesnt_release_behind_it() { + let mut s = Session::new(); + s.prefs.link_notice_skipped = true; + let a = id(&ex(&mut s, "frame.create", json!({"x": 0, "y": 0, "width": 300, "height": 400, "name": "A"}))); + let b = id(&ex(&mut s, "frame.create", json!({"x": 400, "y": 0, "width": 300, "height": 400, "name": "B"}))); + let open = |u: &str| json!({"type": "openUrl", "url": u}); + ex( + &mut s, + "proto.interaction.add", + json!({"id": a, "trigger": {"type": "mouseDown"}, "actions": [open("https://example.com/1"), open("https://example.com/2")], "transition": {"type": "instant"}}), + ); + ex( + &mut s, + "proto.interaction.add", + json!({"id": a, "trigger": {"type": "mouseUp"}, "action": {"type": "navigate", "dest": b}, "transition": {"type": "instant"}}), + ); + let (mut u, _) = present("proto.present", s, a); + let at = egui::pos2(700.0, 290.0); + u.button(at, true); + u.frames(2, Vec::new); + assert_eq!(notice(&u), "https://example.com/2"); + u.button(at, false); + u.frames(3, Vec::new); + assert_eq!(u.app.inspect()["presenter"]["current"], json!(a), "the release navigated behind the notice"); +} diff --git a/docs/prototyping-parity.md b/docs/prototyping-parity.md index ec2daf04..f8241097 100644 --- a/docs/prototyping-parity.md +++ b/docs/prototyping-parity.md @@ -94,7 +94,7 @@ tool; opt-in), **silent** (the documentation doesn't say; the case pins the curr | Hotspot hints, inspect panel in the presenter, comments in the presenter | done | | | Preview pane following edits live | done | | | Edit interactions on several layers at once | done | | -| Open link: a notice before a link opens, with a checkbox to skip it on this device | done | every openable link — `http`, `https` and `mailto` — and whatever fired it, a click, a delay, a key (a further link that fires while the notice shows is dropped); skipping it opens at once only a link the viewer's own click, press, drag or key fired, one per frame — a delay, the pointer passing over a layer or a timed mouse trigger still shows the notice, without the checkbox; Open link opens it, Cancel or Escape drops it; the desktop app saves the checkbox with its preferences, and the web build keeps it until the page reloads; `proto.linkNotice {show}` turns it on or off (`present_links_tests`) | +| Open link: a notice before a link opens, with a checkbox to skip it on this device | done | every openable link — `http`, `https` and `mailto` — and whatever fired it, a click, a delay, a key (a further link that fires while the notice shows is dropped); skipping it opens at once only a link the viewer's own click, press, drag or key fired, one per frame and never while a notice shows — a delay, the pointer passing over a layer or a timed mouse trigger still shows the notice, without the checkbox; a press under way when the notice appears ends there, so its release doesn't reach the prototype behind it; Open link opens it, Cancel or Escape drops it; the desktop app saves the checkbox with its preferences, and the web build keeps it until the page reloads; `proto.linkNotice {show}` turns it on or off (`present_links_tests`) | | Opening a link in place | missing | | | Controllers on macOS, Linux and FreeBSD | missing | the usual Rust controller library links the system's libudev on Linux; the pure-Rust desktop builds read none there yet | | Prototype in a separate window; mirror to a phone | missing | see [hardware-parity](hardware-parity.md) | @@ -105,7 +105,7 @@ tool; opt-in), **silent** (the documentation doesn't say; the case pins the curr | Date | Change | Summary | |---|---|---| -| 2026-10-29 | patch | The Open link notice covers mail links, and skipping it covers only links the viewer fired: before, a mail link opened the mail app at once, also from an "After delay" trigger, and while the notice was skipped any trigger opened any number of web links. Now every openable link waits behind the notice; with it skipped, a link the viewer's own click, press, drag or key fired opens at once, one per frame, while a delay, the pointer passing over a layer or a timed mouse trigger still shows the notice, which then offers no "Don't show this again" (#1096, `crates/ui-egui/src/present_links_tests.rs`). Numbers unchanged | +| 2026-10-29 | patch | The Open link notice covers mail links, and skipping it covers only links the viewer fired: before, a mail link opened the mail app at once, also from an "After delay" trigger, and while the notice was skipped any trigger opened any number of web links. Now every openable link waits behind the notice; with it skipped, a link the viewer's own click, press, drag or key fired opens at once, one per frame, while a delay, the pointer passing over a layer or a timed mouse trigger still shows the notice, which then offers no "Don't show this again". Nothing opens while a notice shows, and a press under way when it appears ends there: before, its release could still open a link or navigate behind the notice. The mail notice says the link opens the mail app to write a message (#1096, `crates/ui-egui/src/present_links_tests.rs`). Numbers unchanged | | 2026-10-24 | patch | The Triggers row's Missing column no longer lists delays on mouse enter, leave, down and up, which landed on 2026-10-13 (`crates/engine/src/proto_delay_tests.rs`, `crates/ui-egui/src/present/delay_tests.rs`). Numbers unchanged | | 2026-10-24 | patch | Open link to a website first shows a notice that it leaves the prototype, with the link, Open link, Cancel and "Don't show this again", which the desktop app saves with its preferences and the web build keeps until the page reloads (`proto.linkNotice` turns the notice back on); a second link to a website that fires while the notice shows is dropped, and a mail link still opens. Before, the player opened the link at once, also from an After delay trigger. Numbers unchanged | | 2026-10-23 | patch | A top-level frame taller (or wider) than the device's screen scrolls with no overflow setting, as Figma's documentation says; before, "No scrolling" frames such as the landing page template's Phone frame only clipped. Content past a frame's own edges still scrolls only as its overflow allows. Checked in the app on macOS. Numbers unchanged |