mirror of
https://github.com/got-feedBack/feedBack.git
synced 2026-08-12 03:41:40 +00:00
fix(v3): dismiss Section Practice popover when another player popover opens (#638)
* fix(v3): dismiss Section Practice popover when another player popover opens The Section Practice popover (Songs > Song > Practice pill) stayed open when the user then clicked a v3 player-rail icon (Plugins, Audio, …), leaving two popovers stacked on top of each other. Reported on 0.3.0 (macOS) and still reproducing in the 2026-06-28 build. Root cause: the popover's outside-click dismiss was bound in the bubbling phase, but the v3 rail's icon buttons call e.stopPropagation() in their click handler (player-chrome.js wireRail), which kills bubbling before the click reaches document. So the dismiss listener never fired for a rail-icon click and the popover was orphaned open. Fix: bind the outside-click dismiss in the capture phase, which runs before the target's handler so stopPropagation() can't swallow it. This mirrors the audio mixer popover (audio-mixer.js), which already dismisses outside-clicks via capture-phase listeners for exactly this reason. Esc handling stays in the bubble phase (no rail handler stops keydown propagation, and capturing it would reorder it ahead of the player's Escape-to-exit handling). Shared app.js code, so v2 is covered too; v2 has no stopPropagation rail, so its outside-click dismiss behaviour is unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QbexxfTt8q2tAn436MqGWF * chore(#638): add CHANGELOG entry + capture-phase regression test Review follow-ups for the Section Practice popover dismiss fix: - CHANGELOG [Unreleased] → Fixed entry (repo workflow requires one). - tests/js/section_practice_dismiss.test.js pins the fix: the outside-click dismiss binds in the CAPTURE phase (so a rail icon's stopPropagation can't swallow it), exactly one capture binding (Escape keydown stays bubble-phase), and the #section-practice-control containment guard (no self-close). A revert to bubble-phase fails the test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: byrongamatos <xasiklas@gmail.com>
This commit is contained in:
co-authored by
Claude Opus 4.8
byrongamatos
parent
8fbbc761fc
commit
199550e5fb
@@ -0,0 +1,43 @@
|
||||
// Guards the Section Practice popover's outside-click dismiss in static/app.js
|
||||
// (_installSectionPracticeDismiss). The v3 player-rail icon buttons call
|
||||
// e.stopPropagation() in their click handler (static/v3/player-chrome.js
|
||||
// wireRail), so a BUBBLE-phase document dismiss never fires when the user clicks
|
||||
// a different rail icon (Plugins, Audio, …) — leaving the Practice popover
|
||||
// stranded open under the newly-opened one (feedBack#638). The dismiss must bind
|
||||
// in the CAPTURE phase (runs before the target's stopPropagation can swallow it).
|
||||
// Esc must stay bubble-phase so it doesn't reorder ahead of the player's
|
||||
// Escape-to-exit handling. A revert to bubble-phase should fail here.
|
||||
//
|
||||
// Source-level only — same strategy as the other tests/js/ files.
|
||||
|
||||
const { test } = require('node:test');
|
||||
const assert = require('node:assert/strict');
|
||||
const fs = require('node:fs');
|
||||
const path = require('node:path');
|
||||
|
||||
const src = fs.readFileSync(path.join(__dirname, '..', '..', 'static', 'app.js'), 'utf8');
|
||||
const m = src.match(/function _installSectionPracticeDismiss\s*\(\)\s*\{[\s\S]*?\n\}/);
|
||||
assert.ok(m, '_installSectionPracticeDismiss() not found in static/app.js');
|
||||
const body = m[0];
|
||||
|
||||
test('the outside-click dismiss binds in the CAPTURE phase', () => {
|
||||
assert.match(
|
||||
body,
|
||||
/addEventListener\(\s*['"]click['"][\s\S]*?,\s*true\s*\)/,
|
||||
'the click dismiss must pass the capture flag (`, true`) so a rail icon\'s '
|
||||
+ 'stopPropagation() cannot swallow it',
|
||||
);
|
||||
});
|
||||
|
||||
test('only the click listener is capture (Escape keydown stays bubble-phase)', () => {
|
||||
// Exactly one capture binding in the installer — the click. The keydown
|
||||
// (Escape) listener must NOT be capture.
|
||||
const captureBinds = body.match(/,\s*true\s*\)/g) || [];
|
||||
assert.equal(captureBinds.length, 1, 'expected exactly one capture-phase binding (the click)');
|
||||
});
|
||||
|
||||
test('the dismiss ignores clicks inside the control (no self-close)', () => {
|
||||
assert.match(body, /section-practice-control/, 'must scope to #section-practice-control');
|
||||
assert.match(body, /ctrl\s*&&\s*ctrl\.contains\(e\.target\)\)\s*return/,
|
||||
'a click inside the control (incl. the pill) must not dismiss the popover');
|
||||
});
|
||||
Reference in New Issue
Block a user