Skip to content

web: put the viewer's visual language on a token scale - #11245

Open
maliberty wants to merge 6 commits into
The-OpenROAD-Project:masterfrom
The-OpenROAD-Project-staging:web-design-tokens
Open

web: put the viewer's visual language on a token scale#11245
maliberty wants to merge 6 commits into
The-OpenROAD-Project:masterfrom
The-OpenROAD-Project-staging:web-design-tokens

Conversation

@maliberty

Copy link
Copy Markdown
Member

The web viewer's stylesheet had accreted rather than been designed: a VS Code
palette, eight near-identical button rules, four byte-identical tooltip blocks,
and nineteen rules setting bare monospace. Four commits put it on a token
scale and consolidate what the tokens made redundant. No behaviour changes
beyond the visual ones described below.

Best reviewed commit by commit — each one stands alone.

1. Colours and radii on a token scale

Each theme block defines the palette as scales (--surface-*, --line-*,
--ink-*, --accent-*, --sem-*) and every role name the component rules
already use becomes an alias for a scale step. No role name is added or
removed, so no component rule had to change and a palette edit is now a scale
edit.

  • Three deliberate surface elevation steps, cool-biased, replacing four values
    that sat inside 8% lightness of each other.
  • A saturated accent. --bg-selected and --accent stay a solid step dark
    enough to carry --fg-white at 4.5:1 in both themes; the row highlights
    that carry --fg-primary are the tints.
  • Light mode picked against its own ground rather than derived from dark —
    --bg-hover and --border sharing #d0d0d0 was a symptom of that.
  • --ink-3 chosen so --fg-muted clears 4.5:1 on every surface it appears
    over.
  • Radii collapse to --radius-xs/sm/md, picked by what the element is.
  • color-scheme per theme, so native scrollbars and <select> popups follow
    it, and accent-color so checkboxes follow the accent.

2. Buttons, tooltips and canvas overlays

  • Eight button rules — .toolbar-button, .timing-btn, .drc-btn,
    .inspector-btn, .sb-tool, .heatmap-rebuild, .or-modal-btn and a bare
    .modal-dialog button — become .or-btn with size and emphasis variants.
    Eleven buttons had no class at all and were rendering as browser defaults
    (nine in the schematic toolbar, two in the 3D viewer overlay); they join the
    rest.
  • Four identical tooltip blocks become .or-tooltip.
  • The scale bar, coordinate readout, heat-map legend and pending indicator
    share .or-hud, and the scale bar and coordinate readout merge into one HUD
    instead of two stacked chips.
  • A :focus-visible ring — the file had none, so keyboard navigation was
    invisible — a 120ms transition on buttons and list rows, and a
    prefers-reduced-motion block.

3. Type roles and data tables

  • --font-mono with a real stack. Bare monospace resolves to Courier on
    several systems.
  • Six panels were set entirely in mono, so their toolbars and headers were too
    and they read as log output. Mono is now for values; labels take
    --font-sans. The canvas widgets get both stacks from getThemeColors(),
    so canvas and DOM agree.
  • .timing-table and .sb-table drop the border under every cell, headers go
    sans, and the sticky header gets a shadow.
  • Numeric columns are right-aligned with tabular figures, declared in the
    column definition rather than inferred at render time.
  • Slack cells carry a bar growing from the cell midpoint, scaled by the largest
    |slack| in the table.

4. Canvas frame, panel tabs, status chrome

  • The layout canvas gets a hairline, drawn as an overlay: a border would change
    the size Leaflet measures, and an inset shadow would paint under the tile
    panes.
  • Golden Layout's active tab gets the accent underline the timing widget
    already uses, and each tab gets its panel's icon.
  • The disconnect banner pulsed forever; it settles after three beats. Tile
    requests in flight become a 2px line on the canvas rather than a grey chip.

Things worth a reviewer's attention

Qt parity was the deciding factor twice, against my own initial read. I had
flagged the duplicate colours in clock-tree-widget.js as a collision to fix.
They mirror clockWidget.h:168-174, which deliberately uses Qt::red for both
the root and the leaf register and Qt::darkCyan for both the inverter and the
leaf macro. Same for the charts palette, which transcribes
chartsWidget.cpp:1052-1062. Both palettes are left alone, and the slack bars
reuse those Qt colours so the table reads as the same measure as the charts
panel.

The canvas overlays cannot follow the theme. They sit on --bg-map, which
is black in both themes for Qt parity, so a light-theme HUD was a bright block
on black with unreadable ink. --bg-hud, --border-hud, --fg-hud and
--fg-hud-muted are theme-independent for that reason; the ruler labels and
label handles come off them too. The toast is not on the canvas, so it stays on
the theme.

The tab icons need a re-measure. They widen every tab, and Golden Layout
decides which tabs fit before they exist, so without it the overflowing tabs
overlap the stack's controls instead of moving into the dropdown.
decorateTabIcons returns a count and the caller re-measures only when
something was added, which also keeps that call from looping through the layout
event that triggered it. Header height is a dimensions.headerHeight config
value because GL measures it in JS; a saved layout carries the dimensions it was
saved with, so the restored config is overridden rather than bumping
LAYOUT_VERSION and discarding everyone's panel arrangement.

Two contrast bugs that predate this branch. .fb-entry.fb-selected painted
--bg-selected but inherited --fg-primary, which was 2.6:1 in light mode.
And .gc-invalid marked an invalid field with an outline, which the new focus
ring would have replaced — so a field would stop looking invalid exactly while
being fixed. It is a ring now.

test-status-indicator.js reimplements updateStatus instead of importing
it
, so it passes against its own copy regardless of what main.js does. I
synced it, but this is the third commit in a row that needed hand-syncing.
Extracting updateStatus would fix it properly; I left that alone because
main.js builds the whole app at import time.

Not in this branch

index.html loads Leaflet, both Golden Layout themes and netlistsvg from three
CDNs, so on a locked-down network the viewer renders unstyled. Vendoring them is
a robustness fix rather than a design one and belongs in its own PR.

A density toggle is now cheap — the space and radius scales exist — but is not
here.

Verification

695/695 JS tests pass. Every token pair carrying text audited to 4.5:1 or
better in both themes. Rendered in headless Chrome in both themes throughout,
including against a real Golden Layout loaded from its CDN, which is what caught
the border-bottom: none override, the stale tab measurement, and the
box-sizing: content-box !important the header forces on its children.

Not yet run against a real design in a browser: the scale bar reserves 22px of
horizontal padding for its end labels, and how that sits inside the merged HUD
is the one thing a harness cannot tell me.

@maliberty maliberty self-assigned this Aug 26, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request refactors the web UI styling by introducing a unified button system (or-btn), consolidating floating overlays into a shared HUD component (or-hud), adding panel tab icons, and restructuring CSS variables into a two-layer theme system. The review feedback highlights several improvement opportunities: preventing text clipping on non-delete action buttons by conditionally applying the icon class, clearing and syncing the coordinate readout HUD on mouseout to avoid stale data, cleaning up Leaflet map and DOM references on panel destruction to prevent memory leaks, and avoiding premature marking of tabs with data-or-icon before their titles are initialized.

Comment thread src/web/src/inspector.js
Comment on lines 1098 to 1106
const btn = document.createElement('button');
btn.className = 'inspector-btn inspector-action-btn';
btn.className = 'or-btn or-btn-icon inspector-action-btn';
btn.title = name;
if (name === 'Delete') {
btn.innerHTML = DELETE_SVG;
btn.classList.add('inspector-btn-danger');
btn.classList.add('or-btn-danger-hover');
} else {
btn.textContent = name;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Applying the or-btn-icon class unconditionally to all action buttons assumes they will always render as icons. However, if an action is not 'Delete', it renders as text (btn.textContent = name). The or-btn-icon class enforces a fixed square width (width: var(--or-btn-h)), which will cause any text-based action buttons to overflow or be clipped. It is safer to only apply or-btn-icon when the button actually contains an icon.

Suggested change
const btn = document.createElement('button');
btn.className = 'inspector-btn inspector-action-btn';
btn.className = 'or-btn or-btn-icon inspector-action-btn';
btn.title = name;
if (name === 'Delete') {
btn.innerHTML = DELETE_SVG;
btn.classList.add('inspector-btn-danger');
btn.classList.add('or-btn-danger-hover');
} else {
btn.textContent = name;
}
const btn = document.createElement('button');
btn.className = 'or-btn inspector-action-btn';
btn.title = name;
if (name === 'Delete') {
btn.classList.add('or-btn-icon', 'or-btn-danger-hover');
btn.innerHTML = DELETE_SVG;
} else {
btn.classList.add('or-btn-sm');
btn.textContent = name;
}

Comment thread src/web/src/main.js Outdated
coordBar.textContent = `X: ${app.formatDbu(dbuX)} Y: ${app.formatDbu(dbuY)}`;
syncHud();
});
app.map.on('mouseout', () => { app.lastMouseLatLng = null; });

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

