-
-
Notifications
You must be signed in to change notification settings - Fork 171
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
Adding a reaction makes the chat "scroll up" (then you have to scroll to see new messages) #3753
Comments
I'm noticing that our code is very similar to Signal, and Signal doesn't have this issue. |
You can debug it by adding a EDIT: Adding |
Hmm idk. Didn't work for me |
What did work is this one simple trick (scientists hate me): Basically we gotta put Edit: though idk, it's not 100% reliable. If I react to one message then it works, if I react to another it doesn't... |
Closes #3753 TODO: - [ ] The biggest issue I noticed is that when the chat is scrolled to bottom and a new message appears, it causes a "scroll" event (which doesn't feel like it should be the case, since the all we did is just changed the scroll anchor), which causes the reaction bar to disappear if it was open (see `hideReactionsBar`). Personally I think it is ok to get rid of this feature at least temporarily, because this scrolling thing is far more annoying that that feature is good Otherwise I haven't noticed things that you could call issues. This removes `scrollToBottom` JS on each new message, which is now unnecessary. However, the old code used `ifClose: true`, which would still scroll the chat if it was scrolled up less than 7 pixels up. The new implementation still has similar behavior as the new scroll anchor is not at the very bottom of the scrollable area, so the chat will still show the new message as long as the bottom edge of the previous oldest message was still visible when the new one arrived.
Closes #3753 TODO: - [ ] The biggest issue I noticed is that when the chat is scrolled to bottom and a new message appears, it causes a "scroll" event (which doesn't feel like it should be the case, since the all we did is just changed the scroll anchor), which causes the reaction bar to disappear if it was open (see `hideReactionsBar`). Otherwise I haven't noticed things that you could call issues. This removes `scrollToBottom` JS on each new message, which is now unnecessary. However, the old code used `ifClose: true`, which would still scroll the chat if it was scrolled up less than 7 pixels up. The new implementation still has similar behavior as the new scroll anchor is not at the very bottom of the scrollable area, so the chat will still show the new message as long as the bottom edge of the previous oldest message was still visible when the new one arrived.
Closes #3753 TODO: - [ ] The biggest issue I noticed is that when the chat is scrolled to bottom and a new message appears, it causes a "scroll" event (which doesn't feel like it should be the case, since the all we did is just changed the scroll anchor), which causes the reaction bar to disappear if it was open (see `hideReactionsBar`). Personally I think it is ok to get rid of this feature at least temporarily, because this scrolling thing is far more annoying that that feature is good Otherwise I haven't noticed things that you could call issues. This removes `scrollToBottom` JS on each new message, which is now unnecessary. However, the old code used `ifClose: true`, which would still scroll the chat if it was scrolled up less than 7 pixels up. The new implementation still has similar behavior as the new scroll anchor is not at the very bottom of the scrollable area, so the chat will still show the new message as long as the bottom edge of the previous oldest message was still visible when the new one arrived.
Closes #3753 This removes `scrollToBottom` JS on each new message, which is now unnecessary. However, the old code used `ifClose: true`, which would still scroll the chat if it was scrolled up less than 7 pixels up. The new implementation still has similar behavior as the new scroll anchor is not at the very bottom of the scrollable area, so the chat will still show the new message as long as the bottom edge of the previous oldest message was still visible when the new one arrived.
Closes #3753 This removes `scrollToBottom` JS on each new message, which is now unnecessary. However, the old code used `ifClose: true`, which would still scroll the chat if it was scrolled up less than 7 pixels up. The new implementation still has similar behavior as the new scroll anchor is not at the very bottom of the scrollable area, so the chat will still show the new message as long as the bottom edge of the previous oldest message was still visible when the new one arrived.
I left some extra ideas in #4012. Another quite simple one I have is to have an |
just some thoughts as i was queried: how is the state "keep scroll down" detected? iirc, there is somethings as "if the scroll position is not more than X pixels from the bottom, and a new message arrives, scroll down - otherwise the user has scrolled up manually and we stay to not disturb the user reading" if now the added reactions enlarges the view by more than X pixels, the calculation may fail. so, making sure, the threshold is large enough may already help. or, maybe better, recalculate. |
Yes, this is what's happening deltachat-desktop/src/renderer/components/message/MessageList.tsx Lines 351 to 352 in 26f8d98
Though it is not triggered when a reaction is added. |
Would anybody be against simply adjusting CSS such that reactions are more terse and don't cause height to change? |
OK, But for some reason when more messages are fetched, the scroll position jumps god knows where. However, I think this is not a problem with the approach, window.fetchMoreTop = fetchMoreTop
window.fetchMoreBottom = fetchMoreBottom And then call those functions manually from the console. The messages get loaded both above and below just fine, and the scroll position is preserved just fine. Update: no, it's not Delta Chat scrolling code. It appears to be a Chromium bug. It manifests itself when an element is inserted while you're scrolling. In that case the position does not get preserved. But it only applies when messages = document.querySelector('.scroller-content')
messages.parentElement.style.height = '600px';
function insertBottom() {
messages.appendChild(messages.lastElementChild.cloneNode(true));
}
function insertTop() {
messages.prepend(messages.firstElementChild.cloneNode(true));
}
// insertTop()
// insertBottom()
setInterval(() => {
insertTop();
insertTop();
insertTop();
setTimeout(() => {
insertBottom()
insertBottom()
insertBottom()
}, 17);
}, 300) Then select some element's text to better see it and try scrolling back and forth around it. You'll see that it shifts (which is bad). It doesn't shift if you don't scroll (which is how it's supposed to work), and it doesn't shift if you remove If you do the same on Firefox (Gecko), there is no such issue. Works like a charm. A suggestion I have to only insert new messages while we're not scrolling. In addition, disabling smooth scrolling could work. Update 2: I created a Chromium bug report Update 3: I uploaded this MR #4116 if anyone wants to check how far I got by now. |
Closes #3753 Closes #3763 WIP because see #3753 (comment) Uploading this purely for journaling purposes for now.
...resulting in new messages not getting scrolled into view when they arrive. This simply removes height changes between messages with / without a reaction. Closes #3753
...resulting in new messages not getting scrolled into view when they arrive. This simply removes height changes between messages with / without a reaction. Closes #3753
...resulting in new messages not getting scrolled into view when they arrive. This simply removes height changes between messages with / without a reaction. Closes #3753
...resulting in new messages not getting scrolled into view when they arrive. This simply removes height changes between messages with / without a reaction. Closes #3753
...resulting in new messages not getting scrolled into view when they arrive. This simply removes height changes between messages with / without a reaction. Closes #3753
...resulting in new messages not getting scrolled into view when they arrive. This simply removes height changes between messages with / without a reaction. Closes #3753
Closes #4404. The bug was introduced in db83279. Prior to that all attachments had a fixed height of 200px. This should also eliminate layout shifting when scrolling the chat, and when new messages arrive. An alternative solution that would still have layout shifting, but at least keep the chat "anchored" to bottom would be #3753 (comment), but it's not supported well by browsers.
Closes #4404. The bug was introduced in db83279. Prior to that all attachments had a fixed height of 200px. This should also eliminate layout shifting when scrolling the chat, and when new messages arrive. An alternative solution that would still have layout shifting, but at least keep the chat "anchored" to bottom would be #3753 (comment), but it's not supported well by browsers. Co-authored-by: Max Philippov <maxphilippov@users.noreply.github.com>
Closes #4404. The bug was introduced in db83279. Prior to that all attachments had a fixed height of 200px. This should also eliminate layout shifting when scrolling the chat, and when new messages arrive. An alternative solution that would still have layout shifting, but at least keep the chat "anchored" to bottom would be #3753 (comment), but it's not supported well by browsers. Co-authored-by: Max Philippov <maxphilippov@users.noreply.github.com>
Operating System (Linux/Mac/Windows/iOS/Android): Windows, Linux
Delta Chat Version: 1.44.0, 828f472
Expected behavior: The scroll position remains at the very bottom of the chat
Actual behavior: The chat becomes "scrolled up", gets "unlocked" from the bottom position
Steps to reproduce the problem:
Screenshots:
DeltaChat_xPvyZE4PNu.mp4
Logs: [none]
I think there should be a proper "declarative" fix. For example, I have noticed that in Chromium if you scroll to the bottom of the page and then increase the height of an element that is somewhere above, out of view, then the scroll height of the page will change, but the scroll position will remain at the bottom, unlike if the element was in view.
Update: oh lord oh please don't tell me this has to be fixed through scroll locking
deltachat-desktop/src/renderer/stores/messagelist.ts
Lines 309 to 310 in 0380993
The text was updated successfully, but these errors were encountered: