Open the editor modal from the attachment meta box button - #75
Conversation
The scripts under assets/js/ had no unit tests at all: the only JS the CI pipeline exercised was through Playwright, which needs a full WordPress environment and cannot say why a handler misbehaved. Vitest with happy-dom covers the gap. The admin scripts are plain jQuery IIFEs loaded by wp_enqueue_script, so the harness gives them a window, a document and a global jQuery, then drives them through the DOM the way WordPress does -- no build step and no changes to the scripts themselves. CI runs `npm run test:js` next to PHPCS, before wp-env starts: neither needs a WordPress runtime, so both fail fast and cheap.
The button the attachment meta box prints is a real <a href> to the standalone editor page carrying data-attachment-id. Nothing intercepts it, so on a screen that already has the editor modal the click navigates away from the modal the plugin just rendered. Two cases, because the fix is only correct if it keeps the fallback: with the modal on the page the click must open it in place, and without one the href must still be followed. The first fails against the current handler set -- that is the bug -- and the second passes today and must keep doing so.
The "Edit in eXeLearning" action in the attachment meta box and media modal navigated to a full page instead of opening the existing in-page editor modal. Bind a delegated handler so any .exelearning-edit-page-button[data-attachment-id] opens the modal (Save / Close) in place.
Adding Vitest brought vite in, which declares engines ^20.19.0 || >=22.12.0. Nothing in the workflow pinned Node, so the suite ran on whatever ubuntu-latest happened to ship that week -- fine today, an unexplained install failure the day the image moves. actions/setup-node@v4 pins it at 22 and takes over npm caching, so the hand-rolled actions/cache step on ~/.npm goes away: `cache: npm` keys on package-lock.json and restores the same directory.
Test in WordPress PlaygroundTest the plugin with the code from this branch:
|
…button # Conflicts: # package-lock.json # package.json # vitest.config.mts
Each test imports a fresh copy of exelearning-editor.js, and every copy registers a native message listener on window that afterEach() never removed — it only cleaned the jQuery handlers on document. Tests that dispatch message events could therefore reach stale editor instances from previous tests and become order-dependent. Track message-listener registrations on window and unregister them in the shared cleanup, with a regression test that dispatches a message after cleanup and asserts no stale instance handles it.
|
Review notes The fix looks good: the meta box button now opens the in-page editor modal only when both the modal and the iframe are present, and keeps the plain One test-isolation issue surfaced during review and is addressed in b31116b: each test imports a fresh copy of Optional follow-up (non-blocking): the click handler also intercepts Ctrl/Cmd/Shift-clicks on the link, so opening the standalone editor in a new tab is no longer possible on screens that have the modal. Ignoring modified clicks before calling |
1 participant
Problem
The attachment meta box prints "Edit in eXeLearning" as a real
<a href>to thestandalone editor page, carrying
data-attachment-id(
includes/integrations/class-media-library.php). Nothing intercepted that click,so on any screen that already has the editor modal the button navigated away from
the modal the plugin had just rendered — the in-page Save / Close flow was
reachable from everywhere except the button that most obviously means "edit this".
What this changes
bindEvents()now binds a delegated handler for.exelearning-edit-page-button[data-attachment-id]that opens the modal in place.It is deliberately conditional: when the screen has no modal and no iframe the
handler returns without calling
preventDefault(), so thehrefis followed andscreens without a modal keep working exactly as before.
Tests
The Vitest + happy-dom harness already ships on
main(#76, together withtests/js/exelearning_embed_loader.test.jsand thenpm run test:jsCI step), sothis PR only adds a second suite plus what it needs: the admin scripts are jQuery
IIFEs that close over the
jQueryglobal, so the realjquerypackage joins thedev dependencies and each test publishes it as the global before importing the
script — no stubbing, and nothing about the scripts had to change to make them
testable.
tests/js/exelearning_editor.test.js(3 tests):prevented — fails against the handler set before this fix;
open()is never called and thehrefis still followed —guards the fallback;
messageevent reaches nostale editor instance — each test imports a fresh copy of the script and every
copy registers a native
messagelistener onwindow, so the cleanup tracksand unregisters them to keep the tests order-independent.
CI
Node is now pinned with
actions/setup-node@v4at 22 rather than inherited fromthe runner image: vite (pulled in by Vitest) declares engines
^20.19.0 || >=22.12.0, and whateverubuntu-latesthappens to ship is not aguarantee. That action also takes over npm caching, so the hand-rolled
actions/cachestep on~/.npmgoes away —cache: npmkeys on the samepackage-lock.jsonand restores the same directory.Verification
npm run test:js— 12 passed across both suites (3 new, 9 existing); themodal test failed before the fix commit
npm ci && npm run test:js— the exact sequence CI runsvendor/bin/phpcs --standard=.phpcs.xml.dist -q .— exit 0