fix(e2e): restore 12 broken e2e tests + add CI e2e job - #81
Conversation
… fixtures Popover: position synchronously on open instead of deferring to rAF, and keep the popover visibility:hidden via CSS until positioned. This prevents the popover from appearing at viewport (0,0) for one frame, which broke light-dismiss under load. Added _isPopoverOpen() helper to safely handle environments that don't support :popover-open (jsdom). Table fixture: migrate from removed sibling <tr else> pattern to else="templateId" on the each element with <template> references, per Core v1.15 breaking change. DnD fixture: rename on:reorder/on:remove/on:receive to on:nojs:dnd-reorder/on:nojs:dnd-remove/on:nojs:dnd-receive per the post-1b109ce event rename.
The CSS-class approach (.nojs-popover { visibility: hidden } +
.nojs-popover--positioned) broke dropdowns: the dropdown directive sets
popover="auto" on its menu, which causes the NoJS framework to also run
the popover directive and add the nojs-popover class -- making the menu
permanently invisible since the dropdown open path never adds the
--positioned class.
Fix: remove the CSS visibility rule entirely and use inline
style.visibility on the specific popover element being opened. Set
visibility:hidden before togglePopover()/showPopover(), position
synchronously, then clear the inline style. This scopes the guard to
only elements explicitly opened through the popover code path, with
zero impact on dropdown or other components that share the popover
attribute.
Proof: dropdown.spec.ts passes 24/24 on main, fails 15 with the CSS
approach, passes 24/24 with this inline approach.
- dnd.spec.ts: replace deprecated aria-grabbed assertions with current a11y contract (aria-roledescription, nojs-dragging class, live-region announcements for grab and cancel) - popover.spec.ts: update aria-haspopup expectation from "true" to "dialog" to match popover.js implementation - ci.yml: add e2e job running Playwright chromium against built Core + Elements, closing the 7-week CI gap that let these tests rot
ErickXavier
left a comment
ErickXavier
left a comment
There was a problem hiding this comment.
QA Review — PR #81
Popover Fix Correctness
Verified all three open paths for the visibility hidden/position/clear sequence:
1. Click handler (popover-trigger, line 220-239): Sets style.visibility = "hidden", calls togglePopover(), then either positions+clears (if open) or just clears (if the toggle closed it). Synchronous positioning, no rAF in the open path.
2. popoverApi.open (line 287-301): Sets style.visibility = "hidden", wraps showPopover() in try/catch — on exception clears visibility and returns false. On success, positions synchronously + clears. All branches clear.
3. popoverApi.toggle (line 311-326): Same pattern as click handler — sets hidden, calls togglePopover(), branches on _isPopoverOpen, clears in both branches.
Safety net: The toggle event handler (line 163) clears el.style.visibility = "" on close, providing a secondary cleanup path.
should — inconsistent exception handling: popoverApi.open wraps showPopover() in try/catch (line 291) but the click handler and popoverApi.toggle call togglePopover() without try/catch. If togglePopover() throws after _supportsPopover() passes (theoretically possible per spec for InvalidStateError), style.visibility = "hidden" would stick and the popover would be permanently invisible. Low probability since _supportsPopover() already validates the method exists, but the inconsistency is easy to fix — wrap the two bare togglePopover() calls the same way showPopover() is wrapped in popoverApi.open. No urgency, but worth a follow-up.
Dropdown interaction: Confirmed safe. The dropdown directive (dropdown.js:164) sets popover="auto" on its menu and calls showPopover()/hidePopover() directly — it never goes through the popover-trigger click handler or the programmatic API. The inline style.visibility is only set in those explicit popover code paths, so dropdown menus are never affected. The commit message documents the CSS-class approach failure (15 of 24 dropdown tests broke) and the inline approach passing 24/24.
Test Assertion Accuracy
Live-region regexes vs source strings:
| Test regex | Source string (element.js) | Match? |
|---|---|---|
/Grabbed .+\. Use arrow keys to (?:move|reorder)\./ |
Grabbed ${label}. Use arrow keys to move. (line 345/430) |
Yes |
| same | Grabbed ${label}. Use arrow keys to reorder. (line 787/831) |
Yes |
/(?:Drag|Reorder) cancelled\./ |
Drag cancelled. (line 440) |
Yes |
| same | Reorder cancelled. (line 841) |
Yes |
Regexes are specific enough — they won't false-positive on unrelated text. The (?:move|reorder) alternation correctly covers both drag-item and drag-list contexts.
Regression detection: If the a11y contract regresses (class not added, attribute removed, announcement text changed, live-region element removed), the tests will fail. Verified each assertion path.
Popover ARIA: aria-haspopup assertion changed from "true" to "dialog" — matches popover.js line 211 which sets "dialog".
Fixtures
Table: else="noUsersTpl" + <template id="noUsersTpl"> matches Core v1.15 else="templateId" semantics.
DnD events: on:nojs:dnd-receive, on:nojs:dnd-remove, on:nojs:dnd-reorder match the CustomEvent names dispatched in element.js (lines 1042, 1033, 1014).
CI YAML
- Triggers:
push: [main]+pull_request: [main]— runs on PRs targeting main. Correct. - Core clone:
git clone --depth 1from main (unpinned). This matches the existingbuild-and-testjob pattern. Unpinned main means Core breakage can red-light Elements CI — acceptable since they're tightly coupled. - Chromium-only: PR body documents the rationale and states 3-browser validation is local-only. Acceptable trade-off.
- No Playwright browser caching: Each run downloads chromium. Works but adds ~10s. Not a blocker.
CI Check Status
The e2e job failed (12s runtime). Root cause: npm ci on NoJS Core failed with Missing: conventional-commits-parser@6.4.0 from lock file. This is a lockfile sync issue in the Core repo (likely from the merged commitlint bump in PR #299), not caused by this PR. The build-and-test job passed (28s). A CI re-run after Core's lockfile is fixed should pass.
Hygiene
- dist/ committed: All three formats (cjs, esm, iife) contain the visibility hidden pattern and
_isPopoverOpenhelper. Consistent with source. - No attribution trailers: Verified via
--json commits— clean. - Commit messages: Use branch-name prefix (
fix/80-e2e-restoration:) rather than standard conventional format. The PR title itself (fix(e2e): ...) is conventional. Nit — since the project uses merge commits (not squash), individual commit format is visible in history.
Not Verified
Did not rerun popover.spec.ts or dropdown.spec.ts in a scratch clone. The code-level analysis of the inline-style approach is thorough and the approach is sound — the dropdown code path never sets inline visibility.
Verdict
Approve. The popover fix is correct across all three open paths, the inline style approach is properly scoped to avoid dropdown interference, test assertions match the shipped a11y contract, fixtures use the current Core semantics, and dist/ is consistent. The CI e2e failure is an upstream Core lockfile issue — re-run after that is fixed.
One follow-up item: wrap the two bare togglePopover() calls in try/catch for exception safety parity with popoverApi.open.
togglePopover()/showPopover() can throw, and _positionPopover() can throw after a successful open (e.g. getBoundingClientRect on a detached anchor). Either would leave style.visibility='hidden' stuck on an open popover. Wrap the hide-open-position-clear sequence in try/finally in all three open paths (click handler, popoverApi.open, popoverApi.toggle) so the inline visibility is cleared unconditionally.
Core's package-lock.json was regenerated under npm 11 (node 26) during the v1.20.1 release. npm 10 (node 20) rejects it with missing dependency errors and EBADENGINE warnings from @commitlint/cli@21 (requires node >=22.12).
1 participant
Summary
Refs #80
Restores the full e2e suite to green (435/435 across chromium, firefox, webkit) by addressing all 14 pre-existing failures documented in the root-cause analysis:
@dev commits (already on branch):
src/popover/popover.jsto prevent the 1-frame misposition that swallowed outside clicksdocs/examples/table/index.htmlfrom removed sibling<tr else>pattern toelse="templateId"+<template>docs/examples/dnd/index.htmlfromon:receive/on:removetoon:nojs:dnd-receive/on:nojs:dnd-removeto match the BREAKING rename in commit 1b109ce@qa commits (this PR):
aria-grabbedto assert the current a11y contract —aria-roledescription="draggable item"on draggable items,nojs-draggingclass toggle on Space/Escape keyboard drag, and live-region announcements ("Grabbed ... Use arrow keys to move/reorder." / "Drag/Reorder cancelled.")aria-haspopupexpectation from"true"to"dialog"to matchpopover.jsimplementatione2ejob to.github/workflows/ci.ymlthat clones + builds NoJS Core, builds Elements, installs Playwright chromium, and runsnpx playwright test --project=chromium— closing the 7-week CI gap that let stale fixtures and tests rot undetectedTest results
CI design choice
The e2e job runs chromium-only in CI to keep runner time reasonable (the full 3-browser suite takes ~45s locally, chromium-only takes ~15s). Chromium covers the rendering engine used by >70% of users; firefox and webkit are validated locally before merge. This matches the existing
workers: 1CI config inplaywright.config.tswhich already trades speed for determinism.Stale artifacts
test-results/andplaywright-report/directories are already in.gitignoreand are NOT tracked in git (verified viagit ls-files). No cleanup needed.