Skip to content

Conversation

@PVince81
Copy link
Member

For the initial scroll to work, the first call to getOldMessages must
fetch messages beyond the read marker itself. Otherwise the marker will
appear at the bottom of the screen as there aren't any new messages yet
until lookForNewMessages runs.

Fixes #5872

Tests with conversation that has a lot of new unread messages:

  • marker at top of viewport when switching to conversation after an initial page load
  • marker at top of viewport when switching back to a conversation after having been in the conversation before

@PVince81
Copy link
Member Author

with a bit of luck this change will also make conversation switching feel even faster because now the new unread messages are loaded right away in the first call instead of waiting for the second one.
it also means one less server call: instead of "getOldMessages", "lookForNewMessages->returns directly", "lookForNewMessages->long poll" it becomes "getOldMessages", "lookForNewMessages->long poll"

@nickvergessen
Copy link
Member

PRs against master are now "23"

@nickvergessen
Copy link
Member

/backport to stable22

@PVince81
Copy link
Member Author

temp test plan while still debugging, I've pushed a WIP with log statements.

  1. Create two conversations "first" and "second" as Alice and invite Bob in both
  2. Open converstion "second" as Alice
  3. Open a second browser for Bob
  4. As Bob, open "first"
  5. Quickly type messages: "---", 1, 2, 3, 4, and count up to 20 or more, one number per message
  6. As Alice while still on "second", refresh the page
  7. As Alice, open "first": this tests the initial load and scroll
  8. Wait for unread marker to appear above the "---" message: the viewport should be scrolled so that the marker is visible
  9. As Alice, Switch to "second" again
  10. As Bob, type another set of messages "---", 1, 2, 3, 4, ... 20
  11. As Alice, wait for the conversation list to refresh and display the unread messages counter
  12. As Alice, switch to "first" again: this tests the scroll when we already had old messages in the store
  13. Wait for unread marker to appear above the "---" message: the viewport should be scrolled so that the marker is visible
  14. Scroll down a tiny bit but not to the bottom
  15. Create and switch to a new tab in Alice's browser to trigger the blur/focus event, wait for two seconds
  16. Go back to the former tab and wait two seconds
  17. Read marker should have moved down to the top of the viewport a few messages further down from the previous marker position: this tests the read marker movement when switching away
  18. Scroll down to bottom
  19. Go back to the empty second tab and wait two seconds
  20. Go back to the former tab from Alice and wait two seconds
  21. Read marker should have disappeared, the conversation is marked as read: this tests read marker disappearance when scrolled to bottom

@PVince81
Copy link
Member Author

PVince81 commented Jun 25, 2021

The new approach seems to work but has a tiny glitch: the initial conversation load has the read marker at the bottom and it stays there until lookForNewMessages finishes. I think we had that already in previous iterations and might not be that bad.
If bad we could either try to:

  • add a loading spinner below
  • or add a spinner on the whole viewport
  • and/or smooth scroll to the new marker position after its update

This delay might be annoying if someone scrolls up manually while the code is between getOldMessages and lookForNewMessages, because then it will force-scroll back to the read marker. In theory we could detect those scrolls and cancel the automatic scroll then. Seems a bit overcomplicated though.

@PVince81 PVince81 force-pushed the bugfix/5872/fix-unread-messages-initial-scroll branch from 7d7a9aa to ecbcf2c Compare June 25, 2021 12:52
Scroll at the right moments to make both the initial scroll and the one
when coming back to an already loaded conversation work.

Signed-off-by: Vincent Petry <[email protected]>
@PVince81 PVince81 force-pushed the bugfix/5872/fix-unread-messages-initial-scroll branch from ecbcf2c to 6ebddc1 Compare June 25, 2021 15:08
@PVince81 PVince81 merged commit 6d831c0 into master Jun 25, 2021
@PVince81 PVince81 deleted the bugfix/5872/fix-unread-messages-initial-scroll branch June 25, 2021 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unread messages marker always at bottom of viewport when initially switching

3 participants