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

webview, js: Only scroll on resize when !isNearBottom(). #4003

Closed
wants to merge 1 commit into from

Conversation

jainkuniya
Copy link
Member

So that when unread notice hides, webview doesn't scroll up.
This also seems to work fine when keyboard pops in/out.

Fixes: #3301

This is a case when user is already near bottom and webview size
increases, one use case is when unread notice pops out.
In such cases, webview content can not further scoll down because it is already at
the end. Thus there is no need of custom handeling in such cases.

This also seems to work fine when keyboard pops in/out.

Fixes: zulip#3301
@gnprice
Copy link
Member

gnprice commented Apr 10, 2020

Hmm. Can you describe the exact repro where this makes a difference? I just spent some time playing with this, before and after this commit, and couldn't find one.

@jainkuniya
Copy link
Member Author

For testing this I would suggest you to

  • or to topic/stream narrow which have unread messages, now scroll to down. As messages are marked as read, unread notice will pop out without changing the scroll.

@jainkuniya
Copy link
Member Author

On iPhone 5s 12.1
Before this:
2020-04-10 08 16 11

On this PR:
2020-04-10 08 15 05

@gnprice
Copy link
Member

gnprice commented Apr 15, 2020

Hmm -- in the "on this PR" screencast, that behavior doesn't look like what we want!

At the very end, just before the banner disappears, we're at the bottom of the message list -- the last message in the narrow, with content "hello", is there at the bottom of the viewport.

Then the banner disappears, and in the next frame that last message is out of view -- we've effectively scrolled up by a bit, and the next-to-last message in the narrow is the one visible at the bottom.

So that sounds a lot like the original problem in #3301.

@gnprice
Copy link
Member

gnprice commented Apr 15, 2020

OK, @jainkuniya tells me he thinks he reversed the two screencasts above.

I do reproduce this problem easily on my iPhone 7 (iOS 12), though still not in emulators running Android 8 or Android 10. And I see how something like this could help.

The commit message and PR description don't agree either with each other or the code.

  • PR: don't scroll on resize when near bottom
  • commit message: don't scroll on resize when near bottom and difference > 0
  • code: don't scroll on resize when near bottom and not difference > 0

I think that's reflective ultimately of confusion in the code. What does difference mean, anyway? What's the significance of it being positive or negative -- probably one of those is when the viewport grows and one when it shrinks, but which is which? It's basically impossible to tell without closely reading the code.

As I think a bit more about this, I'm also wondering: why near the bottom? In fact we already have a closely-related check:

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

namely documentBody.scrollHeight !== documentBody.scrollTop + documentBody.clientHeight. The isNearBottom compares the exact same two values, but just asks if they're within 100px rather than if they're equal.

The theory this change is based on is: when we're at the bottom of the message list and the viewport grows by N px, there isn't room for it to show N px more content below, so it doesn't; and so when we get the resize event, we shouldn't scroll N px up to compensate like we normally would.

But on that theory, when that happens, we should be at the very bottom of the message list. Even if we were just near the bottom before the viewport grew, the browser would have first shown all the remaining few px of content at the bottom before showing more above.

So it seems like there's basically some kind of bug here, on iOS only, where the signals are getting crossed.

gnprice added a commit to gnprice/zulip-mobile that referenced this pull request Apr 15, 2020
As the code comments (which we added in the last few commits) explain:
when the viewport grows, we generally want to scroll up to keep its
bottom edge aligned within the content, but we want to skip that when
we're at the very bottom.

If we're at the bottom and scroll up anyway, the effect would be
exactly the symptoms in zulip#3301.

It turns out that when we're at the bottom, the `scrollTop` value may
not actually quite line up with where you'd think it should be.
Empirically:
 * On iOS (in Safari), it tends to be 1px beyond the bottom.
 * On Android (in Chrome), it's not an integer, and can be a fraction
   of a px either beyond or short of the bottom.

So, allow for that when checking to see if we're at the bottom.

Thanks to Vishwesh for suggesting a related direction in zulip#4003.

Fixes: zulip#3301
@gnprice
Copy link
Member

gnprice commented Apr 15, 2020

The theory this change is based on is: when we're at the bottom of the message list and the viewport grows by N px, there isn't room for it to show N px more content below, so it doesn't; and so when we get the resize event, we shouldn't scroll N px up to compensate like we normally would.

But on that theory, when that happens, we should be at the very bottom of the message list. Even if we were just near the bottom before the viewport grew, the browser would have first shown all the remaining few px of content at the bottom before showing more above.

I dug into this some more, and seem to have resolved the mystery. See #4043, which also unpacks that existing code.

Thanks for the testing of this direction!

@gnprice gnprice closed this Apr 15, 2020
gnprice added a commit to gnprice/zulip-mobile that referenced this pull request May 5, 2020
As the code comments (which we added in the last few commits) explain:
when the viewport grows, we generally want to scroll up to keep its
bottom edge aligned within the content, but we want to skip that when
we're at the very bottom.

If we're at the bottom and scroll up anyway, the effect would be
exactly the symptoms in zulip#3301.

It turns out that when we're at the bottom, the `scrollTop` value may
not actually quite line up with where you'd think it should be.
Empirically:
 * On iOS (in Safari), it tends to be 1px beyond the bottom.
 * On Android (in Chrome), it's not an integer, and can be a fraction
   of a px either beyond or short of the bottom.

So, allow for that when checking to see if we're at the bottom.

Thanks to Vishwesh for suggesting a related direction in zulip#4003.

Fixes: zulip#3301
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Message list scroll position is often slightly messed up
2 participants