Round-5 review fixes: two PR #64 regressions, undo/redo, embed, engine, a11y

css/style.css

@@ -753,6 +753,12 @@ input[type="range"] {
   background: transparent; border-color: transparent;
 }
 
+/* Reachable by assistive technology, painted nowhere. */
+.sr-only {
+  position: absolute; width: 1px; height: 1px; margin: -1px; padding: 0;
+  overflow: hidden; clip: rect(0 0 0 0); clip-path: inset(50%); white-space: nowrap; border: 0;
+}
+
 /* ── Timeline panel ── */
 #timeline {
   height: 200px; flex-shrink: 0; background: var(--panel);
@@ -1538,7 +1544,12 @@ body.embed #palette,
 body.embed #props-panel,
 body.embed #diagram-rail,
 body.embed .file-controls,
-body.embed .analysis-controls { display: none; }
+body.embed .analysis-controls,
+/* The overflow menu carries everything the collapsed topbar hides, so at any
+   width under 768px, which is most iframes, it handed every control the rules
+   above just removed straight back: the palette, the file controls, New
+   diagram, Open file, Save as JSON and the rest. */
+body.embed #btn-mobile-menu { display: none; }
 body.embed #topbar { min-height: 44px; }
 .embed-open {
   margin-left: auto; font-size: 11px; color: var(--text-faint); white-space: nowrap;

index.html

@@ -292,6 +292,10 @@
     <!-- Canvas -->
     <div id="canvas-wrap">
       <svg id="canvas" role="application" aria-label="Diagram editing canvas" tabindex="0"></svg>
+      <!-- Keyboard selection moves inside the SVG, so DOM focus never leaves the
+           canvas and a screen reader has nothing to announce. Tab and Shift+Tab
+           write what they landed on here instead. -->
+      <div id="canvas-live" class="sr-only" role="status" aria-live="polite" aria-atomic="true"></div>
       <div id="minimap" class="hidden" aria-hidden="true">
         <canvas id="minimap-canvas"></canvas>
       </div>

js/app-fields.js

@@ -224,7 +224,13 @@ class AppFields {
   // Unique id generator for programmatic label↔control association.
   _uid() { return 'fld-' + (App._fieldSeq = (App._fieldSeq || 0) + 1); }
 
-  _field(panel, label, type, value, onChange, placeholder = '') {
+  // `commitOn` is the DOM event that applies the edit. 'input' (the default)
+  // fires on every keystroke, which is what makes a field feel live. Pass
+  // 'change' for a field whose write is destructive, so it applies once the
+  // user has finished typing: typing "12" through an 'input' field commits the
+  // intermediate value 1 first, and for a Delay or Queue that physically
+  // discards the units and timers the difference stands for.
+  _field(panel, label, type, value, onChange, placeholder = '', commitOn = 'input') {
     const row = document.createElement('div');
     row.className = 'prop-row';
     const lbl = document.createElement('label');
@@ -235,7 +241,7 @@ class AppFields {
     if (placeholder) inp.placeholder = placeholder;
     if (label) { inp.id = this._uid(); lbl.htmlFor = inp.id; }
     else inp.setAttribute('aria-label', placeholder || 'value');
-    inp.addEventListener('input', () => onChange(inp.value));
+    inp.addEventListener(commitOn, () => onChange(inp.value));
     row.appendChild(lbl);
     row.appendChild(inp);
     panel.appendChild(row);

js/app-props.js

@@ -600,6 +600,12 @@ class AppProps {
   // the new timeline plots over the ghosts.
   _forkFrom(cp) {
     this.engine.stop();
+    // Leave replay first. Scrub mode makes the renderer paint a past step's
+    // values over the live model, so forking while scrubbing left the canvas
+    // showing the replayed step's numbers under the checkpoint's step label:
+    // two different moments in one view, with the slider still pointing at a
+    // history the fork has just thrown away.
+    this._exitScrub();
     this._syncRunButton();
     let kept = null;
     if (this.engine.history.length >= 2
@@ -613,6 +619,7 @@ class AppProps {
     this.renderer.flowFx.clear();
     this._clearSparklines();
     this.renderer.render();
+    this._refreshScrubber();
     this._commit();
     // The timeline is where the comparison lives — make sure it's on screen.
     if (kept && !this._timelineVisible) document.getElementById('btn-timeline').click();
@@ -799,14 +806,27 @@ class AppProps {
       // *displays* the first interactive node; the model is assigned on
       // change, which commits (mutating here would drift undo snapshots).
       const shownId = rule.nodeId || (interactives[0] && interactives[0].id);
+      // The rule's target can go away: the node is deleted, or its activation is
+      // switched off interactive. The dropdown then fell back to displaying the
+      // first interactive node, so the panel claimed the rule was wired to a
+      // node it has nothing to do with while the rule silently never fired.
+      const targetGone = !!rule.nodeId && !interactives.some(n => n.id === rule.nodeId);
+      if (targetGone) {
+        const o = document.createElement('option');
+        o.value = ''; o.textContent = '(node deleted)'; o.selected = true;
+        ns.appendChild(o);
+      }
       for (const n of interactives) {
         const o = document.createElement('option');
         o.value = n.id; o.textContent = n.label || n.type;
-        if (n.id === shownId) o.selected = true;
+        if (!targetGone && n.id === shownId) o.selected = true;
         ns.appendChild(o);
       }
       ns.addEventListener('change', () => { rule.nodeId = ns.value; this._commit(); });
       nodeRow.appendChild(nl); nodeRow.appendChild(ns); box.appendChild(nodeRow);
+      if (targetGone) {
+        this._info(box, 'This rule points at a node that is gone or is no longer interactive, so it never fires. Pick another node.');
+      }
 
       // Firing mode.
       const modeRow = document.createElement('div'); modeRow.className = 'prop-row';
@@ -1468,6 +1488,9 @@ class AppProps {
       // node.resources is the live count once a run has stepped; only at rest
       // does this field set the baseline that Reset returns to. Label it
       // honestly here, and keep it honest per step in _refreshResourceCount.
+      // A Delay's or Queue's amount edit moves the in-flight pipeline, so it
+      // must not run per keystroke: see the commitOn note on _field.
+      const pipelineNode = node.type === NodeType.DELAY || node.type === NodeType.QUEUE;
       this._field(panel, this.engine.step > 0 ? AMOUNT_LABEL_LIVE : AMOUNT_LABEL_AT_REST, 'number', node.resources, v => {
         // Keep the node's own colour. setCount defaults to DEFAULT_COLOR, so
         // retyping a pool's amount used to convert every typed resource it held
@@ -1493,7 +1516,7 @@ class AppProps {
         // number directly above the new one.
         this._refreshResourceCount();
         this._refreshTypeReadouts();
-      });
+      }, '', pipelineNode ? 'change' : 'input');
       // Quick +/- steppers for adjusting the current amount during play
       // (these nudge the live value without changing the starting baseline).
       const stepRow = document.createElement('div');

js/app.js

@@ -505,7 +505,7 @@ class App {
   _commitReplace(prevSnap) {
     this._dropScenarioState();
     const snap = this._snapshot();
-    if (snap === prevSnap) { this._lastState = snap; this._updateUndoButtons(); return; }
+    if (this._sameSnapshot(snap, prevSnap)) { this._lastState = snap; this._updateUndoButtons(); return; }
     if (prevSnap != null) {
       this._undoStack.push(prevSnap);
       if (this._undoStack.length > 100) this._undoStack.shift();
@@ -516,11 +516,31 @@ class App {
     this._persistAutosave();
   }
 
+  // Two snapshots describe the same diagram. The module-level id counter rides
+  // along in the JSON and loadJSON only ever raises it, so that ids handed out
+  // since cannot collide, which means restoring a snapshot never reproduces its
+  // own text. Comparing the raw strings therefore reported a change after every
+  // undo, and the next commit, even one that changed nothing at all, pushed an
+  // undo entry and wiped the redo stack.
+  _sameSnapshot(a, b) {
+    if (a === b) return true;
+    if (a == null || b == null) return false;
+    try {
+      const strip = (j) => { const o = JSON.parse(j); delete o._idSeq; return JSON.stringify(o); };
+      return strip(a) === strip(b);
+    } catch { return false; }
+  }
+
   // Mirror the current state into the autosave slot. Every path that changes
   // what is on the canvas has to call this: undo and redo moved _lastState
   // without it, so an undo looked repaired on screen and was thrown away on the
   // next reload, taking the mistake it had just undone with it.
   _persistAutosave() {
+    // Never in embed mode. The chrome is hidden there but the canvas is still
+    // editable, and sim_autosave is same-origin: a visitor who nudged a node in
+    // someone's embedded diagram had their own saved work silently replaced by
+    // it, and found the embed's diagram waiting for them on their next visit.
+    if (document.body.classList.contains('embed')) return;
     try { localStorage.setItem('sim_autosave', this._lastState); } catch { /* blocked storage */ }
   }
 
@@ -530,7 +550,7 @@ class App {
   // delegated commit listener) don't create empty undo steps.
   _commit() {
     let snap = this._snapshot();
-    if (snap === this._lastState) return;
+    if (this._sameSnapshot(snap, this._lastState)) return;
     // A real change happened: bump the file's modified timestamp (it is part
     // of the snapshot, so re-take it after stamping).
     this.diagram.meta.modified = Date.now();
@@ -560,6 +580,7 @@ class App {
   }
 
   undo() {
+    this.editor.flushPending();
     if (!this._undoStack.length) return;
     this._redoStack.push(this._lastState);
     this._lastState = this._undoStack.pop();
@@ -569,6 +590,7 @@ class App {
   }
 
   redo() {
+    this.editor.flushPending();
     if (!this._redoStack.length) return;
     this._undoStack.push(this._lastState);
     this._lastState = this._redoStack.pop();
@@ -685,7 +707,10 @@ class App {
     if (!play) return;
     const on = !!this._scrubPlayTimer;
     play.replaceChildren(this._faIcon(on ? 'pause' : 'play'));
+    // The icon and the tooltip swapped but the accessible name was markup, so
+    // the control announced "Replay the run" while it was the Pause button.
     play.title = on ? 'Pause replay' : 'Replay the run';
+    play.setAttribute('aria-label', play.title);
   }
 
   // Auto-advance through history at the current sim speed; stops at the end.
@@ -772,7 +797,25 @@ class App {
       name.id = 'embed-title';
       tail.appendChild(name);
       const link = document.createElement('a');
-      link.href = location.href.replace(/([?&])embed(=[^&]*)?/, '$1').replace(/[?&]$/, '');
+      // Rebuild the URL without the embed marker, wherever it came from. The
+      // marker is accepted in the query (?embed) and in the hash (#embed, or
+      // #d=...&embed, which is what the knowledge base documents), but only the
+      // query form was ever stripped: for a hash embed the link pointed back at
+      // the embed itself, so the one escape hatch an embed offers did nothing.
+      link.href = (() => {
+        try {
+          const u = new URL(location.href);
+          u.searchParams.delete('embed');
+          u.hash = u.hash
+            .replace(/(^#|&)embed\b(=[^&]*)?/g, '$1')
+            .replace(/^#&/, '#')
+            .replace(/&&+/g, '&')
+            .replace(/[#&]$/, '');
+          return u.toString();
+        } catch {
+          return location.href.replace(/([?&])embed(=[^&]*)?/, '$1').replace(/[?&]$/, '');
+        }
+      })();
       link.target = '_blank'; link.rel = 'noopener';
       link.textContent = 'open in Simulations ↗';
       tail.appendChild(link);
@@ -1509,7 +1552,11 @@ class App {
     const zoomLabel = document.getElementById('btn-zoom-level');
     zoomLabel.addEventListener('click', () => this.renderer.zoomTo(1));
     this.renderer.onViewChange = (scale) => {
-      zoomLabel.textContent = `${Math.round(scale * 100)}%`;
+      const pct = Math.round(scale * 100);
+      zoomLabel.textContent = `${pct}%`;
+      // The visible readout is live but the accessible name was markup, so it
+      // announced "100%" at every zoom level.
+      zoomLabel.setAttribute('aria-label', `Current zoom ${pct}%. Click to reset to 100%`);
       this._minimap.update();
     };
     this.renderer.onViewChange(this.renderer._scale);

js/editor.js

@@ -857,8 +857,25 @@ class Editor {
   // Select a node from the keyboard and make sure it can actually be seen.
   // Only pans when the node is outside the viewport: recentring on every step
   // would make the whole diagram lurch while tabbing between visible nodes.
+  // Say what the keyboard just landed on. Selection moves inside the SVG, so
+  // DOM focus never moves and assistive technology is told nothing at all:
+  // tabbing through a diagram was completely silent. The properties panel does
+  // repaint, but it is not focused and not a live region.
+  _announceSelection(node, index, total) {
+    const live = document.getElementById('canvas-live');
+    if (!live || !node) return;
+    const name = node.label || node.type;
+    const value = node.chartValue;
+    const parts = [`${node.type} ${name}`];
+    if (isFinite(value)) parts.push(String(value));
+    parts.push(`${index + 1} of ${total}`);
+    live.textContent = parts.join(', ');
+  }
+
   _keyboardGoTo(node) {
     if (!node) return;
+    const order = this._nodesInReadingOrder();
+    this._announceSelection(node, order.findIndex(n => n.id === node.id), order.length);
     this._select(node.id, 'node');
     const r = this.svg.getBoundingClientRect();
     const sx = node.x * this.renderer._scale + this.renderer._panX;
@@ -1032,10 +1049,22 @@ class Editor {
       for (const t of targets) { t.x += dx; t.y += dy; }
       this.renderer.render();
       clearTimeout(this._nudgeTimer);
-      this._nudgeTimer = setTimeout(() => this._changed(), 400);
+      this._nudgeTimer = setTimeout(() => { this._nudgeTimer = null; this._changed(); }, 400);
     }
   }
 
+  // Commit a nudge that is still inside its coalescing window. Arrow-key moves
+  // are batched for 400ms so a held key is one undo step, but anything that
+  // reads the undo history has to see the moves the user has already made:
+  // with the commit still pending, Ctrl+Z stepped straight past the nudge and
+  // undid whatever edit came before it.
+  flushPending() {
+    if (!this._nudgeTimer) return;
+    clearTimeout(this._nudgeTimer);
+    this._nudgeTimer = null;
+    this._changed();
+  }
+
   _onKeyUp(e) {
     if (e.code === 'Space') {
       // Always clear, even behind a dialog: _onKey may have swallowed the

js/engine.js

@@ -24,6 +24,12 @@ class SimEngine {
   }
 
   saveInitial() {
+    // One capture per run. run() takes the baseline at step 0 and then the
+    // first doStep() took it again, so anything that changed state in between,
+    // in particular clicking an interactive node while waiting for the first
+    // tick, was captured as the diagram's authored starting amount and Reset
+    // could never get the real one back.
+    this._baselineSaved = true;
     for (const n of this.diagram.nodes.values()) {
       const infiniteSource = n.type === NodeType.SOURCE && !n.limited;
       n._initialResources = infiniteSource ? Infinity : n.resources;
@@ -105,6 +111,7 @@ class SimEngine {
     // a prior Monte Carlo batch may have left on the shared RNG.
     SimRandom.seed(this.diagram.seed || null);
     this.step = 0;
+    this._baselineSaved = false;
     this.history = [];
     this._histStride = 1;
     // Flow accumulator for spike attribution: resource amounts per connection
@@ -201,7 +208,7 @@ class SimEngine {
 
   doStep() {
     if (this.step === 0) {
-      this.saveInitial();
+      if (!this._baselineSaved) this.saveInitial();
       this._seedPipelines();
       this._updateVariables();
       this._evalRegisters();
@@ -237,7 +244,7 @@ class SimEngine {
     // Pressing Play resamples 'play'-updated random variables once.
     this._sampleCustomVars('play');
     if (this.step === 0) {
-      this.saveInitial();
+      if (!this._baselineSaved) this.saveInitial();
       this._seedPipelines();
       this._updateVariables();
       this._evalRegisters();
@@ -254,6 +261,11 @@ class SimEngine {
   fireInteractive(nodeId) {
     const node = this.diagram.nodes.get(nodeId);
     if (!node || node.activation !== ActivationMode.INTERACTIVE) return;
+    // Firing is a play action, never an authoring one, so the baseline Reset
+    // returns to has to be whatever was on the canvas before the click. At step
+    // 0 it may not have been captured yet (Run takes it, but the click can land
+    // in the gap before the first tick, and Step has not run at all).
+    if (this.step === 0 && !this._baselineSaved) this.saveInitial();
     const ctx = this._makeCtx();
     const fired = [];
     this._runFireQueue([{ node, forced: true }], ctx, fired);
@@ -978,7 +990,14 @@ class SimEngine {
       return movedAny;
     }
 
-    // Deterministic: split proportionally to output weights.
+    // Deterministic: split proportionally to output weights. A weight of 0 is
+    // off, which is what the panel says ("0 = off") and what it shows (0%), so
+    // a gate whose outputs are all 0 routes nothing. _proportionalShares falls
+    // back to an even split when no weight is positive, which is right for a
+    // delay handing a matured batch to its outputs but is the opposite of what
+    // a gate's weights mean: it emptied the gate into every output at once,
+    // while the Random and All modes correctly held everything.
+    if (weights.reduce((a, b) => a + b, 0) <= 0) return movedAny;
     const shares = this._proportionalShares(node.resources, weights);
 
     outs.forEach((conn, i) => {
@@ -1069,7 +1088,15 @@ class SimEngine {
       // pays away (a full pool can still swap like-for-like).
       const canAccept = (n, recv, pays) => {
         if (recv <= 0) return true;
-        if (n.type === NodeType.SOURCE || n.type === NodeType.REGISTER) return false;
+        // Mirrors the general intake rule in _acceptable: a trader never holds
+        // resources, its connections are trade routes. This list was missing
+        // TRADER, so a trader used as another trader's partner accepted the
+        // payment and kept it forever, invisibly: its canvas number, chart
+        // value and history entry all report the trade count, not what it
+        // holds, so the units simply left the economy and a mid-run save
+        // carried them into the file as a hidden holding.
+        if (n.type === NodeType.SOURCE || n.type === NodeType.REGISTER
+          || n.type === NodeType.TRADER) return false;
         if (n.capacity === Infinity || n.type === NodeType.DRAIN) return true;
         return n.capacity - n.resources - (ctx.reserved.get(n.id) || 0) + pays >= recv;
       };
@@ -1519,8 +1546,13 @@ class SimEngine {
           r = job.next();
         }
         if (r.done) { resolve(r.value); return; }
-        // Mid-trial breaths carry no new progress; only completed trials do.
-        if (opts.onProgress && !r.value.partial) opts.onProgress(r.value.done, r.value.total);
+        // Every yield carries the number of trials finished so far, mid-trial
+        // breaths included, so the chunk's last yield is always the current
+        // count. Reporting only on a non-partial yield reported almost never:
+        // with hundreds of steps per trial, well under 1% of yields end a
+        // trial, so a chunk hardly ever stopped on one and the dialog sat at
+        // "Running…" and a 0% bar for the whole batch.
+        if (opts.onProgress) opts.onProgress(r.value.done, r.value.total);
         setTimeout(tick, 0);
       };
       tick();