#2548, #2430 - Fix chat scroll related bugs #2590
Conversation
group-income
|
||||||||||||||||||||||||||||
| Project |
group-income
|
| Branch Review |
sebin/task/#2548-chat-scrolling-bug
|
| Run status |
|
| Run duration | 10m 50s |
| Commit |
|
| Committer | Sebin Song |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
10
|
Skipped
|
0
|
Passing
|
112
|
| View all changes introduced in this branch ↗︎ | |
SebinSong
changed the title
| @@ -166,10 +166,13 @@ const onChatScroll = function () { | |||
|
|
|||
| for (let i = this.messages.length - 1; i >= 0; i--) { | |||
| const msg = this.messages[i] | |||
| const msgEl = this.$refs[msg.hash][0].$el | |||
There was a problem hiding this comment.
DRYing reference to a particular message DOM element here.
| eleTarget.scrollIntoView({ behavior: this.isReducedMotionMode ? 'instant' : 'smooth' }) | ||
| eleMessage.scrollIntoView({ behavior: this.isReducedMotionMode ? 'instant' : 'smooth' }) | ||
| eleMessage.classList.add('c-focused') | ||
| setTimeout(() => { | ||
| eleMessage.classList.remove('c-focused') | ||
| }, 1500) | ||
| } else { | ||
| eleTarget.scrollIntoView() | ||
| eleMessage.scrollIntoView() |
There was a problem hiding this comment.
This part is the fix for #2548. (It's unclear what the original intention here was but) eleTarget here is a message placed above the actual target message and eleTarget.scrollIntoView(...) leads to not fully scrolling to the target message. (especially when that previous message has associated file-attachments and takes up large space in the DOM). So made a fix here.
|
|
||
| // NOTE: we do want the 'c-focused' animation if there is a message-scroll query. | ||
| if (events.length) { | ||
| // NOTE: if 'messageHashToScroll' was not there in the messages of the contract state | ||
| // we need to retrieve more events, and render to scroll to that message | ||
| this.updateScroll(messageHashToScroll, Boolean(mhash)) | ||
| } else { | ||
| // NOTE: we need to scroll to the message first in order to no more infiniteHandler is called | ||
| await this.updateScroll(messageHashToScroll, Boolean(mhash)) | ||
|
|
||
| if (mhash) { | ||
| // NOTE: delete mhash in the query after scroll and highlight the message with mhash | ||
| const newQuery = { ...this.$route.query } | ||
| delete newQuery.mhash | ||
| this.$router.replace({ query: newQuery }) | ||
| } | ||
| } |
There was a problem hiding this comment.
This logic here handles scrolling to a particular message after message states are initialized. And moving this logic to below this.ephemeral.messagesInitiated = true is the fix for issue #2430 .
I noticed infiniteHandler() and renderMoreMessages() methods are called multiple times throughout the entire chatroom initialization process (which honestly I have not been able to figure out yet exactly from where and why..) and observed that executing scrolling logic here looked premature most of the time.
So moved this logic and it fixed the mhash issue.
SebinSong
changed the title
| if (this.ephemeral.scrolledDistance > ignorableScrollDistanceInPixel) { | ||
| if (curScrollTop > ignorableScrollDistanceInPixel) { |
There was a problem hiding this comment.
This is part of the fix for #2548 too.
Using this.ephemeral.scrolledDistance here is apparently wrong. If we see L165 below:
group-income/frontend/views/containers/chatroom/ChatMain.vue
Lines 164 to 165 in 5b89d70
ephemeral.scrolledDistance isn't actually calculating scrollED distance but rather measuring srollABLE distance. This value gets closer to 0 as the chatroom scrolls closer to the bottom(the latest message) and as a result the logic in below if block (which saves the current scroll position of the chatroom)
if (curScrollTop > ignorableScrollDistanceInPixel) { ... }ends up not getting executed for a couple of most recent messages, in some cases.
So replaced it with currScrollTop (which is the correct variable that calculates currently scrollED distance)
I didn't go ahead and update this.ephemeral.scrolledDistance itself in this PR because, this might cause unexpected bugs in other places where this is currently used.
There was a problem hiding this comment.
Great work @SebinSong!
I can confirm that #2430 seems to be closed, however #2548 still seems to be an issue, and there are two problems:
Problem 1: Scrolling to end still doesn't work at certain viewport sizes
When the browser window is a certain height/width, it still doesn't seem to work:
broken.compressed.mov
Problem 2: Remembering scroll position now completely broken
The behavior we want is that if we're scrolled near the end, then when we switch back to that channel, it takes us all the way to the very end.
However, if we've scrolled kind of far up, then upon switching channels back, we should scroll to where we were.
With this PR now it seems that that behavior is completely broken, as it no longer remembers scroll position in my testing in FFDev.
|
@taoeffect I will probably spend some more time on it in the future and send a separate PR Or, someone else could work on it(Tbh, I think whoever worked on implementing |
closes #2548
closes #2430
[Fix for issue 2548]
scroll-issue-fix-1.mp4
[Fix for issue 2430]
scroll-issue-fix-2.mp4
Even though I still have not demystified 100% of How message states in chatroom are managed / entire steps of how message states are initialized, I tried my best to ensure these updates don't end up breaking any existing behavior the chatroom has. Thanks.