Skip to content

Commit 5c9968c

Browse files
committed
fix(popup): preserve focus after marking notifications read
Move focus to the next available notification open target after marking one as read. Fall back to the previous notification and cover the behavior with renderer tests.
1 parent 92579ff commit 5c9968c

2 files changed

Lines changed: 64 additions & 0 deletions

File tree

src/popup/notification-renderer.js

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -275,6 +275,30 @@ function createNotificationItem(notif, repoHeader, repoFullName) {
275275
li.dataset.id = notif.id;
276276
li.dataset.repo = repoFullName;
277277

278+
function getNotificationOpenTarget(item) {
279+
return item?.querySelector(".notification-open-target") || null;
280+
}
281+
282+
function getFocusTargetAfterRemoval() {
283+
let sibling = li.nextElementSibling;
284+
while (sibling) {
285+
if (sibling.matches(".notification-item")) {
286+
return getNotificationOpenTarget(sibling);
287+
}
288+
sibling = sibling.nextElementSibling;
289+
}
290+
291+
sibling = li.previousElementSibling;
292+
while (sibling) {
293+
if (sibling.matches(".notification-item")) {
294+
return getNotificationOpenTarget(sibling);
295+
}
296+
sibling = sibling.previousElementSibling;
297+
}
298+
299+
return null;
300+
}
301+
278302
async function openNotification() {
279303
await sendMessage(MESSAGE_TYPES.OPEN_NOTIFICATION, { notificationId: notif.id });
280304
window.close();
@@ -478,6 +502,8 @@ function createNotificationItem(notif, repoHeader, repoFullName) {
478502
void li.offsetHeight;
479503
li.classList.add("fade-out");
480504

505+
const nextFocusTarget = getFocusTargetAfterRemoval();
506+
481507
// Restore the notification item to its original state on failure
482508
function restoreItem() {
483509
li.classList.remove("marking-read", "fade-out");
@@ -520,6 +546,8 @@ function createNotificationItem(notif, repoHeader, repoFullName) {
520546
emptyState.hidden = false;
521547
markAllBtn.disabled = true;
522548
}
549+
550+
nextFocusTarget?.focus({ preventScroll: true });
523551
} else {
524552
// Failure: restore item
525553
restoreItem();

tests/notification-renderer-mark-read.test.js

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -186,4 +186,40 @@ describe("mark single notification as read — repo header count", () => {
186186
// Notification should still be present
187187
expect(notificationsList.querySelector('.notification-item[data-id="1"]')).not.toBeNull();
188188
});
189+
190+
it("moves focus to the next notification open target after marking one as read", async () => {
191+
renderNotifications([makeNotif(1), makeNotif(2)]);
192+
193+
sendMessage.mockResolvedValueOnce({ success: true });
194+
const btn = notificationsList.querySelector('.notification-item[data-id="1"] .btn-mark-read');
195+
btn.focus();
196+
btn.click();
197+
198+
await vi.waitFor(() => {
199+
expect(notificationsList.querySelector('.notification-item[data-id="1"]')).toBeNull();
200+
});
201+
202+
const nextOpenTarget = notificationsList.querySelector(
203+
'.notification-item[data-id="2"] .notification-open-target',
204+
);
205+
expect(document.activeElement).toBe(nextOpenTarget);
206+
});
207+
208+
it("falls back to the previous notification open target when the last item is removed", async () => {
209+
renderNotifications([makeNotif(1), makeNotif(2)]);
210+
211+
sendMessage.mockResolvedValueOnce({ success: true });
212+
const btn = notificationsList.querySelector('.notification-item[data-id="2"] .btn-mark-read');
213+
btn.focus();
214+
btn.click();
215+
216+
await vi.waitFor(() => {
217+
expect(notificationsList.querySelector('.notification-item[data-id="2"]')).toBeNull();
218+
});
219+
220+
const previousOpenTarget = notificationsList.querySelector(
221+
'.notification-item[data-id="1"] .notification-open-target',
222+
);
223+
expect(document.activeElement).toBe(previousOpenTarget);
224+
});
189225
});

0 commit comments

Comments
 (0)