Skip to content

Keep menu edit links inside the current window - #444

Merged
AllTerrainDeveloper merged 3 commits into
trunkfrom
fix/in-window-nav-after-submenu-tab
Jul 29, 2026
Merged

Keep menu edit links inside the current window#444
AllTerrainDeveloper merged 3 commits into
trunkfrom
fix/in-window-nav-after-submenu-tab

Conversation

@AllTerrainDeveloper

@AllTerrainDeveloper AllTerrainDeveloper commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator

Fixes #442.

The bug

Editing a nav menu spawned a new window on every click. Picking a menu from the Menus screen's tab row, or hitting Edit Menus / Manage Locations, opened yet another window instead of updating the one already showing the Menus screen.

Why

The chromeless bridge preventDefaults every admin-internal link click inside an iframe and lets the parent shell route it (handleCrossPageAdminLink in src/window/iframe-bridge.ts). Same page → location.assign() in place; different page → windowManager.open().

"Same page" was decided by comparing the target slug against the window's baseId — the page the window was opened on, which never moves.

That's stale the moment the submenu tab strip re-points the iframe. Appearance → Menus navigates the existing window's iframe to nav-menus.php while the window keeps baseId: themes-php. So every link on the Menus screen — including WP's own per-menu tab row (nav-menus.php?action=edit&menu=N) — derived to nav-menus-php, didn't match themes-php, and was classified cross-page.

Not specific to Menus: the same stale comparison hits any screen reached through the tab strip (Appearance → Widgets, Tools → Import, Settings → Writing, …). Menus is where it's most visible because its UI is a row of links back to itself.

The fix

A window now counts as being "on" two slugs: its baseId and the slug its iframe currently displays (getCurrentUrl()). A click matching either one navigates in place.

Matching both rather than swapping baseId out for the live slug is deliberate — it only ever widens the same-page set, so it can convert a would-be new-window open into an in-place navigation but never the reverse. A window that navigated from admin.php?page=foo to admin.php?page=foo&path=/bar still treats a link back to the landing page as in-page.

Tests

Three cases added to src/window/iframe-bridge.test.ts, covering the fix and both of its guardrails:

  • a click matching the live iframe slug navigates in place (the Menus case);
  • a click matching baseId still navigates in place after the iframe has moved (no narrowing);
  • a click matching neither still opens a fresh window.

npm run lint, npm run typecheck, npm run test:js (2459 tests), and npm run build all green.

Docs: docs/bridge-protocol.md — the "Admin link routing inside chromeless iframes" dispatch list now states the two-slug rule and why the live slug can only widen the match.

How to verify

  1. Have at least two nav menus on the site (Appearance → Menus).
  2. Open Appearance from the dock, then click the Menus tab in the window's tab strip.
  3. Click through the per-menu tabs, Edit Menus, and Manage Locations.

Before: a new window per click. After: the same window's iframe updates.

🤖 Generated with Claude Code

Open WordPress Playground Preview

AllTerrainDeveloper and others added 2 commits July 29, 2026 10:19
Clicking a link inside a chromeless iframe is routed by the parent
shell: same page navigates the iframe in place, a different page opens
a fresh window. "Same page" was decided by comparing the target slug
against the window's `baseId` alone — the page the window was OPENED
on, which never moves.

That breaks as soon as the submenu tab strip re-points the iframe. The
Appearance window opened on `themes.php` keeps `baseId: themes-php`
while displaying `nav-menus.php`, so every link on the Menus screen —
including its own per-menu tab row (`nav-menus.php?action=edit&menu=N`)
— read as cross-page and spawned another window per click.

A window now counts as being "on" two slugs: its `baseId` and the slug
its iframe currently shows (`getCurrentUrl()`). Matching either one
only ever widens the same-page set, so it can turn a would-be new
window into an in-place navigation but never the reverse — a window
that navigated away still treats a link back to its landing page as
in-page.

Fixes #442

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015sKW1mu1SGg5k9oSHgxv5s
A submenu tab points at one landing URL, but the screen behind it has
more states than that: nav-menus.php also renders as
`?action=locations` and `?action=edit&menu=2`, list tables paginate
into `?paged=2`, settings screens redirect back with
`?settings-updated=true`. Matching tabs by exact URL blanked the strip
on all of them — clicking "Manage Locations" inside the Menus screen
dropped the Menus tab's highlight.

