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.

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

fix(MessagesList): Fix chat jumping on reference widgets #11844

Closed
wants to merge 1 commit into from

Conversation

DorraJaouad
Copy link
Contributor

@DorraJaouad DorraJaouad commented Mar 18, 2024

☑️ Resolves

  • This will make messages load from bottom to the top, so whatever height added from above, it won't affect the bottom message position.

However, a few things to look into them soon:

  • If last message contains reference widget, only height of ( regular message height + loading spinner) will be shown when loaded for the first time -> track loading widget and scroll down once loaded OR mutation/resize observer on last message.
  • There is jumping when you open the conversation because scroll bottom takes time to be called (depending on the network), I am not sure how to solve this ( what should be done is that the visible part of the scroller should be kept at the bottom, default behavior of a scrolling div is that its visible part is at the top when adding items, so the only way to keep it is to maintain a list when switching conversations and inject the new messages in its items OR caching will solve it anyway)
  • Scrolling process is duplicated when executed, handleStartGettingMessagesPreconditions and handleScroll are the roots of each process -> can be improved.

🖌️ UI Checklist

  • add some reference widgets
  • a/ Load the conversation with the URL
    b/ Load the conversation from switching conversations

🏁 Checklist

  • 🌏 Tested with Chrome, Firefox and Safari or should not be risky to browser differences
  • 🖥️ Tested with Desktop client or should not be risky for it
  • 🖌️ Design was reviewed, approved or inspired by the design team
  • ⛑️ Tests are included or not possible
  • 📗 User documentation in https://github.com/nextcloud/documentation/tree/master/user_manual/talk has been updated or is not required

@DorraJaouad DorraJaouad added feature: chat 💬 Chat and system messages feature: frontend 🖌️ "Web UI" client labels Mar 18, 2024
@DorraJaouad DorraJaouad added this to the 💞 Next Beta (29) milestone Mar 18, 2024
@DorraJaouad DorraJaouad self-assigned this Mar 18, 2024
Copy link
Contributor

@Antreesy Antreesy left a comment

Choose a reason for hiding this comment

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

Need more time to check, how it would behave, and what it could fix. We probably should reverse groups too

:previous-message-id="group.previousMessageId"
:next-message-id="group.nextMessageId" />
</ul>
<div class="messages-list-wrapper__reversed">
Copy link
Contributor

Choose a reason for hiding this comment

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

Just apply these styles to 'scroller' class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it will put unscrollable list ( messages that do no exceed the view) at the bottom.

Copy link
Contributor Author

@DorraJaouad DorraJaouad Mar 18, 2024

Choose a reason for hiding this comment

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

We probably should reverse groups too

I would yes, it will be good but it's not critical to change atm (usually groups are short around 4 to 5 messages).
and please let's keep this for fixing reference widgets jumping only here. Otherwise, it will be huge.

Copy link
Member

Choose a reason for hiding this comment

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

I'm very unsure about this. Also seeing the internal reports with people having 10 files locked with text because they opened a talk room and the widget stealing the focus while typing a message, make me prefer an "opt out" of the widgets :-/

also turning around the loading order will not work for the context load and basically it will immediately jump to the end marking everything read?

@Antreesy
Copy link
Contributor

Antreesy commented Mar 19, 2024

All occurences and triggers of scrollTo..., handleScroll and debounceHandleScroll:

  • mounted - scrollToBottom

Was added just in case 4 years ago, not sure we still need it?

  • scrollToFocusedMessage - if URL hash contains message id (in handleStartGettingMessagesPreconditions)

check if messages are in DOM?

  • EventBus.$on('scroll-chat-to-bottom') - call started, file upload started
  • EventBus.$emit('smooth-scroll-chat-to-bottom') - message posted, click "scroll to bottom"

Keep only one scroll method (with optional smooth, ifSticky, e.t.c.)?,

  • EventBus.$emit('scroll-chat-to-bottom-if-sticky') - reactions changed
  • getNewMessages - after each polling, if sticky

replace both with "after messages array changed" in watcher?

Would it also make sense to store previous scroll value per chat (chatIdentifier), to avoid jumping when switch?

@DorraJaouad
Copy link
Contributor Author

mounted - scrollToBottom

It's wrong anyway because it can contain unread messages and it should scroll to the last read message not the bottom. It's not impacting it now as scrollToFocusedMessage is called after it.

EventBus.$on('scroll-chat-to-bottom') - call started, file upload started
EventBus.$emit('smooth-scroll-chat-to-bottom') - message posted, click "scroll to bottom"
Keep only one scroll method (with optional smooth, ifSticky, e.t.c.)?,

Yes, better

EventBus.$emit('scroll-chat-to-bottom-if-sticky') - reactions changed
getNewMessages - after each polling, if sticky
replace both with "after messages array changed" in watcher?

Yes, it will skip scrolling in vain where polling didn't add more messages, just need to check for reactions if computed prop watcher can detect its changes or not.

I would centralize all scrolling process in one method scrollToFocusedMessage and there:

  • If there is a message id from hash -> scroll to focused message id
  • If there is no message id from hash -> scroll to last read message id ( if sticky : scrollToBottom)

scrollToBottom is used independently only in the arrow down button

@nickvergessen
Copy link
Member

Let's wait for the result of nextcloud/text#5532 (comment)

@DorraJaouad
Copy link
Contributor Author

It doesn't work anymore with the new changes :')

@Antreesy Antreesy deleted the fix/noid/chat-jumping-reference-widgets branch May 31, 2024 18:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature: chat 💬 Chat and system messages feature: frontend 🖌️ "Web UI" client
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants