Mark room as read when escape is pressed

[aaronraimist]

Fixes element-hq/element-web#12537

[t3chguy]

Can this get matching documentation in the keyboard shortcuts dialog?

[aaronraimist]

I guess this would fit best in the Navigation section? There isn't really a section for the room view.

There is already an Esc shortcut in that section. Do you want me to add another one or do you want me to add it to the description of the existing shortcut?

[t3chguy]

For now I guess just add it to the description of the existing shortcut, also do ensure that this doesn't wrongly trigger when one of those shortcuts is caught. E.g when closing a context menu I don't want my timeline to reset to bottom.

Once we have a more complete inventory of shortcuts the categorization may change up.

Thanks

[aaronraimist]

Yeah it does seem to trigger multiple things. I don't know how to prevent that.

[t3chguy]

So the issue here is RoomView listens on a native event handler not a React one so it'll trigger regardless unless you tie up every other Key.ESCAPE path with ev.nativeEvent.stopImmediatePropagation(); which seems ugly. I would definitely not register any "ambiguous" shortcuts (unlike e.g Cmd+E) in RoomView's onKeyDown because of this.

Something like this seems to work, if you could also put a comment in RoomView's onKeyDown to point out its a global document native handler that'd be swell to avoid this issue in future.

Index: src/components/structures/RoomView.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- src/components/structures/RoomView.js	(revision 76104edeca3b8ba37d4a24903fa1e0193dad56c2)
+++ src/components/structures/RoomView.js	(date 1585517162034)
@@ -560,12 +560,6 @@
                     handled = true;
                 }
                 break;
-
-            case Key.ESCAPE:
-                this._messagePanel.forgetReadMarker();
-                this.jumpToLiveTimeline();
-                handled = true;
-                break;
         }
 
         if (handled) {
Index: src/components/structures/LoggedInView.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- src/components/structures/LoggedInView.js	(revision 76104edeca3b8ba37d4a24903fa1e0193dad56c2)
+++ src/components/structures/LoggedInView.js	(date 1585517123393)
@@ -406,6 +406,16 @@
                     });
                     handled = true;
                 }
+                break;
+
+            case Key.ESCAPE:
+                if (!hasModifier && this._roomView.current) {
+                    this._roomView.current._messagePanel.forgetReadMarker();
+                    this._roomView.current.jumpToLiveTimeline();
+                    handled = true;
+                }
+                break;
         }
 
         if (handled) {

[t3chguy]

Equally you could create an onKeyDown in RoomView which is handled by React which would probably be cleaner in terms of keeping related code together 🤷‍♂

[t3chguy]

Hey @aaronraimist are you still interested in taking this on?

[aaronraimist]

Yes but I'd like to do it the second way and I don't honestly know how to do that. Feel free to finish this up if you like.

[t3chguy]

Any reason this makes Esc jump to bottom too, this seems unlike Discord and Slack at least.

[aaronraimist]

Mainly because element-hq/element-web#12537 asked for it. Discord does scroll to the bottom when pressing Esc.

[t3chguy]

Pushed my change up to your branch, now back up for review, but product/design/someone in control needs to decide if we want it to jump to bottom too or not, also still needs documenting in the Keyboard Shortcuts dialog.

[nadonomy]

Does this PR also scroll down the timeline as described in element-hq/element-web#12537?

[t3chguy]

Currently, yes. Like Discord, unlike Slack. Made the desired behaviour a product/design decision.

[valynor]

Just heard about this upcoming feature, I would like to suggest:
first ESC press marks read, second ESC press scrolls to bottom

Best of both worlds. :-)

[nadonomy]

This LGTM.

@valynor I'd suggest filing a new issue for your suggestion as we'll want to merge this PR as is to avoid bit rot.

[turt2live]

Thanks for taking a look at this!

[t3chguy]

also still needs documenting in the Keyboard Shortcuts dialog.

:((

[turt2live]

That should have been a request changes then :(