Skip to content

Commit e383f9b

Browse files
committed
fix(notes): address review findings on teleprompter mode
Play was a no-op whenever there was nothing left to scroll: a note that fits the window, or a run that had already reached the bottom, flipped the button to Pause and back within two frames. isAtTeleprompterEnd() now reports the end only when there is somewhere to go, and starting playback at the bottom rewinds to the top instead of leaving the control looking inert. Playback also tracked its position through the DOM, re-reading scrollTop every frame. At the slowest speed a frame advances ~0.17px, which an engine that snaps scroll offsets to whole pixels would round away, stalling the teleprompter. resolveTeleprompterPosition() keeps the fractional position internally and hands control back to the DOM once it drifts past a pixel, so scrolling by hand mid-playback still moves the teleprompter rather than fighting it. Also: - Drop NotesToolbar.browser.test.tsx and port what jsdom can assert into NotesToolbar.test.tsx. vitest.config.ts excludes src/**/*.browser.test.*, and the repo has no vitest.browser.config.ts and no test:browser script, so the file never ran anywhere — CI included. - Restore the no-scrollbar utility the toolbar lost when it was split into two rows; without it both rows paint a horizontal scrollbar once they overflow. - Convert the remaining tiptap spacing to em so it tracks the teleprompter font size. Identical at the default 16px; heading margins stay in rem because em there resolves against the heading's own enlarged size. - Stop announcing playback twice: the play button keeps its state-dependent label and drops aria-pressed, via a new highlighted prop that styles without the toggle semantics. - Make the note read-only while playing and disable formatting with it, so a keystroke can no longer yank the scroll through scroll-caret-into-view. - Move the generic units.* keys from the launch namespace to common. - Skip the mount-time settings write, load the initial note once, and drop a no-op transform-origin rule.
1 parent afac668 commit e383f9b

34 files changed

Lines changed: 411 additions & 228 deletions

src/components/launch/NotesToolbar.browser.test.tsx

Lines changed: 0 additions & 136 deletions
This file was deleted.

src/components/launch/NotesToolbar.test.tsx

Lines changed: 92 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,15 +2,22 @@ import "@testing-library/jest-dom";
22
import { render, screen, within } from "@testing-library/react";
33
import userEvent from "@testing-library/user-event";
44
import type { Editor } from "@tiptap/react";
5-
import { type ReactNode, useLayoutEffect } from "react";
6-
import { describe, expect, it, vi } from "vitest";
5+
import { type ReactNode, useLayoutEffect, useState } from "react";
6+
import { beforeEach, describe, expect, it, vi } from "vitest";
77
import { I18nProvider, useI18n } from "@/contexts/I18nContext";
8+
import { LOCALE_STORAGE_KEY } from "@/i18n/config";
89
import { NotesToolbar, type NotesToolbarProps } from "./NotesToolbar";
910

1011
vi.mock("@/components/ui/tooltip", () => ({
1112
Tooltip: ({ children }: { children: React.ReactNode }) => children,
1213
}));
1314

15+
beforeEach(() => {
16+
// `setLocale` persists its choice, and `I18nProvider` reads it back on mount — without
17+
// this, the one test that switches language would leak into every test after it.
18+
localStorage.removeItem(LOCALE_STORAGE_KEY);
19+
});
20+
1421
function createEditor(): Editor {
1522
const chain: Record<string, ReturnType<typeof vi.fn>> = {};
1623
for (const command of [
@@ -131,6 +138,40 @@ describe("NotesToolbar teleprompter controls", () => {
131138
expect(screen.queryByRole("button", { name: "Pause auto-scroll" })).not.toBeInTheDocument();
132139
});
133140

141+
it("announces playback through its label rather than a second pressed state", () => {
142+
const { rerender } = renderToolbar(createProps());
143+
expect(screen.getByRole("button", { name: "Start auto-scroll" })).not.toHaveAttribute(
144+
"aria-pressed",
145+
);
146+
147+
rerender(
148+
<I18nProvider>
149+
<NotesToolbar {...createProps({ isPlaying: true })} />
150+
</I18nProvider>,
151+
);
152+
expect(screen.getByRole("button", { name: "Pause auto-scroll" })).not.toHaveAttribute(
153+
"aria-pressed",
154+
);
155+
});
156+
157+
it("locks formatting while the teleprompter scrolls", () => {
158+
renderToolbar(createProps({ isPlaying: true }));
159+
160+
// Sweep the whole row rather than a hand-written list, so a formatting button added
161+
// later cannot quietly escape the lock.
162+
const formatting = within(screen.getByTestId("notes-formatting-controls")).getAllByRole(
163+
"button",
164+
);
165+
expect(formatting).toHaveLength(7);
166+
for (const button of formatting) {
167+
expect(button).toBeDisabled();
168+
}
169+
170+
// Teleprompter controls stay live so playback can always be stopped.
171+
expect(screen.getByRole("button", { name: "Pause auto-scroll" })).toBeEnabled();
172+
expect(screen.getByRole("button", { name: "Mirror horizontally" })).toBeEnabled();
173+
});
174+
134175
it("formats readout values for the active locale", () => {
135176
renderToolbar(createProps(), "ar");
136177
const speed = within(screen.getByRole("group", { name: "سرعة التمرير" })).getByRole("status");
@@ -142,3 +183,52 @@ describe("NotesToolbar teleprompter controls", () => {
142183
expect(fontSize).toHaveTextContent(`${new Intl.NumberFormat("ar").format(16)} بكسل`);
143184
});
144185
});
186+
187+
function ToolbarHarness() {
188+
const [isPlaying, setIsPlaying] = useState(false);
189+
const [mirrored, setMirrored] = useState(false);
190+
191+
return (
192+
<NotesToolbar
193+
{...createProps({
194+
isPlaying,
195+
mirrored,
196+
onTogglePlaying: () => setIsPlaying((current) => !current),
197+
onToggleMirror: () => setMirrored((current) => !current),
198+
})}
199+
/>
200+
);
201+
}
202+
203+
describe("NotesToolbar keyboard reachability", () => {
204+
it("walks every teleprompter control in order and toggles them from the keyboard", async () => {
205+
const user = userEvent.setup();
206+
const { container } = render(
207+
<I18nProvider>
208+
<ToolbarHarness />
209+
</I18nProvider>,
210+
);
211+
212+
const row = container.querySelector<HTMLElement>('[data-testid="notes-teleprompter-controls"]');
213+
const controls = Array.from(
214+
container.querySelectorAll<HTMLButtonElement>("[data-teleprompter-control]"),
215+
);
216+
expect(row).not.toBeNull();
217+
expect(controls).toHaveLength(6);
218+
219+
controls[0]?.focus();
220+
expect(document.activeElement).toBe(controls[0]);
221+
for (let index = 1; index < controls.length; index++) {
222+
await user.tab();
223+
expect(document.activeElement).toBe(controls[index]);
224+
}
225+
226+
controls[0]?.focus();
227+
await user.keyboard("{Enter}");
228+
expect(controls[0]).toHaveAttribute("aria-label", "Pause auto-scroll");
229+
230+
const mirror = controls.at(-1);
231+
await user.click(mirror as HTMLButtonElement);
232+
expect(mirror).toHaveAttribute("aria-pressed", "true");
233+
});
234+
});

0 commit comments

Comments
 (0)