Skip to content

WebGL viewer: fix help-menu shortcut display & position (+ propose in-browser ROI drawing)#642

Open
jackgallant wants to merge 1 commit into
mainfrom
claude/webgl-help-menu-fixes
Open

WebGL viewer: fix help-menu shortcut display & position (+ propose in-browser ROI drawing)#642
jackgallant wants to merge 1 commit into
mainfrom
claude/webgl-help-menu-fixes

Conversation

@jackgallant
Copy link
Copy Markdown

Summary

This PR fixes several long-standing display bugs in the WebGL viewer's h help menu. Separately — in this description only, no code in this PR — it also proposes upstreaming an in-browser ROI-drawing feature we've prototyped on top of the viewer.

Bug fixes (ready to merge)

Small, self-contained diff in mriview.js + mriview.css:

  1. Shortcut key casing. The help generator rendered every key with key.toUpperCase(). The viewer binds both plain lowercase keys (r, s, l, h, …) and genuinely Shift-modified ones (key:'R'/'S'/'L' with modKeys:['shiftKey'] — these are different commands, e.g. toggle right hemisphere). Uppercasing made the two indistinguishable in the menu (both shown as R/S/L), and the help toggle h was shown as H (implying Shift+H). The binding data already carries the correct case, so we now render the key verbatim.
  2. Modifier-label casing. shiftKey now renders as Shift instead of shift.
  3. Panel position. #helpmenu was pinned to the left edge (left:0%), so its lower half slid behind the lower-left legend. It's now centered (left/top:50% + translate(-50%,-50%)).
  4. Panel font. #helpmenu set no font-family, so it fell back to the browser-default serif (Times in Firefox) while the rest of the UI is sans-serif. Now explicitly sans-serif.

(Note on Firefox scroll-zoom: it's already correct on main — the active camera controller in movement.js uses the standard wheel/deltaY. Only the legacy LandscapeControls.js, used by simple.html, still reads the WebKit-only wheelDelta, so it's intentionally left out of this PR.)

Proposal: in-browser ROI drawing (discussion — no code here)

We've built a working prototype that adds ROI drawing + editing + export directly in the WebGL viewer:

  • A Display / Draw toggle; entering Draw flattens the surface.
  • Lasso a region on the flatmap → it's fitted to a smooth, editable bezier and baked into the surface SVG overlay, so it occludes and morphs with the surface just like the built-in rois/sulci layers.
  • Re-edit later with full vector controls: drag anchors and tangent handles, double-click to add a point, toggle corner/smooth, delete points.
  • Export / Import a portable JSON (per-hemisphere vertex indices + ordered boundary ring + the flat-UV bezier with handles). It re-imports onto any viewer on the same surface.

It's structured as a self-contained drop-in bundle — pure core/ geometry (unit-tested), a thin ViewerAdapter that quarantines every pycortex-specific quirk (flat-offset, pivot matrices, SVG overlay, data-ptidx convention), and host-agnostic ui/. It attaches to a static or dynamic pycortex viewer with two <script> tags and no per-viewer edits.

Live demo (drawing baked into the public group-stories viewer):

👉 https://gallantlab.org/viewer-stories-group-roidraw/

(Display/Draw toggle at the top → Draw → drag to lasso a region → name it → it's drawn on the surface; Export/Import live in the panel. Tested in Firefox.)

Would the maintainers be interested in incorporating this upstream so pycortex ships ROI drawing out of the box? If so I'm glad to open a follow-up PR with the implementation and wire it into the viewer build. A few design points we'd want to align on first:

  • where the bundle/source should live under cortex/webgl/resources/
  • how to surface the Draw toggle in the existing menu/UI
  • the export JSON schema (currently vertex indices + boundary ring + bezier, keyed to the surface)

🤖 Generated with Claude Code

The 'h' help menu had several long-standing display bugs:

- Shortcut keys were rendered with key.toUpperCase(), so a plain
  lowercase binding (e.g. key:'h', key:'r', key:'s', key:'l') was shown
  as an uppercase letter -- indistinguishable from the genuinely
  Shift-modified bindings (key:'R'/'S'/'L' with modKeys:['shiftKey'],
  which are different commands). The binding data already carries the
  correct case, so render the key verbatim instead of uppercasing it.
- The modifier label was lowercased ("shift"); capitalize it ("Shift").
- #helpmenu was pinned to the left edge (left:0%), so its lower half
  slid behind the lower-left legend. Center it horizontally and
  vertically (left/top:50% + translate(-50%,-50%)).
- #helpmenu set no font-family, so the panel fell back to the
  browser-default serif (Times in Firefox) while the rest of the UI is
  sans-serif. Give it an explicit sans-serif.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

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

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 centers a UI element and updates the formatting of modifier keys in the MRI viewer. The review feedback suggests using a consistent font family (Helvetica Neue, sans-serif) in the CSS. In the JavaScript code, the reviewer recommends replacing fragile string manipulation for modifier keys with a mapping object, and warns of a potential TypeError if modKeys is undefined during wheel actions.

Comment on lines 1253 to 1256
var modKeys = list[i][name]['modKeys']
modKeys = modKeys.map((modKey) => modKey.substring(0, modKey.length - 3))
modKeys = modKeys.map((modKey) => modKey.charAt(0).toUpperCase() + modKey.substring(1, modKey.length - 3))
modKeys = modKeys.join(' + ')
new_html += modKeys + ' + wheel </td><td>' + diplay_name + '</td></tr>'
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.

high

If modKeys is undefined (e.g., if a wheel action does not require modifier keys), calling .map on it will throw a TypeError. We should guard against this by defaulting to an empty array, and also use a mapping object for cleaner and safer translation.

                            var modKeys = list[i][name]['modKeys'] || [];
                            const modMap = { shiftKey: 'Shift', ctrlKey: 'Ctrl', altKey: 'Alt', metaKey: 'Meta' };
                            modKeys = modKeys.map((modKey) => modMap[modKey] || modKey);
                            var modStr = modKeys.join(' + ');
                            new_html += (modStr ? modStr + ' + ' : '') + 'wheel  </td><td>' + diplay_name + '</td></tr>'

max-width:400px;
display:none;
font-size:12pt;
font-family:Helvetica, Arial, sans-serif;
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

For design consistency across the application, please use Helvetica Neue, sans-serif to match the font family defined for the body element on line 3.

    font-family:Helvetica Neue, sans-serif;

Comment on lines 1244 to 1246
let modKeys = list[i][name]['modKeys'];
modKeys = modKeys.map((modKey) => modKey.substring(0, modKey.length - 3));
modKeys = modKeys.map((modKey) => modKey.charAt(0).toUpperCase() + modKey.substring(1, modKey.length - 3));
modKeys = modKeys.join(' + ');
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

Using string manipulation (substring and charAt) to format modifier keys is fragile and hard to maintain. If a modifier key name does not end with 'Key' or is formatted differently, this logic will break. Using a mapping object is much cleaner, safer, and more robust.

Suggested change
let modKeys = list[i][name]['modKeys'];
modKeys = modKeys.map((modKey) => modKey.substring(0, modKey.length - 3));
modKeys = modKeys.map((modKey) => modKey.charAt(0).toUpperCase() + modKey.substring(1, modKey.length - 3));
modKeys = modKeys.join(' + ');
let modKeys = list[i][name]['modKeys'];
const modMap = { shiftKey: 'Shift', ctrlKey: 'Ctrl', altKey: 'Alt', metaKey: 'Meta' };
modKeys = modKeys.map((modKey) => modMap[modKey] || modKey);
modKeys = modKeys.join(' + ');

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant