Clicking jump to bottom resets room hash

[germain-gg]

Fixes element-hq/element-web#2903

Clicking a matrix.to link jumps a user up to a message sent earlier in the timeline it also transitions the hash url

• /#/room/{room-id} => /#/room/{room-id}/{event-id}

When clicking the "jump to bottom" button the event-id fragment remained in the hash. Transforming that a button to an anchor and making it reset the hash to its original state solved the problem, and permalinks can now be clicked as many time as one likes

[turt2live]

This looks like a valuable thing to have, and is a common case, though I'm not sure it's fixing element-hq/element-web#2903 in its entirety. We should probably intercept the click on the permalink and manually do the navigation to avoid relying on browser behaviour if at all possible, or possibly take a look at our dispatcher flow to ensure it's able to send prop updates to the RoomView and such.

I'm happy for this to land as it's own feature, though.

[turt2live]

and of course I failed to even mention the alternate case in my comment, sorry: the user can also scroll down without using the button (which is what I do when the message isn't that far down).

[germain-gg]

We should probably intercept the click on the permalink and manually do the navigation to avoid relying on browser behaviour if at all possible, or possibly take a look at our dispatcher flow to ensure it's able to send prop updates to the RoomView and such.

I try in general to favour browser behaviour over JavaScript as they are more resilient and often more accessible. From what I gathered the hash change is picked up in vector-im/element.web app.tsx#routeUrl which will then propagate the correct props to update the RoomView.
If you sense that this is missing a step let me know I'd love to understand and will update accordingly

the user can also scroll down without using the button (which is what I do when the message isn't that far down).

Thank you for sharing that, I failed to identify that use case. That makes me wonder whether we should reset the eventId from the hash as soon as target event is displayed

[turt2live]

Resetting the hash as soon as the event is displayed would also work.

The built-in browser history is something we do offer as a feature as well - does this cause a trap for users who try the back button?

[turt2live]

This is getting a bit stringy, sorry. I really think it'd be best to intercept the <a onClick> instead - the next thing to consider is PgUp/Down keys, arrow key navigation, screen readers, etc. It all gets a bit messy, unfortunately.

[germain-gg]

I'm still a bit confused as to why we should favour an onClick for navigation compared to a href that would get intercepted by the app.tsx#routeUrl? Will use the dispatcher instead but for my own learning It'd be great if I could get an understanding of the problem you're trying to avoid and the difference you see between the two

the next thing to consider is PgUp/Down keys, arrow key navigation, screen readers, etc. It all gets a bit messy, unfortunately.

I do intercept all those interactions with the newly introduced onUserScroll function that gets passed to the ScrollPanel component.
That should cover all grounds unless you can think of any other scenarios?

[turt2live]

Intercepting the onClick means we can avoid a ton of the edge cases that come with scrolling. For instance, there's three bugs that I can see with this approach:

1. The jump-to-bottom button doesn't work if you scrolled up instead of visited a permalink.
2. Scrolling down in a room causes a screen flicker as the router tries to update the view. This also happens with the jump button, but is less obvious.
3. Permalinks within the same room are no longer smooth.

I haven't tested accessibility tooling, but this sort of jumping/flickering is quite harsh on screen readers and the like. Intercepting the onClick means we can direct the browser where it needs to go without having to rely on it doing the right thing, and I believe we can force it to go to a fragment even if it is the same fragment.

Using onClick is less pretty, but it also solves the problem at hand more completely I believe.

[turt2live]

see above

[germain-gg]

Sorry that pull request completely fell off my radar.

I am using <a onClick> and am dispatching a view_room action. The Page Up/Down and mouse wheel actions are also sorted

Could you give this another review. We should be able to move this forward!

[turt2live]

Oh no, this has been open long enough for it to land in the wasteland that is the normally untouched part of my review requests :( This is the sort of thing I don't mind being poked about to review sooner.

---

My concern is mostly that we shouldn't have to listen for keyboard navigation, scrolling, etc to reset the hash manually: this is a path where we end up having to support the billion different input methods for accessing the app, and there will always be edge cases. Instead of resetting the hash, we can intercept the onClick manually and do various scrollIntoView stuff to 'trick' the browser into doing the right thing, mostly because the browser masks all the different input methods behind onClick and would be more able to navigate the user than we are.

[ara4n]

This feels tantalisingly close to fixing this chronic bug. Can we resolve the impasse on whether to onClick or not? I could try to ctxt switch in, but @jryans may be better positioned?

[germain-gg]

Apologies, this has admitedly fell off my radar

I have spent a bit of time trying to find an alternative approach this morning and it is slightly tricky. The two flags eventId and highlightedEventId are what drive whether the timeline should be reinitialised causing the jump to the correct event.
The key to solving that issue is in my opinion to reset highlightedEventId and I see two ways of achieving that without overhauling the timeline component

• Adding an onLoad callback to TimelinePanel that will reset highlightedEventId once the timeline has been loaded. This comes with a drawback, the selected event is not highlighted with that green line anymore
• Keeping the flag but resetting it when we receive a hint from the user that their attention has shifted to something else (a change in scroll position?)

I believe I moved away from the initial onClick approach. Not sure if you feel like any of the two options are viable? If not I'd appreciate ideas for alternate implementations

[turt2live]

so we talked through this, and this does seem like the best solution we have for now. If the intersection observer stuff ends up working out, we might want to use that, but otherwise this is good to go.

[germain-gg]

After playing around a bit with the IntersectionObserver I didn't notice any considerable changes and the complexity shoot up quite high.
It looks like we're going to have this implementation and we'll see whether we need to add some more input to signal that the attention of the user is moved away from a specific event