Message list scroll position is often slightly messed up

[borisyankov]

When showing/hiding UnreadNotice the message list scroll position is moved by the height of that panel.

As a result, when you narrow to a conversation with one or a few unread messages and at least a screenful of total messages (which is a very common scenario when staying caught up in a busy org), what you typically see is the screen jumps a couple of times, and then settles scrolled slightly too far up, with the last message only partly visible. This is annoying and looks pretty broken.

[summary expanded by @gnprice]

[jainkuniya]

#3027 is also in somewhat similar catalog.

[gnprice]

A few observations, from playing around with this briefly just now while looking at #3302:

• I added some console.log statements to our existing resize event listener. (In the version below, I also flipped the sign of the scroll.)

 window.addEventListener('resize', event => {
-  const difference = viewportHeight - documentBody.clientHeight;
-  if (documentBody.scrollHeight !== documentBody.scrollTop + documentBody.clientHeight) {
-    window.scrollBy({ left: 0, top: difference });
-  }
+  console.log(
+    `resize: ${viewportHeight} -> ${documentBody.clientHeight} (${documentBody.scrollHeight})`,
+  );
+  const difference = documentBody.clientHeight - viewportHeight;
+  window.scrollBy({ left: 0, top: difference });
+  console.log(`resize: (-> ${documentBody.scrollHeight})`);
   viewportHeight = documentBody.clientHeight;
 });

• Then I tried making the UnreadNotice appear and disappear; and making the keyboard appear and disappear.

• We sometimes don't even get the resize events! Specifically when the keyboard pops up. This causes the bizarre symptom [debug builds] App gets pushed partly off-screen when keyboard opens #3010. (Consistent with the reports in that thread, I haven't seen this in the wild -- only in a debug build.)

• When we do get those events, the numbers often don't make sense. We'll have a height of 509 (px), then the keyboard pops up... and the new height is supposedly 469.

• It seems like our scroll position often changes with the resize itself, before we even get the event. I haven't pinned this down, though.

[gnprice]

Also: this issue appears to be related to #3264.

I really dislike the feeling where I know there's been a discussion of something before, with lots of great details... and I can't find it, or it takes several minutes of searching various query terms before I find it. So I find it extremely helpful to stick cross-reference links whenever I find one discussion (issue, PR, chat conversation, etc.) is related to another, and I very much appreciate when others do the same 🙂

[borisyankov]

Relevant here is that the Keyboard-avoidance is set differently on Android and iOS.
On Android we do use:
android:windowSoftInputMode="adjustResize" in AndroidManifest.xml
While in iOS we use
behavior="padding" on a KeyboardAvoidingView.

[gnprice]

I just bumped the priority of this and expanded the description; the effect feels pretty annoying, and we hear complaints about it.

[gnprice]

Further observations, from @timabbott (on his phone: Android, likely Android 9, likely either 25.2.116 or 25.3.117):

we have a really annoying scrolling bug in mobile where closing the keyboard bounces me away from the end of the feed every time
Hmm okay not every time, shoot
But most of the time
May be related to whether local echo is running when i hit back to close keyboard
Hmm maybe not, but seems to happen 75% of the time when I close keyboard in this thread [i.e. a PM thread with me, where we were actively talking] with back button

[gnprice]

BTW see also #3457 "Narrow sometimes lands far in the past".

Probably a separate issue... but I've often found this issue thread when I was looking for that one, and vice versa.

[jainkuniya]

I did some debugging on [current master] (bea1ee5), and observed following issues:

• Our animation is smooth, first we are scaling down the size of unread notice and then making it hide by display:none see AnimatedScaleComponent. Ideally as soon as component scale down, animation component should start releasing space and shrink.

• As we suddenly hide AnimatedScaleComponent by setting display:none after animation completes, it triggers resize in webview. And we are handling this event only according to case keyboard pops in/out. That is when keyboard pops (webview size decreases) => scroll up, else scroll down (when keyboard hides, webview size increases).
  When unread notice disappears, webview size gets increased any by code it scrolls down.

So in #3892, I tried to differentiate resize events with the difference. It was kind of hack, i.e if difference is less than 100 then it is due to unread notice, else due to keyboard.

This resize event also gets triggered on initial paint, at this point difference is > 100.

[gnprice]

• When unread notice disappears, webview size gets increased any by code it scrolls down.

Down, or up?

Even just focusing on the keyboard case for a moment, I think it should be scrolling up when it gets bigger: that's how it keeps the bottom of the viewport, i.e. the last message you're looking at, scrolled to the same place. If it's not doing that, then that's its own bug.

If it is doing that -- if it is making it so that the bottom of the viewport stays at the same spot showing the same message -- then that seems like a perfectly fine behavior for the unread banner coming and going just as much as for the keyboard coming and going.

But the observed behavior is that, somehow, it's not successfully preserving the position of the bottom of the viewport. Why is that? One thing I think we don't yet know the answer to is: what exactly is the step at which something's happening that's different from what we expect, and what's happening instead?

This is a question that can only be answered by observing, and logging details. Reading the code can be a useful source of guesses, but there's enough going on (especially with the choices the browser makes) that we'll need to see things empirically to know for sure. (Maybe you've already done some of this empirical study; I'm not sure from your comment, but in any case I'll be interested in seeing the details from it.) It may be productive to describe some of this debugging in a chat thread on a stream -- that can be a more natural place to post lots of details than a GitHub issue thread.

[jainkuniya]

I think it should be scrolling up when it gets bigger:

ya, when keyboard gets in (webview becomes smaller), webview scrolls up.

and on the other hand: when keyboard hides (webiew becomes larger), webview scrolls down.

Similarly, when unread notice hides, (webiew becomes larger), webview scrolls down. Instead in this case, we need to do nothing.

I have tested this by logging/alert and found that when unread notice hides, resize is called and difference is -40 (on iPhone 5S), and if message list is scrollable then it scrolls by -40 i.e scrolls down.

window.scrollBy({ left: 0, top: difference });

This fits in the keyboard scenario, i.e when keyboard pops in, webview size decreases & it scrolls up and vice versa. But this doesn't fit unread notice case.

[jainkuniya]

I think, we should not rely on resize events for checking & acting when keyboards pops in/out. We can get this information from RN and then we can send events to weview.
So that, we are nothing anything extra on resize events, which occurs in many cases: like when unread notice gets in/out, on initial render, when messages renders etc

[gnprice]

Hmm, it seems we still have some confusing details to sort out. Let's discuss in this chat thread -- I think that will be a better medium for this.

[gnprice]

Just sent #4043 with what appears to be the solution of this mystery.