Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add unread message marker in the chat view #3825

Merged
merged 17 commits into from Mar 9, 2021
Merged

Add unread message marker in the chat view #3825

merged 17 commits into from Mar 9, 2021

Conversation

nickvergessen
Copy link
Member

@nickvergessen nickvergessen commented Jun 18, 2020 •
edited by PVince81

Fix #1788
Fixes #1164

@nickvergessen nickvergessen added this to the 💚 Next Major (20) milestone Jun 18, 2020
@nickvergessen nickvergessen mentioned this pull request Aug 19, 2020
Closed
10 tasks
@nickvergessen nickvergessen mentioned this pull request Aug 27, 2020
Merged
4 tasks
@PVince81
Copy link
Member

what's missing ? from reading the code it seems like it could already work.

let me know if I should take over

@nickvergessen
Copy link
Member Author

Handling the update of the value is still missing. once the next bunch of messages is loaded and the room list refreshes, the read marker will be on the newest message

@PVince81 PVince81 self-assigned this Feb 10, 2021
@PVince81
Copy link
Member

rebased

@PVince81 PVince81 force-pushed the feature/1788/unread-message-marker branch from 79f897c to 4bcb0f8 Compare February 10, 2021 09:54
@PVince81
Copy link
Member

Handling the update of the value is still missing. once the next bunch of messages is loaded and the room list refreshes, the read marker will be on the newest message

Seems there's more to it.
The original ticket mentions how the read marker would move when scrolling up and down based on multiple conditions. The latter is challenging as it requires detecting visible/invisible elements. Not sure if you intended to have this in the scope of this PR ?

@PVince81
Copy link
Member

PVince81 commented Feb 10, 2021 •
edited

I had a quick look at ways for detecting the element visible at the top:

  • https://github.com/AkatQuas/vue-visibility-sensor
    • cons: requires adding this to every Message DOM component, which could trigger a lot of logic when we have lots of messages (this problem would go away if we had virtual scrolling)
  • document.elementFromPoint https://developer.mozilla.org/en-US/docs/Web/API/DocumentOrShadowRoot/elementFromPoint
    • pros: supported by all browsers
    • cons: marked as experimental, so not sure
    • cons: need some calculation gymnastics to find a good coordinate to query to make sure that we'll land on a message element
    • cons: assumes no other element is visually obstructing the top message (like the top bar)
  • iterate over all message elements and find the one which offsetTop is closest to the border
    • cons: can be expensive with many messages, might need to use some algo for shortcutting (like bisecting)

@PVince81
Copy link
Member

PVince81 commented Feb 10, 2021 •
edited

I'll go with elementFromPoint and getBoundingClientRect which are said to work on Edge at least.
I have something working locally.

Next up: tracking whether the unread marker was actually visible on screen or notaccount

Then we only update the unread marker position if the marker was seen at least one and the detected top element is below the marker.

Extra: assume that unread marker was not seen if window was not focused when it happened

@nickvergessen
Copy link
Member Author

We have vue-observe-visibility already, can it do the job?

@PVince81
Copy link
Member

We have vue-observe-visibility already, can it do the job?

Yes. But I won't set it on all the message elements as observing them all won't scale.
It will be useful for tracking the visibility of the read marker at least.

@PVince81
Copy link
Member

PVince81 commented Feb 10, 2021 •
edited

I've pushed some experiment which currently works for me in Chromium.

TODOs:

  • fix left sidebar to not "mark as read" immediately when clicking, and the message count bubble

  • remove console logs

  • double check if we should show the marker at the top or at the bottom of message, might need to "shift by one"

  • test: more testing with various scrolling scenarios

  • test: double check if works with multi-line messages, the latter need to be fully visible to be considered as read

  • test: with Edge, mainly because of the getBoundingClientRect just because, who knows...

  • check more todos from original ticket with special buttons to scroll to mention, etc (maybe for another PR)

@nickvergessen
Copy link
Member Author

Conflicting files
src/components/MessagesList/MessagesGroup/Message/Message.vue

@nickvergessen
Copy link
Member Author

It's a bit weird to have the unread marker start showing for own items. Basically when you have read everything and reload, it shows on the next one instead of not at all:
Peek 2021-03-08 13-41

nickvergessen and others added 16 commits March 8, 2021 15:34
@nickvergessen @PVince81
Add unread message marker in the chat view
abe329c 
Signed-off-by: Joas Schilling <coding@schilljs.com>
@PVince81
Update last read message marker on scroll
c4e739c 
Moved marker behavior from server to client by passing setReadMarker=0
to queries.

Added scroll handler that detects whether the read marker was seen and
scrolled past, if yes it will set the read marker to the first message
visible at the top.

Keep the marker when the document is not focussed.

When opening a conversation, scrolls to the read marker.

The lastMessage attribute of a conversation is now updated earlier to
make sure the read marker can update properly when there are new
messages appended.

Instead of waiting for the next refresh cycle, this fix immediately
updates lastMessage and lastReadMessage after posting a message.

