search

deltachat / deltachat-desktop

Email-based instant messaging for Desktop.
GNU General Public License v3.0
909 stars 167 forks source link

Adding a reaction makes the chat "scroll up" (then you have to scroll to see new messages) #3753

Open WofWca opened 4 months ago

WofWca commented 4 months ago

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

https://github.com/deltachat/deltachat-desktop/blob/0380993ba6f65f7d52ef2acbb64a3779f9ebca22/src/renderer/stores/messagelist.ts#L309-L310

WofWca commented 4 months ago

I'm noticing that our code is very similar to Signal, and Signal doesn't have this issue.

Jikstra commented 4 months ago

You can debug it by adding a display: none; to .styles_module_reactions

EDIT: Adding margin-bottom: -1px; to .styles_module_reactions fixes it i think. Why this happens? Not so sure...

WofWca commented 4 months ago

EDIT: Adding margin-bottom: -1px; to .styles_module_reactions fixes it i think. Why this happens? Not so sure...

Hmm idk. Didn't work for me

WofWca commented 4 months ago

What did work is this one simple trick (scientists hate me):

https://github.com/signalapp/Signal-Desktop/blob/29c404266863491a3b26a73b24602d36d7a3ac46/stylesheets/_modules.scss#L5573-L5587

Basically we gotta put overflow-anchor: none; on all of the messages except the one we want to stay in place (which in this case would be the last message).

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... Edit2: ohhh I see. We gotta ensure that all of the elements within the scrollable element except the one we're interested in, have overflow-anchor: none;, because that way they can get selected as anchors themselves. For me it was a .daymarker message that didn't have overflow-anchor: none;

WofWca commented 1 month ago

I left some extra ideas in #4012. Another quite simple one I have is to have an IntersectionObserver set "visible" class on all the visible messages, and ensure that only the last visible one has overflow-anchor: auto. Or we could apply this only to the last message - if it is in view, then turn off anchoring for all the messages. Otherwise have default anchoring.

r10s commented 1 month ago

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.

WofWca commented 1 month ago

"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"

Yes, this is what's happening

https://github.com/deltachat/deltachat-desktop/blob/26f8d98dc2f9dc6d98ed99b90629b0606d48f62e/src/renderer/components/message/MessageList.tsx#L351-L352

Though it is not triggered when a reaction is added.

WofWca commented 1 month ago

Would anybody be against simply adjusting CSS such that reactions are more terse and don't cause height to change? Until we figure out a better approach.