When the mouse leaves the layout viewer, app.lastMouseLatLng is set to null, but the coordBar text content is not cleared, and syncHud() is not called. This means the HUD will permanently display the last hovered coordinates even when the pointer is outside the canvas. Clearing the text content on mouseout and syncing the HUD ensures it hides correctly when inactive.

Suggested change
app.map.on('mouseout', () => { app.lastMouseLatLng = null; });
app.map.on('mouseout', () => {
app.lastMouseLatLng = null;
coordBar.textContent = '';
syncHud();
});

Comment thread src/web/src/main.js
Comment on lines +659 to +662
const progress = document.createElement('div');
progress.className = 'or-progress';
mapDiv.appendChild(progress);
app.tileProgressEl = progress;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Storing references to DOM elements and the Leaflet map instance on the global app object without cleaning them up when the layout panel is closed/destroyed will cause memory leaks. Since Golden Layout panels can be closed and reopened dynamically, we should listen to the container's destroy event to clean up these references and properly destroy the Leaflet map instance.

Suggested change
const progress = document.createElement('div');
progress.className = 'or-progress';
mapDiv.appendChild(progress);
app.tileProgressEl = progress;
const progress = document.createElement('div');
progress.className = 'or-progress';
mapDiv.appendChild(progress);
app.tileProgressEl = progress;
container.on('destroy', () => {
app.tileProgressEl = null;
app.heatMapLegendEl = null;
if (app.map) {
app.map.remove();
app.map = null;
}
});

Comment thread src/web/src/ui-utils.js
Comment on lines +564 to +574
for (const tab of root.querySelectorAll('.lm_tab:not([data-or-icon])')) {
const titleEl = tab.querySelector('.lm_title');
if (!titleEl) continue;
// Mark before the lookup, so a title with no icon is not re-examined
// on every subsequent layout change.
tab.setAttribute('data-or-icon', '');
const icon = panelTabIcon(titleEl.textContent.trim());
if (icon) {
tab.insertBefore(icon, titleEl);
added++;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

If decorateTabIcons is called before a tab's title is fully initialized or populated (resulting in an empty string), marking the tab with data-or-icon immediately will permanently prevent it from receiving an icon in subsequent layout updates. We should skip empty titles and only mark the tab as processed once a non-empty title is available.

    for (const tab of root.querySelectorAll('.lm_tab:not([data-or-icon])')) {
        const titleEl = tab.querySelector('.lm_title');
        if (!titleEl) continue;
        const title = titleEl.textContent.trim();
        if (!title) continue;
        // Mark before the lookup, so a title with no icon is not re-examined
        // on every subsequent layout change.
        tab.setAttribute('data-or-icon', '');
        const icon = panelTabIcon(title);
        if (icon) {
            tab.insertBefore(icon, titleEl);
            added++;
        }
    }

@maliberty

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. 👍

Reviewed commit: 30b6c89f5e

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@maliberty

Copy link
Copy Markdown
Member Author

Thanks — three of the four were real, and one of them was a visible defect. Addressed in 74b26c45.

Descriptor action buttons — fixed, and worse than the comment suggested. request_handler.cpp passes through any action it does not explicitly suppress (kSuppressedActions), and the comment there says as much: "Any new descriptor action not listed here appears automatically." So Change color, Report Path and Highlight children all reach the inspector as text. Forced into a 26px square they did not merely clip, they overlapped into an unreadable smear. Delete keeps .or-btn-icon; the rest take .or-btn-sm and size to their label.

Worth noting the clipping predates this branch — .inspector-btn was a fixed 26×26 too — but this is the commit that gave the buttons variants to choose from, so it is the commit that should choose correctly.

Coordinate readout — fixed, with a guard. Taken, but not quite as suggested. Leaflet raises mouseout for a move onto anything inside the container, including the zoom controls and the HUD the readout itself lives in. Clearing unconditionally would blank the readout whenever the pointer crossed onto a control sitting next to it, which is worse than the staleness. It now clears only when the pointer actually leaves the viewer:

const to = e.originalEvent && e.originalEvent.relatedTarget;
if (to && mapDiv.contains(to)) return;

Verified against a DOM that it holds for internal moves (control, HUD) and clears for a real exit and for a mouseout with no relatedTarget — the pointer leaving the window.

Tab icons — fixed. Good catch. An unpopulated title would have spent the tab's one chance at an icon on a title that could not be matched. Empty titles are skipped so the next layout change can decorate them.

Panel teardown — taken in part. The element references are released on destroy. I checked in Chrome that container.on('destroy') is the event GoldenLayout 2.6 actually fires on close, since hide/show fire on tab switches and would have been the wrong hook.

I have not taken the map.remove() / app.map = null half. About 140 call sites across eleven modules reach app.map, and many are unguarded — app.map.zoomIn() in the View menu (also bound to Z), app.map.fitBounds(...) in the inspector's zoom-to, app.map.getZoom() in the rulers, app.map.getContainer() in the display controls. Nulling it converts a detached-but-harmless map into a dozen ways to throw while the Layout panel is closed, which is a regression traded for a leak that only occurs if someone closes that panel and never reopens it. Guarding those call sites is a lifecycle change and wants its own PR rather than riding along in a styling one. The comment in createLayoutViewer records that reasoning so the next reader does not have to rediscover it.

@maliberty
maliberty marked this pull request as ready for review August 27, 2026 15:12
@maliberty
maliberty requested a review from a team as a code owner August 27, 2026 15:12
@maliberty
maliberty requested a review from gadfort August 27, 2026 15:12

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 74b26c4557

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/web/src/main.js Outdated
The palette was a set of per-rule literals: four surface values inside 8%
lightness, so panels, headers and inputs were indistinguishable; a desaturated
navy accent, so every selected state read as inert; and five corner radii
across 46 declarations, chosen per rule rather than from a scale.

Restructure it into two layers.  Each theme block defines the full palette as
scales -- --surface-*, --line-*, --ink-*, --accent-*, --sem-* -- and the role
names the component rules already use (--bg-*, --fg-*, --border*, --accent,
--canvas-*, ...) become aliases for a scale step.  No role name is added or
dropped, so the component rules are untouched and a palette change is now a
scale edit.

Colors:

- Three deliberate surface elevation steps plus a sunken pair for inputs,
  cool-biased so the neutrals sit with the accent.
- A saturated accent.  --bg-selected and --accent stay a solid step dark
  enough to carry --fg-white at 4.5:1 in both themes; the row highlights that
  carry --fg-primary instead (--bg-selected-row) are the tints.
- Light is picked against its own ground rather than derived from dark, which
  --bg-hover and --border sharing #d0d0d0 was a symptom of.
- --canvas-bg now matches the panel, so a canvas widget no longer shows a seam
  against the panel it is docked in.
- --ink-3 is set so --fg-muted clears 4.5:1 on every surface it appears over.

The destructive-action reds, the debug-pause yellow and one --fg-white were
literals in component rules; they move to --sem-danger*/--sem-warn*/the token.
--bg-map stays a literal: it is the Qt-parity black and an inline override
replaces that declaration.

Radii collapse to --radius-xs/sm/md, picked by what the element is.

Also:

- color-scheme per theme, so native scrollbars and <select> popups follow the
  theme, and accent-color, so checkboxes follow the accent.
- .fb-entry.fb-selected inherited --fg-primary on --bg-selected, which never
  had the contrast for it; it takes --fg-white like every other --bg-selected
  rule, and .fb-size comes off the same color so it does not drop out.
- --font-sans replaces three verbatim copies of the same stack.

The spacing scale lands with the control consolidation that consumes it.

getThemeColors() in theme.js reads --canvas-*, --fg-primary, --fg-muted,
--bg-panel and --bg-map off the computed style and hands them to Canvas 2D.
var() in a custom property is substituted at computed-value time, so the
aliases resolve to colors there; verified in Chrome that each one is a value
Canvas 2D accepts.

Signed-off-by: Matt Liberty <mliberty@precisioninno.com>
Three sets of near-duplicate rules become one rule each, and the interaction
states the stylesheet never had get added on top of them.

Buttons.  Eight blocks -- .toolbar-button, .timing-btn, .drc-btn,
.inspector-btn, .sb-tool, .heatmap-rebuild, .or-modal-btn and a bare
`.modal-dialog button` -- differed by a pixel of padding or radius and nothing
else.  They become .or-btn with variants for size (.or-btn-sm, .or-btn-icon)
and emphasis (.or-btn-primary, .or-btn-danger), so the next control picks a
variant instead of adding a ninth block.  Eleven buttons had no class at all
and rendered as browser defaults -- the nine in the schematic toolbar and the
two in the 3D viewer's overlay -- and they join the rest.

.timing-tab, .sb-tab, .sb-row-btn, .inspector-edit-btn, .modal-close,
.highlight-swatch, .bg-color-reset and .debug-continue-btn stay out of it:
they are <button> elements shaped like something other than a button.

The inspector's Delete icon was danger-styled on hover only, so
.or-btn-danger-hover keeps that -- one permanently red icon in a toolbar that
is on screen all session reads as an alert rather than an action, which is not
what the always-red confirm-dialog button is for.

Tooltips.  Four byte-identical blocks become .or-tooltip, with
.or-tooltip-fixed for the one positioned in viewport coordinates.  All four
carry `white-space: pre` now: the timing column help arrives as a multi-line
string and needs its newlines, and single-line content renders the same either
way.

Canvas overlays.  The scale bar, coordinate readout, heat-map legend and
pending-request indicator were four separately styled chips; they share
.or-hud, which is the treatment the heat-map legend already had -- translucent,
hairline border, blurred enough to stay legible over dense routing.  The scale
bar and the coordinate readout also merge into one HUD rather than sitting as
two stacked chips.  The scale bar is a display option and still hides on its
own, so the divider goes with it, and the HUD hides entirely when neither
readout has anything to show.

These overlays sit on --bg-map, which is black in both themes for Qt parity,
so their surface and ink cannot follow the app theme -- a light-theme HUD was
a bright block on black with unreadable ink on it.  --bg-hud, --border-hud,
--fg-hud and --fg-hud-muted move to the theme-independent block for that
reason, and the ruler labels and label handles, which are also drawn on the
canvas, come off them too.  The toast is not on the canvas, so it stays on the
theme.  .heatmap-legend-row is used on both grounds and is scoped
accordingly.

Leaflet draws the zoom and Fit controls as its own white chrome, which matched
the app only by coincidence; .leaflet-bar takes the HUD surface.

Interaction states:

- A focus ring.  The stylesheet had no :focus-visible rule at all, so keyboard
  navigation was invisible.  Inputs that already mark focus with their border
  opt out rather than showing both.
- A 120ms transition on the buttons and the dense list rows.  There was one
  transition in the file before this.
- A prefers-reduced-motion block.  Every animation here is decoration over a
  state shown some other way, except the loading spinner, which keeps turning
  slowly.
- .gc-invalid becomes a ring rather than an outline, so the new focus ring
  cannot replace it and leave a field looking valid while it is being fixed.

--space-sm/md/lg/xl are added and consumed by the rules above.

test-status-indicator.js reimplements updateStatus rather than importing it,
so its copy and its colour assertions are updated alongside main.js.

Signed-off-by: Matt Liberty <mliberty@precisioninno.com>
Type.  Nineteen rules set bare `monospace`, which resolves to Courier on
several systems -- wide, and thin at the 11-12px this UI uses.  They move to
--font-mono, which names the faces that ship with each platform and keeps
`monospace` as the floor.

More to the point, six whole panels were set in mono -- the inspector, timing,
hierarchy, clock tree and charts widgets, and the heat-map controls -- so their
toolbars, headers and labels were mono too and the panels read as log output
rather than as tools.  Mono is now for values: table bodies, property values,
the console, the HUD readouts, the ruler labels.  Everything that is a label
takes --font-sans.

The canvas widgets drew with a literal '11px monospace' at fourteen sites.
getThemeColors() hands out --font-mono and --font-sans alongside the colours it
already resolves, so the canvas draws in the same faces as the DOM, with the
same split: mono for tick labels and readouts, sans for titles and empty-state
text.

Tables.  .timing-table (which the hierarchy browser also uses) and .sb-table
put a border under every cell, which made a dense table read as a grid of
boxes.  The row borders are gone; the columns carry the eye instead.  Headers
are sans, so they no longer look like another data row, and the sticky header
gets a gradient under it -- it had nothing separating it from the rows sliding
beneath.

Numeric columns are right-aligned with tabular figures, so a column of values
lines up on the decimal point and does not jitter as digits change.  Which
columns those are is now part of the column definition (`num` in PATH_COLS,
DETAIL_NUM_COLS for the detail table) rather than inferred at render time.

A selected row was a fill that was hard to tell from the hover fill; it also
carries a bar on its leading edge now.

Slack bars.  Each slack cell gets a bar growing from the cell's midpoint --
left for a violation, right for a met path -- scaled by the largest |slack| in
the table, so every zero crossing lines up and the column can be scanned for
where the trouble is.  It is a pseudo-element, so a narrow column cannot
overflow and the cell needs no extra markup.  --slack-neg and --slack-pos are
the Qt GUI's histogram colours (chartsWidget.cpp:1052-1062), which
charts-widget.js also carries, so a slack bar in the table reads as the same
measure as a bar in the charts panel.

Not done, contrary to what I proposed earlier: the clock-tree and charts
palettes stay as they are.  Both mirror the Qt GUI exactly --
clockWidget.h:168-174 uses Qt::red for the root and the leaf register and
Qt::darkCyan for the inverter and the leaf macro, so the duplicate hex values
in clock-tree-widget.js are parity, not a collision to fix, and changing them
would put the two GUIs out of step.

Signed-off-by: Matt Liberty <mliberty@precisioninno.com>
The three items the design proposal's mockup showed but the previous commits
did not implement.

Canvas frame.  The layout canvas is black in both themes and fills its panel
edge to edge, so it read as a hole punched in the UI rather than as the
design.  It gets a hairline, drawn as an overlay rather than a border for two
reasons: a border on .layout-viewer would change the size Leaflet measures for
the map, and an inset box-shadow would be painted under the tile panes and
never seen.  z-index 800 puts it over Leaflet's panes (which reach 700) and
under the HUD and Leaflet's own controls, so it frames the canvas without
drawing a line across either.

Panel tabs.  Golden Layout's tabs were distinguished only by a slightly
different background, which at these surface steps is close to invisible.  The
active tab now carries the accent underline the timing widget already uses on
its own tab bar, so both tab bars in a stacked panel read the same way, and
each tab carries the icon for its panel -- a stack holds six panels whose
titles all read alike at tab width.

Three things this needed:

- The header height is a layout-config value (dimensions.headerHeight), not
  CSS: Golden Layout measures it in JS.  A saved layout carries the dimensions
  it was saved with, so the restored config is overridden rather than bumping
  LAYOUT_VERSION and throwing the user's layout away.
- The tab height has to be stated, not stretched.  .lm_tabs is absolutely
  positioned and shrink-wraps its content, so `align-self: stretch` has nothing
  to stretch against, and the header forces `box-sizing: content-box
  !important` on everything inside it, so the 26px height plus the 2px
  underline is what makes 28.
- The active tab's underline is set as a whole border, not just a colour:
  Golden Layout's theme sets `border-bottom: none` on it at the same
  specificity, so a colour alone applied to a zero-width border.

The icons need one more thing.  They widen every tab, and Golden Layout decides
which tabs fit in a header before they exist, so without a re-measure the tabs
that no longer fit overlap the stack's controls instead of moving into the
overflow dropdown.  decorateTabIcons returns how many it added and the caller
re-measures only then, which is also what keeps that call from looping through
the layout event that triggered it.

Status chrome.  The disconnect banner pulsed forever; three beats say look at
me and the rest was motion in the corner of the eye for as long as the server
stayed down.  Tile requests in flight become a 2px indeterminate line along the
top of the canvas rather than a grey chip in a corner nobody watches.

The count is not dropped outright, as the proposal had it: a long queue is the
explanation for a viewer that feels stuck, so past PENDING_BACKLOG the number
is still shown, and being a backlog by then it is coloured as one in the
stylesheet instead of by an inline style from updateStatus.

test-status-indicator.js reimplements updateStatus rather than importing it, so
its copy and the assertions that assumed any pending count shows a chip are
updated alongside main.js.  Verified against a real Golden Layout in headless
Chrome, both themes.

Signed-off-by: Matt Liberty <mliberty@precisioninno.com>
Four points from the PR review, three taken as-is and one in part.

Descriptor action buttons.  Only Delete renders as a glyph; every other action
renders its name as text, and request_handler.cpp passes through any action it
does not suppress ("Change color", "Report Path", "Highlight children", ...).
The square icon variant was applied to all of them, so those labels overflowed
a 26px box and overlapped each other.  Delete keeps .or-btn-icon; the rest take
.or-btn-sm and size to their label.  The clipping predates this branch --
.inspector-btn was a fixed 26px square too -- but this is the commit that gave
the buttons variants to choose from, so it is the commit that should choose
correctly.

Coordinate readout.  mouseout cleared app.lastMouseLatLng but left the readout
showing wherever the pointer had last been, so the HUD advertised a live
position it no longer had.  It clears now -- but only when the pointer actually
leaves the viewer: Leaflet raises mouseout for a move onto anything inside the
container, including the zoom controls and the HUD itself, and blanking the
readout when the pointer crosses onto a control it is sitting next to would be
worse than the staleness.

Panel teardown.  The panel's own element references are released on the
container's destroy event.  Both readers already null-check, and
createLayoutViewer sets them again if the panel is reopened.

The review also asked for map.remove() and app.map = null alongside them.  Not
done: about 140 call sites across eleven modules reach app.map and many are
unguarded -- the View menu's zoom items, the inspector's zoom-to, the rulers,
the display controls -- so nulling it converts a detached-but-harmless map into
a dozen ways to throw while the panel is closed.  The reference cleanup that is
safe is here; guarding the rest is a lifecycle change and wants its own PR.

Tab icons.  A tab whose title is not populated yet was marked processed, which
spent its one chance at an icon on a title that could not be matched.  Empty
titles are now skipped so the next layout change can decorate them.

Verified in Chrome that container.on('destroy') is the event GoldenLayout 2.6
actually fires when a panel is closed ('hide' and 'show' fire on tab switches,
so neither is the right hook), and that the mouseout guard holds the readout for
internal moves and clears it for real exits and for a mouseout with no
relatedTarget.

Signed-off-by: Matt Liberty <mliberty@precisioninno.com>
The mouseout handler holds the coordinate readout when the pointer only
moves onto something inside the container -- a zoom control, a marker,
the HUD -- but it cleared app.lastMouseLatLng before that check, so
Z/Shift+Z zoomed around the map centre while the readout still showed a
pointer position.  Clear the anchor alongside the readout, on a real
exit only.

Signed-off-by: Matt Liberty <mliberty@precisioninno.com>
@gadfort

gadfort commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

@maliberty LGTM, just need to get it to pass the two failures.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants