diff --git a/docs/unread.md b/docs/unread.md index 6443f8da68..040135b9a5 100644 --- a/docs/unread.md +++ b/docs/unread.md @@ -38,10 +38,18 @@ visible rows, and 300 ms dwell. Scroll/content/focus changes cancel/restart dwel scheduler when positioning finishes, even if an empty history page changes no rows or geometry. The list and its own composer share a reading surface; focus anywhere in an open -thread panel also qualifies, including its selected owning tab header. Explicitly returning to a channel resumes reading in its visible selected thread for that channel; a hidden pane or another channel’s tab keeps the main conversation’s focus. Pane motion completion restarts observation after unchanged rows enter the viewport. Inactive -or hidden tab content and passive restoration do not qualify. Another pane, a -dialog, background window, preload, -and mounted virtualizer overscan do not qualify. +thread panel also qualifies, including its selected owning tab header. Explicitly +returning to a channel reads both its main conversation and its visible selected +side-pane thread tab for that channel: while focus stays anywhere in that +channel's workspace (normally the main composer), that side-pane thread +qualifies too. A thread in the main pane never borrows this grant. A link to +a message or thread (Activity, a message link) is not a plain visit and reads +only what holds focus. A hidden or collapsed pane, another channel's tab, a +members or media-review view, and a passive restoration (`preserve` focus) keep +the main conversation as the only reading surface. Inactive or hidden tab +content does not qualify. Pane motion completion restarts observation after +unchanged rows enter the viewport. Another pane, a dialog, background window, +preload, and mounted virtualizer overscan do not qualify. Away from the bottom, observations mark individual fully visible messages. At the physical bottom, 300 ms visibility of the newest message's bottom edge also earns diff --git a/src/bundled/channels/ChannelsPage.tsx b/src/bundled/channels/ChannelsPage.tsx index 2b102703e5..af2a3a19f1 100644 --- a/src/bundled/channels/ChannelsPage.tsx +++ b/src/bundled/channels/ChannelsPage.tsx @@ -1304,11 +1304,14 @@ function ChannelWorkspace({ const hasPanel = companion || hasChannelPanel; const showingChannelPanel = tabState.paneOpen && hasChannelPanel; const showingPanel = companion || showingChannelPanel; - // An explicit channel opening resumes its visible selected thread. Passive - // restoration and another channel’s retained tab keep their existing focus. + // A plain channel visit reads its visible selected thread along with the + // conversation that holds focus. A link to a message or thread, passive + // restoration and another channel’s retained tab do not. const readingThread = showingChannelPanel && !companion && + !requestedMessage && + !requestedThread && !membersOpen && !showingMediaReview && (navigation?.focus ?? navigator?.snapshot().attempt.focus) !== "preserve" && @@ -1319,28 +1322,33 @@ function ChannelWorkspace({ tab.channelId === currentId, ); const split = usePanelSplit(); - const readingVisit = useRef(undefined); + // A routed thread can mount behind a collapsed pane, where it cannot take + // focus, or already be selected, so its pane does not move focus. Once its + // tab shows, hand it focus nothing else took. + const routedTab = + routedThread && !membersOpen + ? tabState.tabs.find( + (tab) => + tab.kind === "thread" && + tab.channelId === routedThread.channelId && + tab.messageId === routedThread.messageId && + (tabPane(tab) === "main" + ? tabState.mainSelected === tab.id + : showingChannelPanel && !companion && selectedTab === tab.id), + )?.id + : undefined; + const routedVisit = useRef(undefined); useLayoutEffect(() => { const visit = navigation?.signal ?? currentId; - if (!readingThread || readingVisit.current === visit) return; - readingVisit.current = visit; - const focused = document.activeElement; - // Only hand off the channel opener. A person who moved focus while the - // retained pane mounted keeps their new target. - if ( - focused !== document.body && - (!(focused instanceof Element) || - focused - .closest("[data-channel-id]") - ?.getAttribute("data-channel-id") !== currentId) - ) - return; + if (!routedTab || routedVisit.current === visit) return; + routedVisit.current = visit; + if (document.activeElement !== document.body) return; split.ref.current ?.querySelector( - `[role="tab"][data-tab-value="${CSS.escape(selectedTab)}"]`, + `[role="tab"][data-tab-value="${CSS.escape(routedTab)}"]`, ) ?.focus({ preventScroll: true }); - }, [readingThread, navigation?.signal, currentId, selectedTab, split.ref]); + }, [routedTab, navigation?.signal, currentId, split.ref]); useEffect(() => { // Only an explicit settings click moves focus; restoring a visit does not. if (settingsFocus) @@ -1514,8 +1522,7 @@ function ChannelWorkspace({ !current.readOnly && !requestedMessage && !requestedThread && - !routedThread && - !readingThread + !routedThread } trailingTool={ solo ? ( @@ -1535,6 +1542,7 @@ function ChannelWorkspace({ const workspace = (
{ + const h = setup({ focus: "outside" }); + const workspace = document.createElement("div"); + const side = document.createElement("div"); + side.setAttribute("data-panel-workspace", ""); + const surface = h.element.closest("aside"); + const unrelated = document.createElement("button"); + expect(surface).not.toBeNull(); + document.body.append(workspace, unrelated); + if (surface) side.append(surface); + workspace.append(side, h.outside); + h.retarget("thread-room"); + h.outside.focus(); + vi.advanceTimersByTime(300); + expect(h.reading).not.toHaveBeenCalled(); + workspace.setAttribute("data-reading-thread", ""); + h.retarget("thread-room-again"); + h.outside.focus(); + vi.advanceTimersByTime(299); + expect(h.leases.at(-1)?.observe).not.toHaveBeenCalled(); + vi.advanceTimersByTime(1); + expect(h.dwelled()?.observe).toHaveBeenCalledExactlyOnceWith(["visible"]); + const count = h.reading.mock.calls.length; + unrelated.focus(); + vi.advanceTimersByTime(300); + expect(h.reading).toHaveBeenCalledTimes(count); + // A thread in the main pane belongs to no side-pane selection: the same + // workspace focus does not read it. + side.setAttribute("data-main-workspace", ""); + side.removeAttribute("data-panel-workspace"); + h.retarget("thread-room-main"); + h.outside.focus(); + vi.advanceTimersByTime(300); + expect(h.reading).toHaveBeenCalledTimes(count); +}); it("focus moving from the list to its composer keeps reading; another surface's composer cancels it", () => { const h = setup(); vi.advanceTimersByTime(100); diff --git a/src/features/messages/use-reading.ts b/src/features/messages/use-reading.ts index 962c43f57e..88e95a2a54 100644 --- a/src/features/messages/use-reading.ts +++ b/src/features/messages/use-reading.ts @@ -70,6 +70,12 @@ export function useReading({ (element.contains(node) || !!surface?.contains(node) || focusedTab(node) || + // A plain channel visit also reads the side pane's visible selected + // thread while focus stays anywhere in that channel's workspace. + !!surface + ?.closest("[data-panel-workspace]") + ?.closest("[data-reading-thread]") + ?.contains(node) || !!composer?.current?.contains(node)); // Everything but geometry, so an event can check it without a layout. const ready = () => diff --git a/tests/browser/channel-tabs.spec.mjs b/tests/browser/channel-tabs.spec.mjs index 7b40d345d2..e733118ffd 100644 --- a/tests/browser/channel-tabs.spec.mjs +++ b/tests/browser/channel-tabs.spec.mjs @@ -270,10 +270,8 @@ test("content-opened tabs restore drafts without offering blank-tab creation", a exact: true, }); await expect(alphaComposer).toHaveText("Alpha draft"); - // Returning to Alpha resumes reading in its visible selected thread. - await expect( - workspace.locator('[role="tab"][data-tab-value^="thread:alpha:"]'), - ).toBeFocused(); + // Restored side content must not take focus from the main conversation. + await expect(alphaComposer).toBeFocused(); await workspace.getByRole("tab", { name: "Beta", exact: true }).click(); await hoverMessage(row); await row.getByRole("button", { name: "Reply", exact: true }).click(); diff --git a/tests/browser/thread-unread.spec.mjs b/tests/browser/thread-unread.spec.mjs index 5710a681fe..4d77cb72e9 100644 --- a/tests/browser/thread-unread.spec.mjs +++ b/tests/browser/thread-unread.spec.mjs @@ -126,6 +126,143 @@ test.describe("mentioned reply priority", () => { } }); + test("opening a thread from Activity reads it when its channel's pane was collapsed", async ({ + page, + app, + }) => { + // Hold the clock so the thread cannot be read before Activity opens it. + await page.clock.install(); + await holdReadingFocus(page); + await open(page, app); + const root = app.histories + .get("primary/alpha") + .find((row) => row.content === "Thread root 1"); + const panel = page.getByRole("complementary", { + name: "Thread", + exact: true, + }); + const unread = () => + page.evaluate( + (rootId) => + window.fixtureRelay.snapshot().session.unread.snapshot({ + kind: "thread", + channelId: "alpha", + rootId, + }), + root.id, + ); + await page + .locator(`[data-channel-timeline] [data-message-id="${root.id}"]`) + .getByRole("button", { name: /^View thread:/ }) + .click(); + await expect(panel).toBeVisible(); + await page.clock.runFor(1000); + // Collapse Alpha's pane around its selected thread, then leave the channel. + await page + .getByRole("button", { name: "Toggle tab pane", exact: true }) + .click(); + await page.locator("button[data-channel-id=beta]").click(); + await expect( + page.getByRole("textbox", { name: "Message #Beta", exact: true }), + ).toBeVisible(); + expect((await unread()).observedCount).toBeGreaterThan(0); + await page.locator('button[data-channel-id="alpha"]').hover(); + const item = page + .getByRole("dialog", { name: "Activity in Alpha" }) + .getByRole("button", { name: /Open unread thread from/ }) + .filter({ hasText: "Unread reply 1" }); + await expect(item).toBeVisible(); + await releaseReadingFocus(page); + await item.click(); + await expect(panel).toBeVisible(); + await page.clock.runFor(1000); + try { + await expect.poll(async () => (await unread()).observedCount).toBe(0); + } catch (error) { + const focus = await page.evaluate(() => ({ + active: + document.activeElement?.getAttribute("aria-label") ?? + document.activeElement?.tagName, + inThread: !!document.activeElement?.closest("[data-reading-surface]"), + })); + throw new Error( + `The thread opened but stayed unread. Focus: ${JSON.stringify(focus)}`, + { cause: error }, + ); + } + }); + + test("opening a main-pane thread from Activity reads it, not the side-pane thread", async ({ + page, + app, + }) => { + await page.clock.install(); + await holdReadingFocus(page); + await open(page, app); + const rows = app.histories.get("primary/alpha"); + const [x, t] = ["Thread root 1", "Thread root 0"].map((content) => + rows.find((row) => row.content === content), + ); + for (const root of [x, t]) { + await page + .locator(`[data-channel-timeline] [data-message-id="${root.id}"]`) + .getByRole("button", { name: /^View thread:/ }) + .click(); + await expect( + page.getByRole("complementary", { name: "Thread", exact: true }), + ).toBeVisible(); + } + const side = page.locator("[data-panel-workspace]"); + const main = page.locator("[data-main-workspace]"); + const tabs = (host) => + host.locator('[role="tab"][data-tab-value^="thread:"]'); + const unread = (rootId) => + page.evaluate( + (rootId) => + window.fixtureRelay.snapshot().session.unread.snapshot({ + kind: "thread", + channelId: "alpha", + rootId, + }).observedCount, + rootId, + ); + // Move X's tab (the first opened) to the main pane, leaving T in the side. + const [id] = await tabs(side).evaluateAll((all) => + all.map((tab) => tab.dataset.tabValue), + ); + const from = await page + .locator(`[role="tab"][data-tab-value="${id}"]`) + .boundingBox(); + const header = await main.locator("header").first().boundingBox(); + await page.mouse.move(from.x + from.width / 2, from.y + from.height / 2); + await page.mouse.down(); + await page.mouse.move( + header.x + header.width / 2, + header.y + header.height / 2, + { steps: 8 }, + ); + await page.mouse.up(); + await expect(tabs(main)).toHaveCount(1); + await expect(tabs(side)).toHaveCount(1); + await page.clock.runFor(1000); + expect(await unread(x.id)).toBeGreaterThan(0); + expect(await unread(t.id)).toBeGreaterThan(0); + + // Activity reopens X, already selected in the main pane. + await page.locator('button[data-channel-id="alpha"]').hover(); + const item = page + .getByRole("dialog", { name: "Activity in Alpha" }) + .getByRole("button", { name: /Open unread thread from/ }) + .filter({ hasText: "Unread reply 1" }); + await expect(item).toBeVisible(); + await releaseReadingFocus(page); + await item.click(); + await expect(item).toBeHidden(); + await page.clock.runFor(1000); + await expect.poll(() => unread(x.id)).toBe(0); + expect(await unread(t.id)).toBeGreaterThan(0); + }); + test("an Activity reopening leaves focus the person moved meanwhile", async ({ page, app, @@ -709,7 +846,7 @@ for (const { paneOpen, width } of [ { paneOpen: true, width: 1280 }, { paneOpen: false, width: 1440 }, ]) - test(`returning to a channel ${paneOpen ? "reads its already-open thread" : "leaves its hidden thread unread"} at ${width}px`, async ({ + test(`returning to a channel ${paneOpen ? "reads it and its already-open thread" : "leaves its hidden thread unread"} at ${width}px`, async ({ page, app, }) => { @@ -726,16 +863,23 @@ for (const { paneOpen, width } of [ name: "Thread", exact: true, }); - const unread = () => + const otherRoot = app.histories + .get("primary/alpha") + .find((row) => row.content === "Thread root 0"); + const observed = (target) => page.evaluate( - (rootId) => - window.fixtureRelay.snapshot().session.unread.snapshot({ - kind: "thread", - channelId: "alpha", - rootId, - }).observedCount, - root.id, + (target) => + window.fixtureRelay.snapshot().session.unread.snapshot(target) + .observedCount, + target, ); + const unread = (rootId = root.id) => + observed({ kind: "thread", channelId: "alpha", rootId }); + // The neighboring thread is never opened, so once the channel is read its + // replies are all that remains unread there. + const unreadBeyondNeighbor = async () => + (await observed({ kind: "channel", channelId: "alpha" })) - + (await unread(otherRoot.id)); await page .locator(`[data-channel-timeline] [data-message-id="${root.id}"]`) .getByRole("button", { name: /^View thread:/ }) @@ -754,6 +898,7 @@ for (const { paneOpen, width } of [ page.getByRole("textbox", { name: "Message #Beta", exact: true }), ).toBeVisible(); expect(await unread()).toBeGreaterThan(0); + expect(await unreadBeyondNeighbor()).toBeGreaterThan(0); await releaseReadingFocus(page); await alpha.click(); if (!paneOpen) { @@ -769,16 +914,162 @@ for (const { paneOpen, width } of [ panel.getByText("Unread reply 1", { exact: true }), ).toBeInViewport(); await expect( - page.locator(`[role="tab"][data-tab-value="thread:alpha:${root.id}"]`), + page.getByRole("textbox", { name: "Message #Alpha", exact: true }), ).toBeFocused(); - await expect.poll(unread).toBe(0); + await expect.poll(() => unread()).toBe(0); + // The channel's own conversation is read too. + await expect.poll(unreadBeyondNeighbor).toBe(0); // The other thread was never opened and remains unread. - const otherRoot = app.histories - .get("primary/alpha") - .find((row) => row.content === "Thread root 0"); await expect( page .locator(`[data-channel-timeline] [data-message-id="${otherRoot.id}"]`) .getByRole("button", { name: /^View thread:/ }), ).toHaveAccessibleName(/Observed unread replies/); }); + +// A link to a message reads only the conversation it focuses: the channel's +// already-open thread stays unread until the person goes there. +test("opening a message link reads the channel but not its already-open thread", async ({ + page, + app, +}) => { + await page.clock.install(); + await holdReadingFocus(page); + await open(page, app); + const history = app.histories.get("primary/alpha"); + const root = history.find((row) => row.content === "Thread root 1"); + // An ordinary message, not the open thread's root: a link to the root can + // open its thread instead, which then takes focus and reads it. + const message = history.findLast((row) => + row.content.startsWith("primary alpha message"), + ); + const panel = page.getByRole("complementary", { + name: "Thread", + exact: true, + }); + const observed = (target) => + page.evaluate( + (target) => + window.fixtureRelay.snapshot().session.unread.snapshot(target) + .observedCount, + target, + ); + const unread = () => + observed({ kind: "thread", channelId: "alpha", rootId: root.id }); + const channel = () => observed({ kind: "channel", channelId: "alpha" }); + await page + .locator(`[data-channel-timeline] [data-message-id="${root.id}"]`) + .getByRole("button", { name: /^View thread:/ }) + .click(); + await expect( + panel.getByText("Unread reply 1", { exact: true }), + ).toBeInViewport(); + // Setup must stay unread even when opening and switching channels is slow. + await page.clock.runFor(1000); + await page.locator("button[data-channel-id=beta]").click(); + await expect( + page.getByRole("textbox", { name: "Message #Beta", exact: true }), + ).toBeVisible(); + const before = await channel(); + expect(await unread()).toBeGreaterThan(0); + await releaseReadingFocus(page); + await page.evaluate((target) => window.fixtureNavigation.open(target), { + version: 1, + kind: "conversation", + channelId: "alpha", + messageId: message.id, + scope: { viewer: app.viewer, communityOrigin: "https://primary.example" }, + }); + await expect( + page.locator(`[data-channel-timeline] [data-message-id="${message.id}"]`), + ).toBeInViewport(); + await expect( + panel.getByText("Unread reply 1", { exact: true }), + ).toBeInViewport(); + // The channel earned its dwell; the thread had the same chance. + await expect.poll(channel).toBeLessThan(before); + await page.clock.runFor(1000); + expect(await unread()).toBeGreaterThan(0); +}); + +// A thread in the main pane is read only by focus in its own surface: a plain +// visit's workspace-wide grant belongs to the side pane's selected thread, so a +// live reply in main-pane X must stay unread while focus rests in side-pane T. +test("a live reply in a main-pane thread stays unread while focus is in the side-pane thread", async ({ + page, + app, +}) => { + await page.clock.install(); + await holdReadingFocus(page); + await open(page, app); + const roots = app.histories + .get("primary/alpha") + .filter((row) => row.content.startsWith("Thread root")); + for (const root of roots.slice(0, 2)) { + await page + .locator(`[data-channel-timeline] [data-message-id="${root.id}"]`) + .getByRole("button", { name: /^View thread:/ }) + .click(); + await expect( + page.getByRole("complementary", { name: "Thread", exact: true }), + ).toBeVisible(); + } + const x = roots[1]; + const side = page.locator("[data-panel-workspace]"); + const main = page.locator("[data-main-workspace]"); + const tabs = (host) => + host.locator('[role="tab"][data-tab-value^="thread:"]'); + const unread = (rootId) => + page.evaluate( + (rootId) => + window.fixtureRelay.snapshot().session.unread.snapshot({ + kind: "thread", + channelId: "alpha", + rootId, + }).observedCount, + rootId, + ); + // Move X's tab (the second opened) to the main pane, leaving T in the side. + const ids = await tabs(side).evaluateAll((all) => + all.map((tab) => tab.dataset.tabValue), + ); + const moving = page.locator(`[role="tab"][data-tab-value="${ids[1]}"]`); + const from = await moving.boundingBox(); + const header = await main.locator("header").first().boundingBox(); + await page.mouse.move(from.x + from.width / 2, from.y + from.height / 2); + await page.mouse.down(); + await page.mouse.move( + header.x + header.width / 2, + header.y + header.height / 2, + { steps: 8 }, + ); + await page.mouse.up(); + await expect(tabs(main)).toHaveCount(1); + await expect(tabs(side)).toHaveCount(1); + await page.clock.runFor(1000); + expect(await unread(x.id)).toBeGreaterThan(0); + + // Leave and return by a plain sidebar click. + await page.locator("button[data-channel-id=beta]").click(); + await expect( + page.getByRole("textbox", { name: "Message #Beta", exact: true }), + ).toBeVisible(); + await releaseReadingFocus(page); + await page.locator("button[data-channel-id=alpha]").click(); + await expect( + page.getByRole("textbox", { name: "Message #Alpha", exact: true }), + ).toBeVisible(); + + // Select X (reading its own selected tab is legitimate), then park focus in + // T's composer. + await tabs(main).click(); + await expect.poll(() => unread(x.id)).toBe(0); + await side + .getByRole("textbox", { name: "Reply to thread", exact: true }) + .focus(); + app.reply(x.id); + await expect(main.getByText("New peer reply", { exact: true })).toBeVisible(); + await expect.poll(() => unread(x.id)).toBeGreaterThan(0); + await page.clock.runFor(1000); + expect(await unread(x.id)).toBeGreaterThan(0); +});