From efd46a01c8ab2a8ab93d349ac23844051cf63f7e Mon Sep 17 00:00:00 2001 From: npeter83 Date: Mon, 20 Jul 2026 23:45:31 +0200 Subject: [PATCH] =?UTF-8?q?fix(playlists):=20address=20the=20review=20roun?= =?UTF-8?q?d=20=E2=80=94=2011=20findings?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Header (shared, so every table gets these): - The filter funnel could not close itself: useDismiss only knew the panel, so the trigger's mousedown closed it and the click reopened it. It now knows the trigger, captured at open time so it survives the close. - Escape stranded focus on ; it goes back to the funnel, which also gained aria-expanded/aria-haspopup. - A non-sortable header rendered a focusable no-op button — two unlabeled dead stops per playlist table, whose grip and actions headers are empty. Plain span unless the column sorts. Playlist: - The disabled drag grip explained itself to nobody: `disabled` drops a button from the tab order and suppresses its tooltip. It is aria-disabled now, stays focusable, and the reason is its accessible name. - That reason also lied on a built-in list, where the block has nothing to do with sorting or filtering. Third branch, own string. - The open player followed a position in a derived list, so a refetch or a view change could slide a different video under it or unmount it mid-playback. It follows the video id, like PlayerModal's own queue already does. - A failed reorder could not resync: the id set is unchanged by a reorder, so the arriving detail was judged "same list" and the rejected order stayed on screen as the base for the next drag. - The same guard is now keyed by playlist. Two lists holding the same videos looked identical to it, so the second rendered in the first's order and inherited its undo stack — where Undo would write it back to the wrong list. - A one-item list filtered to nothing had no Reset view: the whole control strip was gated on the item count while the funnels rendered regardless. - Both delete outcomes now read as actions ("Delete here only" / "Delete here and on YouTube"); "Here only" scanned as the safe choice while also deleting. - Keyboard dragging shipped with dnd-kit's hardcoded English screen-reader instructions and announcements. Localized, with live positions. --- frontend/src/components/Playlists.tsx | 90 +++++++++++++++------ frontend/src/components/tableHeader.tsx | 64 ++++++++++----- frontend/src/i18n/locales/en/playlists.json | 12 ++- frontend/src/i18n/locales/hu/playlists.json | 12 ++- 4 files changed, 131 insertions(+), 47 deletions(-) diff --git a/frontend/src/components/Playlists.tsx b/frontend/src/components/Playlists.tsx index aabe7f83..2d6dee92 100644 --- a/frontend/src/components/Playlists.tsx +++ b/frontend/src/components/Playlists.tsx @@ -105,11 +105,16 @@ function Row({ // put and can explain itself instead of silently doing nothing. @@ -156,6 +161,10 @@ export default function Playlists() { // Was a bare .then(): a failed reorder left the UI showing an order the server had // rejected, and produced an unhandled rejection. Say so and resync from the server. notifyYouTubeActionError(e, t("playlists.reorderFailed")); + // Clear the membership guard first: a reorder never changes the id SET, so without this + // the arriving detail is judged "same list" and the rejected order stays on screen — and + // becomes the base for the next drag. + lastSetRef.current = ""; qc.invalidateQueries({ queryKey: ["playlist", selectedId] }); }); }, @@ -169,8 +178,10 @@ export default function Playlists() { // re-confirms our own reorder doesn't wipe the undo history — we reset history only when // the actual set of items changes (different playlist, or an add/remove). const lastSetRef = useRef(""); - // The open player, as an index into the DISPLAYED rows (the queue); null = closed. - const [playingIndex, setPlayingIndex] = useState(null); + // The open player is tracked by video ID, not by a position: `shown` is derived, so a refetch or + // a change of view would otherwise slide a different video under the index — or unmount the player + // mid-playback. PlayerModal already tracks its own queue by id; this is the parent half of that. + const [playingId, setPlayingId] = useState(null); const listQuery = useQuery({ queryKey: ["playlists"], @@ -191,7 +202,9 @@ export default function Playlists() { useEffect(() => { if (!detail) return; - const key = idsKey(detail.items); + // Scoped to the playlist: two lists holding the SAME videos would otherwise look identical to + // the guard, so the second would render in the first's order and inherit its undo stack. + const key = `${selectedId}:${idsKey(detail.items)}`; if (key !== lastSetRef.current) { lastSetRef.current = key; order.reset(detail.items); @@ -218,9 +231,7 @@ export default function Playlists() { const [pushing, setPushing] = useState(false); const [reverting, setReverting] = useState(false); - const onPlay = useCallback((v: Video) => { - setPlayingIndex((_) => shownRef.current.findIndex((x) => x.id === v.id)); - }, []); + const onPlay = useCallback((v: Video) => setPlayingId(v.id), []); const onRemove = useCallback((v: Video) => { removeRef.current(v); }, []); @@ -240,7 +251,8 @@ export default function Playlists() { () => applyView(filterRows(items, columns, filters), columns, viewSort, groupBy), [items, columns, filters, viewSort, groupBy] ); - // Read by the memoized onPlay callback without making it depend on the list. + // Read by the drag announcements, which must report the CURRENT positions without making the + // announcement closures a render dependency of DndContext. const shownRef = useRef(shown); shownRef.current = shown; @@ -271,6 +283,22 @@ export default function Playlists() { useSensor(KeyboardSensor, { coordinateGetter: sortableKeyboardCoordinates }) ); + const posOf = (id: string | number) => shownRef.current.findIndex((v) => v.id === id) + 1; + const titleOf = (id: string | number) => + shownRef.current.find((v) => v.id === id)?.title ?? ""; + const total = () => shownRef.current.length; + const screenReaderInstructions = { draggable: t("playlists.dndInstructions") }; + const announcements = { + onDragStart: ({ active }: { active: { id: string | number } }) => + t("playlists.dndPickedUp", { title: titleOf(active.id), pos: posOf(active.id), total: total() }), + onDragOver: ({ active }: { active: { id: string | number } }) => + t("playlists.dndMovedOver", { title: titleOf(active.id), pos: posOf(active.id), total: total() }), + onDragEnd: ({ active }: { active: { id: string | number } }) => + t("playlists.dndDropped", { title: titleOf(active.id), pos: posOf(active.id), total: total() }), + onDragCancel: ({ active }: { active: { id: string | number } }) => + t("playlists.dndCancelled", { title: titleOf(active.id), pos: posOf(active.id) }), + }; + const plName = (p: { kind: string; name: string }) => playlistName(p, t); function refreshAll() { @@ -378,7 +406,7 @@ export default function Playlists() { // drops the item from every snapshot, so the stack stays valid for what's left. const next = items.filter((v) => v.id !== video.id); order.rebase((list) => list.filter((v) => v.id !== video.id)); - lastSetRef.current = idsKey(next); + lastSetRef.current = `${selectedId}:${idsKey(next)}`; try { await api.removeFromPlaylist(selectedId, video.id); refreshAll(); @@ -419,7 +447,7 @@ export default function Playlists() { choices: offerYoutube ? [ { id: "here", label: t("playlists.deleteHereOnly"), danger: true }, - { id: "both", label: t("playlists.deleteOnYoutubeConfirm"), danger: true }, + { id: "both", label: t("playlists.deleteBoth"), danger: true }, ] : [{ id: "here", label: t("playlists.delete"), danger: true }], }); @@ -451,11 +479,15 @@ export default function Playlists() { // Stable identity: PlayerModal's keydown/scroll-lock effect depends on it, so a fresh closure // every render re-subscribed those listeners and re-ran focusModal() under the open player. - const closePlayer = useCallback(() => setPlayingIndex(null), []); + const closePlayer = useCallback(() => setPlayingId(null), []); - const dragHint = filterActive - ? t("playlists.dragBlockedFilter") - : t("playlists.dragBlockedSort"); + // Order matters: a built-in list is never reorderable, whatever the view says. + const dragHint = !canEdit + ? t("playlists.dragBlockedBuiltin") + : filterActive + ? t("playlists.dragBlockedFilter") + : t("playlists.dragBlockedSort"); + const playingIndex = playingId ? shown.findIndex((v) => v.id === playingId) : -1; if (selectedId == null) { return
{t("playlists.pickOne")}
; @@ -544,7 +576,7 @@ export default function Playlists() {
- {items.length > 1 && ( + {(items.length > 1 || viewActive) && (
) : ( - + {/* Not clipped, so the header's filter popovers can overflow the table box. */}
@@ -696,10 +740,10 @@ export default function Playlists() { )} - {playingIndex != null && shown[playingIndex] && ( + {playingIndex >= 0 && ( ({ const { t } = useTranslation(); const [openFilter, setOpenFilter] = useState(null); const panelRef = useRef(null); - useDismiss(!!openFilter, () => setOpenFilter(null), panelRef); + // The funnel that opened the panel. `useDismiss` must know about it too, or the trigger's own + // mousedown closes the panel and the click that follows immediately reopens it — clicking the + // funnel a second time then looks like it does nothing. Also the thing to hand focus back to. + const triggerRef = useRef(null); + const restoreFocus = useRef(false); + useDismiss(!!openFilter, () => setOpenFilter(null), [panelRef, triggerRef]); + + // Escape (or an outside click) used to unmount the focused input and strand focus on , + // so the next Tab restarted from the top of the page. + useEffect(() => { + if (openFilter) { + restoreFocus.current = true; + } else if (restoreFocus.current) { + restoreFocus.current = false; + triggerRef.current?.focus(); + } + }, [openFilter]); return ( @@ -222,25 +238,37 @@ export function ColumnHead({ } > - + {/* Only a sortable header is a control. A non-sortable one used to render a + focusable no-op button, which put unlabeled dead stops in the tab order — + two of them per playlist table, whose grip and actions headers are empty. */} + {col.sortable ? ( + + ) : ( + {col.header} + )} {col.filter && (