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

#2548, #2430 - Fix chat scroll related bugs #2590

Closed
wants to merge 3 commits into from
Closed

#2548, #2430 - Fix chat scroll related bugs #2590

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ff504e8
fix the scroll-related bug
SebinSong Feb 6, 2025
eaec7f0
revert one conditional expression
SebinSong Feb 6, 2025
2fe7f85
additional fix / cypress update
SebinSong Feb 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 40 additions & 27 deletions frontend/views/containers/chatroom/ChatMain.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

DRYing reference to a particular message DOM element here.


if (msg.pending || msg.hasFailed) continue
const offsetTop = this.$refs[msg.hash][0].$el.offsetTop
const offsetTop = msgEl.offsetTop
// const parentOffsetTop = this.$refs[msg.hash][0].$el.offsetParent.offsetTop
const height = this.$refs[msg.hash][0].$el.clientHeight
const height = msgEl.clientHeight

if (offsetTop + height <= curScrollBottom) {
const bottomMessageCreatedHeight = msg.height
const latestMessageCreatedHeight = this.currentChatRoomReadUntil?.createdHeight
Expand All @@ -188,14 +191,14 @@ const onChatScroll = function () {
return
}

if (this.ephemeral.scrolledDistance > ignorableScrollDistanceInPixel) {
if (curScrollTop > ignorableScrollDistanceInPixel) {
Comment on lines -191 to +194
Copy link
Collaborator Author

@SebinSong SebinSong Feb 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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

const scrollTopMax = this.$refs.conversation.scrollHeight - this.$refs.conversation.clientHeight
this.ephemeral.scrolledDistance = scrollTopMax - curScrollTop

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.

// Save the current scroll position per each chatroom
for (let i = 0; i < this.messages.length - 1; i++) {
for (let i = 0; i <= this.messages.length - 1; i++) {
const msg = this.messages[i]
if (msg.pending || msg.hasFailed) continue
const offsetTop = this.$refs[msg.hash][0].$el.offsetTop
const scrollMarginTop = parseFloat(window.getComputedStyle(this.$refs[msg.hash][0].$el).scrollMarginTop || 0)
if (offsetTop - scrollMarginTop > curScrollTop) {

if (offsetTop > curScrollTop) {
sbp('okTurtles.events/emit', NEW_CHATROOM_UNREAD_POSITION, {
chatRoomID: this.nonReactive.renderingChatRoomId,
messageHash: msg.hash
Expand Down Expand Up @@ -257,6 +260,7 @@ export default ({
// NOTE: messagesInitiated describes if the messages are fully re-rendered
// according to this, we could display loading/skeleton component
messagesInitiated: undefined,
scrollHashOnInitialLoad: null, // Message hash to scroll to on chatroom's initial load
replyingMessage: null,
replyingTo: null,
unprocessedEvents: []
Expand Down Expand Up @@ -544,25 +548,27 @@ export default ({

const scrollAndHighlight = (index) => {
const eleMessage = document.querySelectorAll('.c-body-conversation > .c-message')[index]
const eleTarget = document.querySelectorAll('.c-body-conversation > .c-message')[Math.max(0, index - 1)]

if (!eleTarget) { return }
if (!eleMessage) { return }

if (effect) {
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()
Comment on lines -552 to +561
Copy link
Collaborator Author

@SebinSong SebinSong Feb 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

}
}

const msgIndex = findMessageIdx(messageHash, this.messages)
if (msgIndex >= 0) {
// if the message has already been loaded in the DOM, scroll to it.
scrollAndHighlight(msgIndex)
} else {
// If the target message cannot be found in the current messages state, fetch more events
// and render the older messages.
const contractID = this.summary.chatRoomID
const limit = this.chatRoomSettings?.actionsPerPage || CHATROOM_ACTIONS_PER_PAGE
const events =
Expand Down Expand Up @@ -858,23 +864,7 @@ export default ({

if (!this.ephemeral.messagesInitiated) {
this.setStartNewMessageIndex()

// 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 })
}
}
Comment on lines -861 to -877
Copy link
Collaborator Author

@SebinSong SebinSong Feb 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

this.ephemeral.scrollHashOnInitialLoad = messageHashToScroll
}

return events.length > 0 && GIMessage.deserializeHEAD(events[0]).head.height === 0
Expand Down Expand Up @@ -1090,6 +1080,7 @@ export default ({
}, 40),
infiniteHandler ($state) {
this.ephemeral.infiniteLoading = $state

if (this.ephemeral.messagesInitiated === undefined) {
// NOTE: this infinite handler is being called once which should be ignored
// before calling the setInitMessages function
Expand All @@ -1105,6 +1096,7 @@ export default ({

try {
const completed = await this.renderMoreMessages()

if (!this.checkEventSourceConsistency(chatRoomID)) return

if (completed === true) {
Expand All @@ -1129,9 +1121,30 @@ export default ({
} else if (completed === false) {
$state.loaded()
}

if (completed !== undefined && !this.ephemeral.messagesInitiated) {
// NOTE: 'this.ephemeral.messagesInitiated' can be set true only when renderMoreMessages are successfully proceeded
this.ephemeral.messagesInitiated = true

if (this.ephemeral.scrollHashOnInitialLoad) {
const scrollingToSpecificMessage = this.$route.query?.mhash === this.ephemeral.scrollHashOnInitialLoad

this.$nextTick(() => {
this.updateScroll(
this.ephemeral.scrollHashOnInitialLoad,
scrollingToSpecificMessage // NOTE: we do want the 'c-focused' animation if there is a scroll-to-message query.
)

// NOTE: delete mhash in the query after scroll and highlight the message with mhash
if (scrollingToSpecificMessage) {
const newQuery = { ...this.$route.query }
delete newQuery.mhash
this.$router.replace({ query: newQuery })
}

this.ephemeral.scrollHashOnInitialLoad = null
})
}
}
} catch (e) {
console.error('ChatMain infiniteHandler() error:', e)
Expand Down
2 changes: 1 addition & 1 deletion test/cypress/integration/group-chat-message.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -440,7 +440,7 @@ describe('Send/edit/remove/reply/pin/unpin messages & add/remove reactions insid
cy.giSwitchChannel(CHATROOM_GENERAL_NAME)

cy.getByDT('conversationWrapper').within(() => {
cy.contains('Sending three profile pictures which are designed by Apple. Cute, right?').should('be.visible')
cy.contains('Sending two files; one is image, and the other is JSON file.').should('be.visible')
})

cy.getByDT('messageInputWrapper').within(() => {
Expand Down