Skip to content

Commit 51e4490

Browse files
committed
fix(diagrams): #104 review — the full-size view is modal, the picture is not a button, and one diagram is one card
Two findings from the Copilot review, both right. Checking them in a browser, with the messages in the order a real run sends them, showed three more faults in the same code: the page's bookkeeping of which card a diagram is drawn in. chat.html:1523 — "this is declared modal, but opening it neither makes the background inert nor traps Tab focus". The full-size view said aria-modal="true" and covered the page, and the chat behind it could still be tabbed into. While it is open, every other child of the body is now inert — not focusable, not clickable, not read out — and released when it closes, before the focus is handed back (an inert element cannot take it). Tab goes round inside: past the last stop is the first, before the first is the last, and a focus that got outside is brought back. A linked node in the view could be tabbed to and did nothing on Enter; it opens its file now, as a click does. chat.html:4442 — "the stage is exposed as a button while linked SVG nodes inside it are independently focusable links". A button's content is no place for other controls. The picture is no longer a control: no role, no tab stop, no name. Opening it full size is a button of its own, first in the toolbar ("Full size"), and that is what the keyboard and a screen reader use; a click on the picture still does the same, as a shortcut for the pointer. Where there is no picture — the text fallback — there is no such button. The three it turned up. agent.js announces every render_diagram call the moment it starts streaming, before anyone can know whether it is a new diagram, a repair or a redraw, and the page's checks had never been played in that order. - A redraw left BOTH drawings on screen, where a reopened session shows one. The new call had made a placeholder of its own, the record was drawn in it, and the drawing it replaces stayed. The earlier one now goes when its replacement arrives, and the redraw stands where the model drew it again — whether or not the call announced itself. - A different diagram arriving while one was being repaired painted over it. The page lets the next call take over a card that is waiting for a repair, which is right when it is the repair. When it is not, the host settles the waiting diagram into that card, and the newcomer then drew over it: two diagrams on file, one on screen. A record now has a claim on a card that is still a placeholder, or that shows the drawing it replaces, and on no other. - A tool call named `__proto__` broke the ones after it. The table of cards was a plain object keyed by tool-call id, a name a provider chooses. `__proto__` made that card the table's prototype; the next lookup of `parentNode` threw "Illegal invocation", and that diagram was never shown. The table has no prototype now. Tests. The gate cannot boot the page, and until now it only pinned its source; every pin was green through all five faults. Two parts of the page are now RUN there, sliced out of the file. The card bookkeeping, against a stand-in for the few DOM calls it makes: a repair, two calls in one turn, a redraw with and without a placeholder of its own, a change of subject, a record with no claim on the card it points at, seven tool-call ids that are also names the table could inherit, a wiped transcript, and the sweep at the end of a run — each judged by what is on screen and where. And the function that decides where Tab goes. The browser check gains the modal's behaviour in real Chrome — inert, a focus that cannot leave, Tab at both ends, Enter on a node, Escape and the focus back on the button — and a page that plays seven runs in the real order, then replays each as a reopened session and compares the two. 186 checks, up from 155. The real-editor check opens the view from its button in the real webview and confirms the page behind it is inert there: 19 checks, up from 17.
1 parent 49d16b8 commit 51e4490

4 files changed

Lines changed: 444 additions & 23 deletions

File tree

‎extensions/levelcode-ai/media/chat.html‎

Lines changed: 65 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1432,8 +1432,10 @@
14321432
border: 1px solid var(--vscode-inputValidation-warningBorder, rgba(224,160,48,.45));
14331433
}
14341434
.lcd-banner-text { flex: 1 1 12em; min-width: 0; }
1435-
.lcd-stage { cursor: zoom-in; border-radius: 8px; outline-offset: 3px; }
1436-
.lcd-stage:focus-visible { outline: 1px solid var(--accent); }
1435+
/* A click on the picture opens it full size — a shortcut for the pointer only. The picture itself is
1436+
not a control (its linked nodes are, and a control inside a control is nobody's to announce); the
1437+
toolbar's "Full size" button is the way in for the keyboard and for a screen reader. */
1438+
.lcd-stage { cursor: zoom-in; border-radius: 8px; }
14371439
/* "Never wider than the chat column": the picture is laid out to fit, and scales down if the column narrows. */
14381440
.lcd-stage svg { display: block; max-width: 100%; height: auto; }
14391441
.lcd-btn {
@@ -4304,7 +4306,9 @@
43044306
// SVG elements from a fixed allow-list and sets text with textContent. The card chrome below is
43054307
// built with createElement + textContent too. Nothing from a spec is ever assigned to innerHTML.
43064308
const LCD = window.LCDiagram || null; // null if the modules did not load → the host's text version is shown instead
4307-
const lcdCards = {}; // tool-call key → the card that shows (or will show) that diagram
4309+
// tool-call key → the card that shows (or will show) that diagram. The key is whatever the provider
4310+
// called its tool call, so the table inherits nothing: `constructor` and `__proto__` are just keys.
4311+
const lcdCards = Object.create(null);
43084312
let lcdSeq = 0;
43094313
const lcdCtx = (function(){ try { return document.createElement('canvas').getContext('2d'); } catch (e) { return null; } })();
43104314
function lcdFont(){ try { return getComputedStyle(document.body).fontFamily || 'sans-serif'; } catch (e) { return 'sans-serif'; } }
@@ -4372,13 +4376,34 @@
43724376
card.appendChild(skel);
43734377
scrollIfStuck();
43744378
}
4375-
/** A finished record from the host: fill the placeholder that was waiting for it, or make a card. */
4379+
/** Take a card out of the transcript — and the bubble that held it, if nothing else is in it. */
4380+
function lcdDrop(card){
4381+
const body = card.parentElement, msg = body && body.parentElement;
4382+
card.remove();
4383+
Object.keys(lcdCards).forEach(function(k){ if (lcdCards[k] === card){ delete lcdCards[k]; } });
4384+
if (body && !body.childElementCount && !body.textContent.trim() && msg && msg.classList.contains('msg')){ msg.remove(); }
4385+
}
4386+
/**
4387+
* A finished record from the host: fill the placeholder that was waiting for it, or make a card.
4388+
* One diagram, one card — the same cards a reopened session shows (sessionEvents.toDisplayTurns).
4389+
* A record has a claim on a card that is still a placeholder, or that shows the drawing it replaces,
4390+
* and on no card that shows a different diagram.
4391+
*/
43764392
function lcdShow(m){
43774393
const record = m && m.record; if (!record || !record.id){ return; }
43784394
const key = m.key || record.key || record.id;
4379-
let card = lcdCards[key] || (m.replacesKey ? lcdCards[m.replacesKey] : null);
4380-
if (card && !card.isConnected){ card = null; }
4395+
const there = function(c){ return c && c.isConnected ? c : null; };
4396+
const takes = function(c){ return !!c && (!c.dataset.id || c.dataset.id === record.id || c.dataset.id === String(record.replaces || '')); };
4397+
let card = there(lcdCards[key]);
4398+
const old = m.replacesKey ? there(lcdCards[m.replacesKey]) : null;
4399+
// The placeholder this call took over when it began may have been filled since: the host settled
4400+
// the diagram it was holding a place for, and this is a different one. That one keeps its card.
4401+
if (!takes(card)){ card = null; }
4402+
// A repair is drawn where the failed attempt was waiting. A redraw stands where the model drew it
4403+
// again, and the earlier drawing goes — as it does when the chat is reopened.
4404+
if (!card && old && !old.dataset.id){ card = old; }
43814405
if (!card){ card = lcdHost(key); }
4406+
if (old && old !== card && takes(old)){ lcdDrop(old); }
43824407
lcdCards[key] = card;
43834408
lcdDraw(card, record, false);
43844409
scrollIfStuck();
@@ -4439,7 +4464,7 @@
44394464
const descId = 'lcd-desc-' + (++lcdSeq);
44404465
svg.setAttribute('aria-describedby', descId);
44414466
stage.appendChild(svg);
4442-
stage.tabIndex = 0; stage.setAttribute('role', 'button'); stage.setAttribute('aria-label', 'Open the diagram full size'); stage.title = 'Click to open full size';
4467+
stage.title = 'Click to open full size';
44434468
const desc = lcdEl('p', 'lcd-sr', LCD.text.outline(spec).text); desc.id = descId; card.appendChild(desc);
44444469
card._spec = spec; card._geo = geo;
44454470
card.appendChild(lcdTools(card, record, false));
@@ -4527,10 +4552,11 @@
45274552
img.src = 'data:image/svg+xml;charset=utf-8,' + encodeURIComponent(svg.text);
45284553
});
45294554
}
4530-
/** Copy source · SVG · PNG · Open as Mermaid · Insert into Markdown. */
4555+
/** Full size · Copy source · SVG · PNG · Open as Mermaid · Insert into Markdown. */
45314556
function lcdTools(card, record, textOnly){
45324557
const bar = lcdEl('div', 'lcd-tools');
45334558
const fail = function(e){ vscode.postMessage({ type: 'notice', text: 'The diagram could not be exported: ' + String((e && e.message) || e) }); };
4559+
if (!textOnly){ bar.appendChild(lcdBtn('Full size', 'Open the diagram full size, to zoom and pan', function(){ lcdZoomOpen(card); })); }
45344560
const copy = lcdBtn('Copy source', 'Copy the diagram’s spec', function(){ lcdAct(record.id, 'export', { format: 'source' }); lcdFlash(copy, 'Copied'); });
45354561
bar.appendChild(copy);
45364562
if (!textOnly){
@@ -4556,16 +4582,15 @@
45564582
function lcdSweep(){
45574583
Object.keys(lcdCards).forEach(function(k){
45584584
const card = lcdCards[k];
4559-
if (card.dataset.id){ return; }
4560-
const body = card.parentElement, msg = body && body.parentElement;
4561-
card.remove(); delete lcdCards[k];
4562-
if (body && !body.childElementCount && !body.textContent.trim() && msg && msg.classList.contains('msg')){ msg.remove(); }
4585+
if (card && !card.dataset.id){ lcdDrop(card); }
45634586
});
45644587
}
45654588
function lcdReset(){ Object.keys(lcdCards).forEach(function(k){ delete lcdCards[k]; }); lcdZoomClose(); }
45664589

4567-
// Full-size view: zoom about the pointer, drag to pan, fit and 100%.
4568-
const lcdZ = { el: document.getElementById('lcdZoom'), view: null, svg: null, card: null, k: 1, x: 0, y: 0, w: 0, h: 0, back: null };
4590+
// Full-size view: zoom about the pointer, drag to pan, fit and 100%. It says it is modal, so it is:
4591+
// while it is open the rest of the page is inert — not focusable, not clickable, not read out — and
4592+
// Tab goes round inside it. Closing gives the page back, and the focus to where it was.
4593+
const lcdZ = { el: document.getElementById('lcdZoom'), view: null, svg: null, card: null, k: 1, x: 0, y: 0, w: 0, h: 0, back: null, behind: [] };
45694594
lcdZ.view = lcdZ.el.querySelector('.lcdz-view');
45704595
function lcdZoomApply(){ if (lcdZ.svg){ lcdZ.svg.style.transform = 'translate(' + lcdZ.x + 'px,' + lcdZ.y + 'px) scale(' + lcdZ.k + ')'; } }
45714596
function lcdZoomFit(){
@@ -4582,7 +4607,12 @@
45824607
lcdZ.view.textContent = '';
45834608
const svg = src.cloneNode(true);
45844609
svg.removeAttribute('aria-describedby');
4585-
lcdZ.w = card._geo.width; lcdZ.h = card._geo.height; lcdZ.svg = svg; lcdZ.card = card; lcdZ.back = document.activeElement;
4610+
lcdZ.w = card._geo.width; lcdZ.h = card._geo.height; lcdZ.svg = svg; lcdZ.card = card;
4611+
if (lcdZ.el.hidden){
4612+
lcdZ.back = document.activeElement;
4613+
lcdZ.behind = Array.prototype.filter.call(document.body.children, function(n){ return n !== lcdZ.el && !n.inert && !/^(SCRIPT|STYLE|TEMPLATE)$/.test(n.tagName); });
4614+
lcdZ.behind.forEach(function(n){ n.inert = true; });
4615+
}
45864616
svg.style.width = lcdZ.w + 'px'; svg.style.height = lcdZ.h + 'px';
45874617
lcdZ.view.appendChild(svg);
45884618
lcdZ.el.querySelector('.lcdz-title').textContent = (card._spec && card._spec.title) || 'Diagram';
@@ -4593,7 +4623,21 @@
45934623
function lcdZoomClose(){
45944624
if (!lcdZ.el || lcdZ.el.hidden){ return; }
45954625
lcdZ.el.hidden = true; lcdZ.view.textContent = ''; lcdZ.svg = null; lcdZ.card = null;
4626+
lcdZ.behind.forEach(function(n){ n.inert = false; }); lcdZ.behind = []; // before the focus goes back: an inert element cannot take it
45964627
try { if (lcdZ.back && lcdZ.back.focus){ lcdZ.back.focus(); } } catch (e) {}
4628+
lcdZ.back = null;
4629+
}
4630+
/** Where Tab stops in the full-size view: its buttons, then the picture's linked nodes. */
4631+
function lcdZoomStops(){ return Array.prototype.slice.call(lcdZ.el.querySelectorAll('button:not([disabled]), [data-lc-link]')); }
4632+
/**
4633+
* Tab in the full-size view. Returns where the focus has to be PUT: past the last stop is the first,
4634+
* before the first is the last, and from anywhere outside the view it is brought back in. Null when
4635+
* the browser's own next stop is already inside, and Tab can be left to it.
4636+
*/
4637+
function lcdZoomTab(stops, active, back){
4638+
const at = stops.indexOf(active);
4639+
if (at >= 0 && at !== (back ? 0 : stops.length - 1)){ return null; }
4640+
return stops[back ? stops.length - 1 : 0] || active || null;
45974641
}
45984642
lcdZ.el.querySelector('.lcdz-bar').addEventListener('click', function(e){
45994643
const b = e.target.closest && e.target.closest('[data-z]'); if (!b){ return; }
@@ -4623,13 +4667,17 @@
46234667
document.addEventListener('keydown', function(e){
46244668
if (lcdZ.el.hidden){ return; }
46254669
const r = lcdZ.view.getBoundingClientRect();
4670+
const link = e.target && e.target.closest && lcdZ.view.contains(e.target) ? e.target.closest('[data-lc-link]') : null;
46264671
if (e.key === 'Escape'){ e.stopPropagation(); e.preventDefault(); lcdZoomClose(); }
4672+
else if (e.key === 'Tab'){ const to = lcdZoomTab(lcdZoomStops(), document.activeElement, e.shiftKey); if (to){ e.preventDefault(); to.focus(); } }
4673+
else if ((e.key === 'Enter' || e.key === ' ') && link && lcdZ.card){ e.preventDefault(); lcdAct(lcdZ.card.dataset.id, 'openLink', { node: link.getAttribute('data-lc-link') }); }
46274674
else if (e.key === '+' || e.key === '='){ lcdZoomBy(1.25, r.width / 2, r.height / 2); }
46284675
else if (e.key === '-'){ lcdZoomBy(0.8, r.width / 2, r.height / 2); }
46294676
else if (e.key === '0'){ lcdZoomFit(); }
46304677
else if (e.key === '1'){ lcdZoomBy(1 / lcdZ.k, r.width / 2, r.height / 2); }
46314678
}, true);
4632-
// In the transcript: a linked node opens its file; anywhere else on the picture opens it full size.
4679+
// In the transcript: a linked node opens its file — by click, or by Enter/Space when it has the
4680+
// focus; a click anywhere else on the picture opens it full size.
46334681
// The webview only ever names the NODE — which file that means is the host's own record.
46344682
function lcdActivate(e){
46354683
const t = e.target; if (!t || !t.closest){ return false; }
@@ -4640,7 +4688,7 @@
46404688
return false;
46414689
}
46424690
log.addEventListener('click', function(e){ if (lcdActivate(e)){ e.preventDefault(); } });
4643-
log.addEventListener('keydown', function(e){ if ((e.key === 'Enter' || e.key === ' ') && e.target && e.target.closest && e.target.closest('[data-lc-link], .lcd-stage') && lcdActivate(e)){ e.preventDefault(); } });
4691+
log.addEventListener('keydown', function(e){ if ((e.key === 'Enter' || e.key === ' ') && e.target && e.target.closest && e.target.closest('[data-lc-link]') && lcdActivate(e)){ e.preventDefault(); } });
46444692
// The column changed width: re-lay out any diagram that would now turn (or stop needing to).
46454693
let lcdResizeT = null;
46464694
window.addEventListener('resize', function(){

0 commit comments

Comments
 (0)