Skip to content

Commit 98932ee

Browse files
committed
fix(studio): let an author out of the preset and add-effect menus
Opening either menu hid both buttons, and the only thing that set them back was picking something. So an author who opened one and changed their mind had two ways out: add an effect they did not want, or deselect the clip and lose their place. Escape did nothing. The buttons now stay and close what they opened — same control, toggled, reading "Close" while its menu is up. Opening one closes the other, since two menus at once is two surfaces over the rack with nothing saying which the next click belongs to. Escape closes whichever is open. Bound on the section rather than the window, because a keystroke aimed at the timeline is not aimed at this, and it only stops propagation when it actually has a menu to close — the panel has its own Escape handling and swallowing the key unconditionally would break it. Closing reverts an audition in flight, for the same reason leaving the shelf with the pointer does: a preview left playing is audible over a chain the document does not have. Verified live, stepping one render at a time — clicking through the whole cycle in a single synchronous pass batches into one React render and only shows the final state, which is what made this look fixed when it was not. Falsified: a button that only opens, an Escape that does nothing, opening one menu without closing the other (in EITHER direction — the first test only covered one, and the reverse mutation survived it), and closing without reverting the audition each fail a test. studio 3710 passing, 18 todo.
1 parent 5cbe5f4 commit 98932ee

2 files changed

Lines changed: 149 additions & 22 deletions

File tree

‎packages/studio/src/components/editor/propertyPanelFxSection.test.tsx‎

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -695,6 +695,89 @@ describe("FxSection chain", () => {
695695
expect(item?.querySelector(".hf-fx-preset-name")?.textContent).toBe("Telephone");
696696
});
697697

