Page MenuHomePhabricator

Clicking a marked-as-unread notification should mark it as read
Closed, ResolvedPublic


If you mark a notification as unread, visiting the associated page no longer marks it as read. This means clicking it doesn't mark it as read either, because this relies on the primary link going to the associated page.

Event Timeline

Will be made possible after T93673 since the association between event and target page will be preserved.

The notification is wrapped with a link at the moment (<a>) to make it possible to right-click it. We do not intercept those clicks with any actions, because then we'd mess up the right-click option to open the notification.

Since that's the case, there's no way for the UI to send an API request to mark the notification as read - and there's no way for us to know in the backend that the link that is being opened comes from a notification (so it should be marked as read) rather than a direct link (which we agreed that for 'mark unread' notifications we aren't marking-as-read again for)

I'm not sure how to do this, or if this is even possible with the current implementation and the need to allow for right-clicking.

Actually, forget everything I said above. We are catching the click event anyways for logging. We can attach a state-change for marking as read in there.

Working on this now.

Change 277703 had a related patch set uploaded (by Mooeypoo):
Clicking a marked-as-unread notification should mark it as read

Change 277703 merged by jenkins-bot:
Clicking a marked-as-unread notification should mark it as read

Checked the fix in the betalabs

Clicking a marked as un-read notification and get re-directed to the page will mark that notification as read - it makes it consistent with clicking on non-read notifications and get re-directed to the proper pages.

However, visiting (e.g. via Search) pages, opening a permalink for a topic/post does not have any effect on the Notifications status. Just to make sure that it exactly what we want - Notifications are marked as read only when users 1) visit pages via clicking in Notifications 2) click on the menu option 'Mark as read'

Without delaying page navigation (which this doesn't seem to do), there is no guarantee the mark-as-read AJAX request will complete before the page navigates. And if it doesn't complete before page navigation, it won't later either.

It looks from T130853: [Regression wmf.18] Messages badge sometimes shows negative number like maybe this seems to work for certain types because the echo_target_page support is still there. But it may also sometimes work on other types. I just don't think it will reliably.

That's why sendBeacon support was introduced for EventLogging (T44815: Provide a robust way of logging events without blocking until network request completes; use sendBeacon) (sendBeacon is a way to fire-and-forget without blocking page load). We could probably do the same here for supported browsers.

This works OK, but 1) Matt is right that there's no guarantee the AJAX request will finish, and 2) the notification jumping around as it becomes read is jarring. I had an idea to fix this.

As Matt says, mark-as-read-on-click sometimes works because we have an echo_target_page row, but that row won't exist for notifications that have been marked as read before and were then marked as unread again. What we could do, though, is put ?markread=123 in the query string of the primary link URL of every notification (where 123 is the event ID), and have requests with that parameter schedule a DeferredUpdate to mark that notification as read (and also decrease the notification count as appropriate).

  1. the notification jumping around as it becomes read is jarring.

This has now been filed separately as T133975: When opening a notification it disappears before navigating away.