Add shortcut to mark current room as read

[davidegirardi]

I sometime quickly jump through rooms in a space and dismiss the read marker. Since the advent of threads, this is not enough to clear the unread state (see this discussion) so we have to use the "Mark as read" entry in the room hamburger menu.

After discussing a bit below, this PR adds an extra functionality to the Escape shortcut in the timeline. If you scroll up or there is a read marker, it behaves as it did before (scroll to bottom and dismiss the read marker), otherwise it marks the room as read.

Original PR text for context

I sometime quickly jump through rooms in a space and dismiss the read marker. Since the advent of threads, this is not enough to clear the unread state (see [this discussion](https://github.com/vector-im/element-web/issues/25437)) so we have to use the "Mark as read" entry in the room hamburger menu. This PR adds the `Alt+Escape` shortcut for it.

Checklist

• [ ] Tests written for new code (and old code if feasible)
• [x] Linter and other CI checks pass
• [x] Sign-off given on the changes (see CONTRIBUTING.md)

---

Here's what your changelog entry will look like:

✨ Features

• Add shortcut to mark current room as read (#11054). Contributed by @davidegirardi.

[t3chguy]

Looks like this shortcut may already be used by browsers, which we shouldn't override https://defkey.com/google-chrome-shortcuts#3071

[davidegirardi]

Looks like this shortcut may already be used by browsers

It is and it has not been an issue as long as the focus is on the DOM elements of the room (see above) but I will try to find something else.

[davidegirardi]

Looks like Sonarcloud has some valid changes it's asking from you, and if you can get some decent test coverage that would be awesome. (It might be effectively impossible, but definitely worth trying.)

clearRoomNotification could return:

• a promise: client.sendReadReceipt(lastEvent, receiptType, true);
• {}
• undefined

I don't know how to manage all these cases with .then() and .end().

[andybalaam]

clearRoomNotification could return:

I'm fairly sure you just want await clearRoomNotification(... (i.e. literally just add await).

[davidegirardi]

That's what I tried first but onReactKeyDown is synchronous.

[andybalaam]

That's what I tried first but onReactKeyDown is synchronous.

As far I as I can see there should be no harm in making onReactKeyDown async i.e.:

$ git diff
diff --git a/src/components/structures/RoomView.tsx b/src/components/structures/RoomView.tsx
index eb1551cabf..493f7965bb 100644
--- a/src/components/structures/RoomView.tsx
+++ b/src/components/structures/RoomView.tsx
@@ -1028,7 +1028,7 @@ export class RoomView extends React.Component<IRoomProps, IRoomState> {
         }
     };
 
-    private onReactKeyDown = (ev: React.KeyboardEvent): void => {
+    private onReactKeyDown = async (ev: React.KeyboardEvent): Promise<void> => {
         let handled = false;
 
         const action = getKeyBindingsManager().getRoomAction(ev);

but you'd need to test that it still works...

[Johennes]

Looks like this shortcut may already be used by browsers

It is and it has not been an issue as long as the focus is on the DOM elements of the room (see above) but I will try to find something else.

I wonder if we could cleverly overwrite the ESC shortcut? (This is what Slack appears to use as well.)

[davidegirardi]

We already use ESC to jump to the bottom of the timeline if you are scrolling up and dismiss the read marker, which does not affect the thread read status. Are you thinking about something like double pressing ESC?

[Johennes]

We already use ESC to jump to the bottom of the timeline if you are scrolling up and dismiss the read marker, which does not affect the thread read status. Are you thinking about something like double pressing ESC?

Kind of, yeah. Could we trigger mark room as read only when you're scrolled to the bottom and press ESC? Just brainstorming here really. 😅

[davidegirardi]

I gave it a go and it does work well for me. I don't know if overloading a single shortcut with context-dependent functions is OK or not for general users. I do like that very much though.

Managing the shortcut:

            case KeyBindingAction.DismissReadMarker:
                // Clear all notifications if we already dismissed the read marker
                if (this.messagePanel?.dismissedReadMarker() && this.messagePanel?.isAtBottom()) {
                    if (!!this.state.room && !!this.context.client) {
                        await clearRoomNotification(this.state.room, this.context.client);
                        handled = true;
                    }
                } else {
                    this.messagePanel?.forgetReadMarker();
                    this.jumpToLiveTimeline();
                    handled = true;
                }
                break;

And the function I added to TimelinePanel.tsx:

    /**
     * Check if the read marker is dismissed or up to date
     */
    public dismissedReadMarker = () : boolean => {
        if (!this.props.manageReadMarkers) return false;

        // Find the read receipt
        const rmId = this.getCurrentReadReceipt();

        // Is the RM already up to date?
        return (rmId === this.state.readMarkerEventId);
    };

Which solution do we like more?

[andybalaam]

This new approach is exactly the functionality I would like. I would think it's OK accessibility-wise, since it doesn't depend on tapping quickly or similar, so I'm in favour.

[davidegirardi]

I'll prepare a new PR then. I imagine we need to update the shortcut screen with a new description for "Dismiss read marker and jump to bottom" and update the traslations. Let's take the discussion in the new PR.

EDIT: I just realized it would have the same title so I'm reopening and I'll merge the new code in this branch.

[davidegirardi]

I added a unit test for the new function I wrote. Sonarcloud now complains about older parts of the codebase. Adding tests to those parts looks like something for another PR.

Moreover, I would like some feedback on the code I wrote first. I have zero prior experience in writing jest tests.

[altsalt]

Sending a soft nudge to check-in on the status of this PR? I'm guessing that since it has been about a year since the last discussion, there will be changes required. Thanks to all for their input and work on this!

[andybalaam]

Neither I nor @alunturner are the right person to review this going forward, so removing our names.

[davidegirardi]

I left this rot a bit because implementing the tests required too much effort. If I remember correctly, I had to add tests for other parts of the codebase that I didn't directly touch but did not have tests.

Anyhow, I wonder if this is useful now that the thread activity centre is a thing.

[altsalt]

Anyhow, I wonder if this is useful now that the thread activity centre is a thing.

I've wondered whether this is useful with the double-escape generally clearing read notices? The reason I found this issue was that I'm beginning to poke at https://github.com/element-hq/element-web/issues/8541 and figured finding a stable pattern to hook into would be a good first step.

Anyway, once again, thanks for the work on it everyone.

[langleyd]

Anyhow, I wonder if this is useful now that the thread activity centre is a thing.

@davidegirardi I discussed with product and I think now that thread activity has been removed from the room list, we have the activity center and also a "Mark all as read" button in the threads panel, we probably wouldn't want to make escape the keyboard shortcut. From an A11y side we can tab to the "Mark all as read" button in the threads panel, so any additional shortcut would be more for a power user use case. I think we can close this out, thanks for the contribution.