698+
describe("getting back out of a menu", () => {
699+
/** Escape, from inside the section, the way a keystroke really arrives. */
700+
const escape = (host: HTMLElement) =>
701+
act(() => {
702+
host
703+
.querySelector(".hf-fx-section")
704+
?.dispatchEvent(new KeyboardEvent("keydown", { key: "Escape", bubbles: true }));
705+
});
706+
707+
it("closes the preset shelf with the button that opened it", () => {
708+
// Opening a menu used to hide both buttons, and picking something was the
709+
// only thing that set them back — so an author who changed their mind had
710+
// to add an effect they did not want, or deselect the clip.
711+
const { host } = mount();
712+
click(byText(host, "button", "Presets"));
713+
expect(host.querySelector(".hf-fx-preset-menu")).toBeTruthy();
714+
715+
click(byText(host, "button", "Close"));
716+
expect(host.querySelector(".hf-fx-preset-menu")).toBeNull();
717+
expect(byText(host, "button", "Presets")).toBeTruthy();
718+
});
719+
720+
it("closes the add menu the same way", () => {
721+
const { host } = mount();
722+
click(host.querySelector(".hf-fx-add"));
723+
expect(host.querySelector(".hf-fx-add-menu")).toBeTruthy();
724+
725+
click(host.querySelector(".hf-fx-add"));
726+
expect(host.querySelector(".hf-fx-add-menu")).toBeNull();
727+
expect(byText(host, "button", "Add effect")).toBeTruthy();
728+
});
729+
730+
it("opens one menu in place of the other", () => {
731+
// Two open at once is two surfaces covering the rack, and neither says
732+
// which one the next click belongs to.
733+
const { host } = mount();
734+
click(byText(host, "button", "Presets"));
735+
click(host.querySelector(".hf-fx-add"));
736+
expect(host.querySelector(".hf-fx-add-menu")).toBeTruthy();
737+
expect(host.querySelector(".hf-fx-preset-menu")).toBeNull();
738+
739+
// And back the other way, which is a separate handler.
740+
click(byText(host, "button", "Presets"));
741+
expect(host.querySelector(".hf-fx-preset-menu")).toBeTruthy();
742+
expect(host.querySelector(".hf-fx-add-menu")).toBeNull();
743+
});
744+
745+
it("closes on Escape, which is what anyone reaches for first", () => {
746+
const { host } = mount();
747+
click(byText(host, "button", "Presets"));
748+
escape(host);
749+
expect(host.querySelector(".hf-fx-preset-menu")).toBeNull();
750+
751+
click(host.querySelector(".hf-fx-add"));
752+
escape(host);
753+
expect(host.querySelector(".hf-fx-add-menu")).toBeNull();
754+
});
755+
756+
it("puts the chain back when a closing menu was auditioning", () => {
757+
// Leaving the shelf by closing it is still leaving it, and an audition
758+
// left playing is audible over a chain the document does not have.
759+
const { host, onChainPreview } = mount({ chain: chainOf("peaking") });
760+
click(byText(host, "button", "Presets"));
761+
act(() => (presetButton(host, "telephone") as HTMLElement | null)?.focus());
762+
click(byText(host, "button", "Close"));
763+
764+
const back = onChainPreview.mock.calls.at(-1)?.[0] as HfAudioFxChain;
765+
expect(back.nodes.map((n) => n.type)).toEqual(["peaking"]);
766+
});
767+
768+
it("leaves Escape alone when no menu is open", () => {
769+
// The panel has its own Escape handling; swallowing the key when this has
770+
// nothing to close would break it.
771+
const { host } = mount();
772+
let reached = false;
773+
host.addEventListener("keydown", () => {
774+
reached = true;
775+
});
776+
escape(host);
777+
expect(reached).toBe(true);
778+
});
779+
});
780+
698781
it("shows a wave on a preset only when hovering it can be heard", () => {
699782
// Hovering plays the preset, and playing is otherwise invisible — the panel
700783
// looks identical whether the audition is sounding or the pointer is just

‎packages/studio/src/components/editor/propertyPanelFxSection.tsx‎

Lines changed: 66 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
* is not an entry in the chain.
66
*/
77

8-
import { useCallback, useEffect, useMemo, useRef, useState } from "react";
8+
import { useCallback, useEffect, useMemo, useRef, useState, type KeyboardEvent } from "react";
99
import {
1010
defaultAudioFxParams,
1111
getAudioFxDef,
@@ -499,8 +499,34 @@ export function FxSection({
499499
[chain.nodes, mutate],
500500
);
501501

502+
/**
503+
* Escape closes whichever menu is open.
504+
*
505+
* The first thing anyone reaches for, and on a surface that covers the rack it
506+
* is the one that needs no discovering. Bound on the section rather than the
507+
* window: a keystroke aimed at the timeline is not aimed at this.
508+
*/
509+
const closeMenus = useCallback(
510+
(event: KeyboardEvent) => {
511+
if (event.key !== "Escape" || (!adding && !picking)) return;
512+
// Stops the panel's own Escape handling from also firing — closing a menu
513+
// and deselecting the clip on one keystroke loses the author their place.
514+
event.stopPropagation();
515+
audition(null);
516+
onAuditionLevel?.(false);
517+
setAdding(false);
518+
setPicking(false);
519+
},
520+
[adding, picking, audition, onAuditionLevel],
521+
);
522+
502523
return (
503-
<div className="hf-fx-section space-y-2">
524+
<div
525+
className="hf-fx-section space-y-2"
526+
// Focus lives on the buttons and menu items inside, so the keystroke
527+
// bubbles to here without the section needing focus of its own.
528+
onKeyDown={closeMenus}
529+
>
504530
<div className="hf-fx-chain space-y-1">
505531
{/* The rack IS the signal path, and saying so costs two lines. Without
506532
them the order reads as a list, which is the one reading that makes
@@ -791,26 +817,44 @@ export function FxSection({
791817
/>
792818
) : null}
793819

794-
{adding || picking ? null : (
795-
<div className="flex gap-1">
796-
<button
797-
type="button"
798-
className="hf-fx-preset w-full rounded-[4px] border border-dashed border-panel-border-input py-1 text-[11px] text-panel-text-4 hover:text-panel-text-0 disabled:opacity-40"
799-
disabled={disabled}
800-
onClick={() => setPicking(true)}
801-
>
802-
Presets
803-
</button>
804-
<button
805-
type="button"
806-
className="hf-fx-add w-full rounded-[4px] border border-dashed border-panel-border-input py-1 text-[11px] text-panel-text-4 hover:text-panel-text-0 disabled:opacity-40"
807-
disabled={disabled}
808-
onClick={() => setAdding(true)}
809-
>
810-
Add effect
811-
</button>
812-
</div>
813-
)}
820+
{/* The buttons stay while their menu is open, and close it — an author who
821+
opened one and changed their mind had no way back: picking something
822+
was the only thing that set these false, so the only exits were adding
823+
an effect they did not want or deselecting the clip. */}
824+
<div className="flex gap-1">
825+
<button
826+
type="button"
827+
className="hf-fx-preset w-full rounded-[4px] border border-dashed border-panel-border-input py-1 text-[11px] text-panel-text-4 hover:text-panel-text-0 disabled:opacity-40"
828+
aria-expanded={picking}
829+
disabled={disabled}
830+
onClick={() => {
831+
// Leaving the shelf by closing it is still leaving it, and an
832+
// audition left playing is audible over a chain the document does
833+
// not have.
834+
if (picking) audition(null);
835+
setPicking(!picking);
836+
setAdding(false);
837+
}}
838+
>
839+
{picking ? "Close" : "Presets"}
840+
</button>
841+
<button
842+
type="button"
843+
className="hf-fx-add w-full rounded-[4px] border border-dashed border-panel-border-input py-1 text-[11px] text-panel-text-4 hover:text-panel-text-0 disabled:opacity-40"
844+
aria-expanded={adding}
845+
disabled={disabled}
846+
onClick={() => {
847+
if (adding) {
848+
audition(null);
849+
onAuditionLevel?.(false);
850+
}
851+
setAdding(!adding);
852+
setPicking(false);
853+
}}
854+
>
855+
{adding ? "Close" : "Add effect"}
856+
</button>
857+
</div>
814858
</div>
815859
);
816860
}

0 commit comments

Comments
 (0)