Signed-off-by: Vincent Petry <vincent@nextcloud.com>
@PVince81
Don't mark conversation as read any more on click
6929de8 
When joining a conversation, don't mark it as read any more.
The only moment will be when scrolling down to the bottom of a
conversation and when clearing the read marker, in which case we
immediately update the store.

Signed-off-by: Vincent Petry <vincent@nextcloud.com>
@PVince81
Properly clear when focussed and sticky
f24ac54 
Signed-off-by: Vincent Petry <vincent@nextcloud.com>
@PVince81
Read marker tweaks
4d5ad40 
Adjust design a bit of the marker.

Debounce read marker processor on focus to delay it a bit.

Signed-off-by: Vincent Petry <vincent@nextcloud.com>
@PVince81
Scroll to unread when reopening sidebar with chat
3c6d5bc 
Signed-off-by: Vincent Petry <vincent@nextcloud.com>
@PVince81
Tweak read marker behavior
b99adb6 
When no scrolling is possible, add delay before clearing.

Small related code cleanup.

Signed-off-by: Vincent Petry <vincent@nextcloud.com>
@PVince81
Read marker is now a long bar
31b3c3e 
Signed-off-by: Vincent Petry <vincent@nextcloud.com>
@PVince81
Use DOM attributes to access message/seen information
3dbd551 
Signed-off-by: Vincent Petry <vincent@nextcloud.com>
@PVince81
Fix immediate update of read marker
30285b0 
Use int instead of string for lastReadMessage update.

Signed-off-by: Vincent Petry <vincent@nextcloud.com>
@PVince81
New menu item mark conversation as read
9374c25 
Signed-off-by: Vincent Petry <vincent@nextcloud.com>
@PVince81
New menu item mark message as unread
22eb262 
This will move the unread marker to a specific position.

Signed-off-by: Vincent Petry <vincent@nextcloud.com>
@PVince81
Move unread messages marker out of the hover element
f8ca906 
Signed-off-by: Vincent Petry <vincent@nextcloud.com>
@PVince81
Decouple visible unread marker from its actual position
0298bf5 
To avoid jumping around when updating the lastReadMessage property, its
last value is now cached in visualLastReadMessageId in the
messagesStore. The marker always keeps its position until the user
focusses away then back. In the latter case the marker position is
refreshed based on the last backend/store state.

Signed-off-by: Vincent Petry <vincent@nextcloud.com>
@PVince81
Remove delay for unread marker on focus
579fbdc 
Signed-off-by: Vincent Petry <vincent@nextcloud.com>
@PVince81
Mark as unread now applied to previous message id
7d4d6fb 
Added props for previous and next message id in MessagesGroup and
Message components.

Now using the previous message id when marking as unread.

Since now the next message id is available on all message components,
the logic in MessagesList that iterates over the next messages is now
using the next message id attribute instead of handling the DOM tree on
its own.

Signed-off-by: Vincent Petry <vincent@nextcloud.com>
@PVince81
Copy link
Member

PVince81 commented Mar 8, 2021

It's a bit weird to have the unread marker start showing for own items. Basically when you have read everything and reload, it shows on the next one instead of not at all:
Peek 2021-03-08 13-41

okay, it seems I forgot to also force-update the marker when posting...

@PVince81
Update read marker UI when posting + tweaks
c26d5e3 
When posting new messages, the read marker UI will also be updated.
Fixed read marker update when scrolled to bottom.
Fixed warnings related to temporary message id being a string.

Signed-off-by: Vincent Petry <vincent@nextcloud.com>
@PVince81 PVince81 force-pushed the feature/1788/unread-message-marker branch from fde7613 to c26d5e3 Compare March 8, 2021 15:23
@PVince81
Copy link
Member

PVince81 commented Mar 8, 2021

Fixed case where posting new messages: the read marker is now updated.

Also fixed issue with marker update when being scrolled to bottom, blur then focus again.

marcoambrosini
marcoambrosini approved these changes Mar 9, 2021
View reviewed changes
Copy link
Member

@marcoambrosini marcoambrosini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👌

@PVince81 PVince81 merged commit ec0d799 into master Mar 9, 2021
@PVince81 PVince81 deleted the feature/1788/unread-message-marker branch March 9, 2021 15:49
@nickvergessen nickvergessen added this to In progress in Talk 12 via automation Apr 27, 2021
@nickvergessen nickvergessen moved this from In progress to Done - Split 1 in Talk 12 Apr 27, 2021
@PVince81 PVince81 mentioned this pull request Apr 28, 2021
Merged
@nickvergessen nickvergessen mentioned this pull request Jul 1, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PVince81 PVince81 PVince81 left review comments

@marcoambrosini marcoambrosini marcoambrosini approved these changes

@danxuliu danxuliu Awaiting requested review from danxuliu

Assignees

@PVince81 PVince81

@marcoambrosini marcoambrosini

Labels
3. to review enhancement feature: chat 💬 Chat and system messages feature: frontend 🖌️ "Web UI" client
Projects
No open projects
Talk 12
Done - Split 1
Milestone
v12.0.0-rc.1
Development

Successfully merging this pull request may close these issues.

Indicator for read position Chat flow experience
3 participants
@nickvergessen @PVince81 @marcoambrosini