The active tab is now resolved in two passes: an exact URL match wins
outright, and failing that the shell lights the tab whose page the URL
belongs to. Ownership needs both halves to hold — the page-identity
params agree (`post_type`, `taxonomy`, `page`, `path`, …, via the new
`pageIdentityKey` helper, which reuses `deriveWindowId`'s identity set)
AND every param the tab's own URL declares is present with the same
value. So Categories can't claim Tags, and `admin.php?page=x&tab=test`
can't claim `?tab=logs` — that falls back to the `?page=x` entry.
Ties go to the more specific entry.

Exactly one tab is active now; the old loop could light several if two
entries shared a URL.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015sKW1mu1SGg5k9oSHgxv5s
@AllTerrainDeveloper

Copy link
Copy Markdown
Collaborator Author

Follow-up pushed in 561d94d — the tab highlight.

nav-menus.php?action=locations and nav-menus.php?action=edit&menu=2 are the same page as the Menus tab, but syncActiveTab matched tabs by exact URL, so any of those blanked the strip. Same story for edit.php?paged=2 on All Posts, or any settings screen redirecting back with ?settings-updated=true.

The active tab is now resolved in two passes:

  1. Exact URL match wins outright — unchanged behavior.
  2. Otherwise the tab whose page the URL belongs to is lit. A tab owns a URL when both halves hold:
    • page-identity params agree (post_type, taxonomy, page, path, …), via a new pageIdentityKey() in src/utils.ts that reuses deriveWindowId's identity set;
    • every param the tab's own URL declares is present in the current URL with the same value.

The second condition is what keeps this from over-matching. Categories can't claim Tags. And a plugin registering both admin.php?page=x and admin.php?page=x&tab=test as submenu entries lights the deeper one on ?tab=test while ?tab=logs falls back to the ?page=x entry rather than lighting the wrong tab. Ties go to the more specific entry.

One incidental tightening: exactly one tab is active now. The old loop could light several if two entries resolved to the same URL.

New src/window/tabs.test.ts, 10 cases: exact match, chromeless flag, the three nav-menus sub-views, pagination, Categories-vs-Tags separation, specificity, the contradicting-param fallback, no-match blanking, external-tab clearing, and aria-selected.

npm run lint, npm run typecheck, npm run test:js (254 files / 2469 tests), and npm run build all green. docs/javascript-reference.md's config.submenu entry documents the matching rule.

Every Analytics submenu entry shares `page=wc-admin` and differs only
by `path`. `path` is an identity param, so they're distinct pages — a
sub-view of one must never light another's tab. Verified against the
real menu tree; pinning it so a future tweak to the identity set can't
quietly collapse them.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015sKW1mu1SGg5k9oSHgxv5s
@AllTerrainDeveloper

Copy link
Copy Markdown
Collaborator Author

Regression check against the real menu tree

I didn't want to answer "no regressions" from reasoning alone, so I dumped the live admin menu off the dev site (wp eval-file against wordpress-alcazaba-cli-1, read-only) and replayed the actual shipping syncActiveTab over it. 19 parent menus — core plus WooCommerce, Astra, WP File Manager.

Probe: every tab under every parent (including the synthetic back-to-parent tab, deduped the same way dom.ts does it), crossed with 11 realistic sub-view suffixes — paged=2, action=locations, action=edit&menu=2, settings-updated=true, s=…, orderby/order, message&_wpnonce, tab=logs, view=list, updated=1. Each result compared against the pre-change algorithm.

probes=935  regressions=0  newly-lit=850
identity collisions: none
  • 0 regressions — no probe that previously lit a tab now lits a different one, or lits several.
  • 850 newly lit — probes that used to blank the strip and now correctly highlight their own tab. That's the bug, and it was much wider than Menus.
  • 0 identity collisions — no two entries under any parent share a pageIdentityKey, so no parent even has two candidate tabs to confuse. The WooCommerce Analytics submenu is the closest call (10 entries all on page=wc-admin, differing only by path) and stays fully separated because path is an identity param. Pinned as a test in 57bf3d9.

The scratch harness isn't committed — it depends on a fixture dumped from one specific site. The shapes it exercised are covered by the 11 cases in src/window/tabs.test.ts.

On the routing change (commit 1), the guarantee is structural rather than empirical: the check went from target === baseId to target === baseId || target === liveSlug. The second form is true whenever the first is, so it can only convert a would-be new-window open into an in-place navigation — never the reverse. There is no input for which the old code navigated in place and the new code opens a window.

@AllTerrainDeveloper
AllTerrainDeveloper merged commit 77969a3 into trunk Jul 29, 2026
5 checks passed
@AllTerrainDeveloper
AllTerrainDeveloper deleted the fix/in-window-nav-after-submenu-tab branch July 29, 2026 08:47
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.

Menu Bug

1 participant