Defer removal of the last ghost read marker

[gibson042]

When navigating to a room with unread messages, a green line "unread marker" appears to indicate where they start, but it quickly animates away. It would be much more convenient for that line to remain, and this PR attempts to implement just that. It also includes an initial commit to synthesize a transitionend event in browsers that don't understand the specified transition style (fixing a bug that is currently preventing their ability to collect the read markers).

Notes: Keep unread markers visible until they need to move.

I did my best to follow https://github.com/matrix-org/matrix-js-sdk/blob/master/CONTRIBUTING.md , but I'm not sure if I succeeded. I also couldn't find a way to run tests or even successfully compile, so help with that would be appreciated.

---

Here's what your changelog entry will look like:

✨ Features

• Keep unread markers visible until they need to move. (#7630). Contributed by @gibson042.

[germain-gg]

Before we get to review this, is there an issue in the tracker that would explain the problem you're trying to fix?
If not, would you be able to write a little bit about what you're trying to achieve, the approach you took and why you believe that's important

[gibson042]

@gsouquet I've updated the PR description to clarify. However, I'm not sure what tracker you're referring to... should I open an issue at https://github.com/vector-im/element-web/issues per https://github.com/matrix-org/matrix-react-sdk#github-issues ?

[SimonBrandner]

However, I'm not sure what tracker you're referring to... should I open an issue at https://github.com/vector-im/element-web/issues per matrix-org/matrix-react-sdk#github-issues ?

Yes

[andybalaam]

Hi @gibson042 - thank you for the contribution! I would say the description you have added is a helpful start to having a conversation about your change - there's probably no need to create a separate issue. However, a change like this might need to go to the Design team before being approved, so be ready for some delays and discussion...

You should be able to run the tests by typing:

yarn test

inside the matrix-react-sdk directory.

Make sure you've follow these instructions first: Setting up a dev environment

If it doesn't work, please ask for help in #element-dev:matrix.org Matrix room.

[gibson042]

Thanks @andybalaam. I was able to get successful tests in matrix-js-sdk, matrix-react-sdk in this branch, and element-web, so at least the changes are not breaking anything fundamental.

[andybalaam]

@gibson042 I've clicked the approval to build a test version of the code, so people can play with it. I've also added this as needing review from the Design team. To make it easier for them, I'd recommend adding a tiny video or screenshot of how it looks to the description. (Even better would be to do before and after videos.)

[andybalaam]

@gibson042 it is totally awesome that you are contributing, thank you. I suspect this one might take a while to progress. If you want faster gratification, pick up a small bug that doesn't make visual changes and it can be merged by a dev reasonably quickly if it's uncontroversial.

[gibson042]

Thanks very much @andybalaam. I'll see if I can get a demonstration, although that might be tricky because it depends upon a stream of messages from other people. I understand that this might take a while, and there's no rush on my side because I'm already addressing it by user script.

[gibson042]

Here's a demonstration of the user script approximation (which differs a bit from this PR, most notably in replacing the current "animate away" behavior with "shrink and turn red" so I know it's still working).

155849006-c97886ff-b07d-4f51-a5b8-05d07cbf6373-reencoded2.mp4

ffmpeg -i "input.mp4" -pix_fmt yuv420p -c:v libx264 -crf 18 -preset slow -c:a copy "reencoded.mp4"

Original video (corrupted playback in Firefox)

gh-7630.mp4

[turt2live]

this code is probably fine, though I question the cases where we end up with multiple read markers as we should be handling that better.

Pending design review before code review goes much further though.

[MadLittleMods]

I'll see if I can get a demonstration, although that might be tricky because it depends upon a stream of messages from other people.

-- #7630 (comment)

We recently introduced Cypress for end-to-end tests to the codebase. This means you can setup a list of repeatable steps to run against a real Element interface and Synapse homeserver and it will record each step along the way. This seems perfect if you want a way to demo this for real and will guarantee the behavior in the future so we don't regress.

[amshakal]

Here's a demonstration of the user script approximation (which differs a bit from this PR, most notably in replacing the current "animate away" behavior with "shrink and turn red" so I know it's still working).
gh-7630.mp4

Hi there, thanks for contributing. Unfortunately the video is broken so I am unable to see any changes you are proposing. Could you upload the video again? Or perhaps share screenshots?
:)

[gibson042]

@amshakal sure.

before room navigation

before marker removal

after marker "removal"

[amshakal]

Thanks for sharing the screenshots. We tend to use the colour red to communicate an error or an alert state. I would suggest having the green line as is but make it static till the user scrolls again. This way we'll be staying true to our design language and make sure users are aware where they should catch up from. Would that work for you?

[gibson042]

I don't really care about the color or other styling concerns, just that the marker does not vanish.

[turt2live]

@gibson042 in order for us to be able to merge this, it'll need to meet the design team's requirements, for which @amshakal is part of.

If this PR can be updated according to @amshakal's suggestions then we can take another look.

[gibson042]

@turt2live The red is something I added for demonstration purposes and is not included in this PR, so I don't think any changes are needed to the current state are needed.

matrix-react-sdk/src/components/structures/MessagePanel.tsx

Lines 589 to 595 in da6b3bd

	if (index === ghostReadMarkers.length - 1) {
	requestAnimationFrame(() => {
	node.style.width = '30%';
	node.style.opacity = '0.5';
	});
	return;
	}

[turt2live]

@amshakal I think it'll be best to test this from the deployment rather than via screenshots.

[amshakal]

I have been using the deployed version all morning, the line seems disappears after a while, unlike what was mention above/what was in the video.

I am fine with the implementation if:

1. the line is green (which has been already discussed above)
2. if the line disappears when the user interacts in the timeline - this could include actions like writing a message, reacting or scrolling.

[andybalaam]

We think this has rotted enough that we should just close it. We're not convinced it improves things, and it has no tests and many conflicts.