fix(playlists): address the review round — 11 findings
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 <body>; 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.
This commit is contained in:
@@ -105,11 +105,16 @@ function Row({
|
||||
// put and can explain itself instead of silently doing nothing.
|
||||
<button
|
||||
{...attributes}
|
||||
{...listeners}
|
||||
disabled={dragDisabled}
|
||||
{...(dragDisabled ? {} : listeners)}
|
||||
// `disabled` would drop it from the tab order AND suppress the tooltip, so the
|
||||
// reason reached nobody. aria-disabled keeps it focusable and the name carries the why.
|
||||
aria-disabled={dragDisabled || undefined}
|
||||
onClick={(e) => dragDisabled && e.preventDefault()}
|
||||
title={dragDisabled ? dragHint : t("playlists.dragHandle")}
|
||||
aria-label={t("playlists.dragHandleOf", { title: video.title })}
|
||||
className="text-muted enabled:hover:text-fg enabled:cursor-grab enabled:active:cursor-grabbing disabled:opacity-30 disabled:cursor-not-allowed"
|
||||
aria-label={
|
||||
dragDisabled ? dragHint : t("playlists.dragHandleOf", { title: video.title })
|
||||
}
|
||||
className={`text-muted ${dragDisabled ? "opacity-30 cursor-not-allowed" : "hover:text-fg cursor-grab active:cursor-grabbing"}`}
|
||||
>
|
||||
<GripVertical className="w-4 h-4" />
|
||||
</button>
|
||||
@@ -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<string>("");
|
||||
// The open player, as an index into the DISPLAYED rows (the queue); null = closed.
|
||||
const [playingIndex, setPlayingIndex] = useState<number | null>(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<string | null>(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
|
||||
// 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 <div className="text-muted text-sm p-4">{t("playlists.pickOne")}</div>;
|
||||
@@ -544,7 +576,7 @@ export default function Playlists() {
|
||||
</div>
|
||||
<div className="flex gap-1.5 mt-3 flex-wrap items-center">
|
||||
<button
|
||||
onClick={() => shown.length && setPlayingIndex(0)}
|
||||
onClick={() => shown.length && setPlayingId(shown[0]!.id)}
|
||||
disabled={!shown.length}
|
||||
className="inline-flex items-center gap-1.5 text-xs font-semibold px-3 py-1.5 rounded-lg bg-accent text-accent-fg disabled:opacity-40 hover:opacity-90 transition"
|
||||
>
|
||||
@@ -605,7 +637,7 @@ export default function Playlists() {
|
||||
</div>
|
||||
</div>
|
||||
|
||||
{items.length > 1 && (
|
||||
{(items.length > 1 || viewActive) && (
|
||||
<div className="flex items-center gap-2 mb-3 flex-wrap max-w-5xl">
|
||||
<label className="inline-flex items-center gap-1.5 text-xs text-muted cursor-pointer select-none">
|
||||
<input
|
||||
@@ -627,12 +659,17 @@ export default function Playlists() {
|
||||
{canSaveOrder && (
|
||||
<button
|
||||
onClick={saveOrder}
|
||||
title={t("playlists.saveOrderHint")}
|
||||
aria-describedby="pl-save-hint"
|
||||
className="inline-flex items-center gap-1.5 text-xs font-semibold px-2.5 py-1 rounded-lg border border-accent text-accent hover:bg-accent/10 transition"
|
||||
>
|
||||
<Check className="w-3.5 h-3.5" /> {t("playlists.saveOrder")}
|
||||
</button>
|
||||
)}
|
||||
{canSaveOrder && (
|
||||
<span id="pl-save-hint" className="text-[11px] text-muted/80">
|
||||
{t("playlists.saveOrderHint")}
|
||||
</span>
|
||||
)}
|
||||
{viewActive && (
|
||||
<span className="text-[11px] text-muted/80">{dragHint}</span>
|
||||
)}
|
||||
@@ -659,7 +696,14 @@ export default function Playlists() {
|
||||
</div>
|
||||
</div>
|
||||
) : (
|
||||
<DndContext sensors={sensors} collisionDetection={closestCenter} onDragEnd={onDragEnd}>
|
||||
<DndContext
|
||||
sensors={sensors}
|
||||
collisionDetection={closestCenter}
|
||||
onDragEnd={onDragEnd}
|
||||
// dnd-kit's defaults are hardcoded English. They were harmless while only a pointer could
|
||||
// drag; now that the keyboard can, they ARE the instructions for the feature.
|
||||
accessibility={{ screenReaderInstructions, announcements }}
|
||||
>
|
||||
{/* Not clipped, so the header's filter popovers can overflow the table box. */}
|
||||
<div className="max-w-5xl">
|
||||
<table className="w-full border-collapse text-sm">
|
||||
@@ -696,10 +740,10 @@ export default function Playlists() {
|
||||
</DndContext>
|
||||
)}
|
||||
|
||||
{playingIndex != null && shown[playingIndex] && (
|
||||
{playingIndex >= 0 && (
|
||||
<Suspense fallback={null}>
|
||||
<PlayerModal
|
||||
video={shown[playingIndex]}
|
||||
video={shown[playingIndex]!}
|
||||
queue={shown}
|
||||
startIndex={playingIndex}
|
||||
startAt={null}
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
import { useRef, useState, type ReactNode, type RefObject } from "react";
|
||||
import { useEffect, useRef, useState, type ReactNode, type RefObject } from "react";
|
||||
import { useTranslation } from "react-i18next";
|
||||
import { ArrowDown, ArrowUp, ListFilter, X } from "lucide-react";
|
||||
import { useDismiss } from "../lib/useDismiss";
|
||||
@@ -198,7 +198,23 @@ export function ColumnHead<T>({
|
||||
const { t } = useTranslation();
|
||||
const [openFilter, setOpenFilter] = useState<string | null>(null);
|
||||
const panelRef = useRef<HTMLDivElement | null>(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<HTMLButtonElement | null>(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 <body>,
|
||||
// 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 (
|
||||
<thead className="sticky top-0 z-base after:pointer-events-none after:absolute after:inset-x-0 after:top-full after:h-4 after:bg-[linear-gradient(to_bottom,var(--bg),transparent)] after:content-['']">
|
||||
@@ -222,14 +238,16 @@ export function ColumnHead<T>({
|
||||
}
|
||||
>
|
||||
<span className="inline-flex items-center gap-1">
|
||||
{/* 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 ? (
|
||||
<button
|
||||
onClick={() => col.sortable && onToggleSort(col.key)}
|
||||
className={`inline-flex items-center gap-1 ${
|
||||
col.sortable ? "hover:text-fg cursor-pointer" : "cursor-default"
|
||||
}`}
|
||||
onClick={() => onToggleSort(col.key)}
|
||||
className="inline-flex items-center gap-1 hover:text-fg cursor-pointer"
|
||||
>
|
||||
{col.header}
|
||||
{col.sortable && isSorted && sort && (
|
||||
{isSorted && sort && (
|
||||
sort.dir === "asc" ? (
|
||||
<ArrowUp className="w-3 h-3" />
|
||||
) : (
|
||||
@@ -237,10 +255,20 @@ export function ColumnHead<T>({
|
||||
)
|
||||
)}
|
||||
</button>
|
||||
) : (
|
||||
<span>{col.header}</span>
|
||||
)}
|
||||
{col.filter && (
|
||||
<button
|
||||
onClick={() => setOpenFilter((o) => (o === col.key ? null : col.key))}
|
||||
// Captured at open time rather than bound with `ref`, so it survives the close
|
||||
// (a `ref` scoped to the open column nulls out before focus can go back).
|
||||
onClick={(e) => {
|
||||
triggerRef.current = e.currentTarget;
|
||||
setOpenFilter((o) => (o === col.key ? null : col.key));
|
||||
}}
|
||||
aria-label={t("datatable.filter")}
|
||||
aria-expanded={openFilter === col.key}
|
||||
aria-haspopup="dialog"
|
||||
// Generous padding: the icon alone was a 14x14 hit target, the smallest in the app.
|
||||
className={`-m-1.5 p-1.5 transition ${
|
||||
filterOn ? "text-accent" : "text-muted/60 hover:text-fg"
|
||||
|
||||
@@ -42,8 +42,7 @@
|
||||
"pushDone": "“{{name}}” synced to YouTube ✓",
|
||||
"pushPartial": "Synced, but {{count}} item(s) were skipped.",
|
||||
"pushFailed": "Couldn't sync to YouTube.",
|
||||
"deleteOnYoutubeConfirm": "Delete on YouTube",
|
||||
"deleteHereOnly": "Here only",
|
||||
"deleteHereOnly": "Delete here only",
|
||||
"deleteYtFailed": "Couldn't delete on YouTube; removed here only.",
|
||||
"sortLabel": "Sort",
|
||||
"dirAsc": "Ascending",
|
||||
@@ -73,5 +72,12 @@
|
||||
"reorderFailed": "Couldn't save the new order.",
|
||||
"renameFailed": "Couldn't rename the playlist.",
|
||||
"deleteFailed": "Couldn't delete the playlist.",
|
||||
"confirmDeleteLinked": "Delete the playlist “{{name}}”? It is synced with YouTube — choose whether to delete it there as well. This can't be undone."
|
||||
"confirmDeleteLinked": "Delete the playlist “{{name}}”? It is synced with YouTube — choose whether to delete it there as well. This can't be undone.",
|
||||
"dragBlockedBuiltin": "This list's order is fixed — it can't be reordered by hand.",
|
||||
"deleteBoth": "Delete here and on YouTube",
|
||||
"dndInstructions": "Press space to pick up a video, the arrow keys to move it, space again to drop it, or escape to cancel.",
|
||||
"dndPickedUp": "Picked up “{{title}}”, position {{pos}} of {{total}}.",
|
||||
"dndMovedOver": "“{{title}}” moved to position {{pos}} of {{total}}.",
|
||||
"dndDropped": "“{{title}}” dropped at position {{pos}} of {{total}}.",
|
||||
"dndCancelled": "Move cancelled; “{{title}}” is back at position {{pos}}."
|
||||
}
|
||||
|
||||
@@ -42,8 +42,7 @@
|
||||
"pushDone": "„{{name}}” szinkronizálva a YouTube-ra ✓",
|
||||
"pushPartial": "Szinkronizálva, de {{count}} elem kimaradt.",
|
||||
"pushFailed": "Nem sikerült a YouTube-ra szinkronizálás.",
|
||||
"deleteOnYoutubeConfirm": "Törlés a YouTube-on",
|
||||
"deleteHereOnly": "Csak itt",
|
||||
"deleteHereOnly": "Törlés csak itt",
|
||||
"deleteYtFailed": "Nem sikerült törölni a YouTube-on; csak itt lett eltávolítva.",
|
||||
"sortLabel": "Rendezés",
|
||||
"dirAsc": "Növekvő",
|
||||
@@ -73,5 +72,12 @@
|
||||
"reorderFailed": "Nem sikerült elmenteni az új sorrendet.",
|
||||
"renameFailed": "Nem sikerült átnevezni a listát.",
|
||||
"deleteFailed": "Nem sikerült törölni a listát.",
|
||||
"confirmDeleteLinked": "Törlöd a(z) „{{name}}” listát? A YouTube-bal szinkronban van — válaszd ki, hogy ott is törlődjön-e. Nem vonható vissza."
|
||||
"confirmDeleteLinked": "Törlöd a(z) „{{name}}” listát? A YouTube-bal szinkronban van — válaszd ki, hogy ott is törlődjön-e. Nem vonható vissza.",
|
||||
"dragBlockedBuiltin": "Ennek a listának kötött a sorrendje — kézzel nem rendezhető át.",
|
||||
"deleteBoth": "Törlés itt és a YouTube-on",
|
||||
"dndInstructions": "Szóközzel felveszed a videót, a nyilakkal mozgatod, újabb szóközzel leteszed, Escape-pel megszakítod.",
|
||||
"dndPickedUp": "„{{title}}” felvéve, {{pos}}. a(z) {{total}} közül.",
|
||||
"dndMovedOver": "„{{title}}” a(z) {{pos}}. helyre került a(z) {{total}} közül.",
|
||||
"dndDropped": "„{{title}}” letéve a(z) {{pos}}. helyre a(z) {{total}} közül.",
|
||||
"dndCancelled": "Mozgatás megszakítva; „{{title}}” visszakerült a(z) {{pos}}. helyre."
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user