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

New events may cause the timeline to jump up 30-40px after scrolling down #1162

Closed
ara4n opened this issue Mar 15, 2016 · 12 comments
Closed
Assignees
Labels
P1 S-Tolerable Low/no impact on users T-Defect

Comments

@ara4n
Copy link
Member

ara4n commented Mar 15, 2016

The jump happens either immediately after you send the message, or in the middle of sending it.

Am trying to repro with scroll debugging enabled.

@ara4n ara4n added T-Defect P1 S-Tolerable Low/no impact on users labels Mar 15, 2016
@ara4n
Copy link
Member Author

ara4n commented Mar 17, 2016

I think this may be related to very tall EventTiles (be they containing images or lots of multi-line stuff) in history.

@ara4n
Copy link
Member Author

ara4n commented Mar 18, 2016

i finally caught this with debug on (but inspector shut). it coincides with the rate-limited logging section.
barf.txt

@richvdh
Copy link
Member

richvdh commented Mar 19, 2016

So an interesting thing in that barf is that it shows the scroll state is tracking a particular event (Scrolled to token $1458321036372erwrk:dbkr.me + 59) rather than being stuck-to-bottom (Scrolled to bottom).

That implies that when we last saw a scroll event (17:12:50.011 in the barf), the messagepanel is scrolled up more than 3 pixels from the bottom. I can't really explain why that would have been the case, though

@ara4n
Copy link
Member Author

ara4n commented Mar 19, 2016 via email

@richvdh
Copy link
Member

richvdh commented Mar 19, 2016

agreed on all fronts...

@ara4n
Copy link
Member Author

ara4n commented Mar 20, 2016

Just got a classic instance of this, complete with uncensored logging. I think I even have the right timespan (marked in the log) where it kicked off. The scrollback suddenly jumped up whilst Erik was typing to me in Internal, and jumped as I pressed the first keystroke in starting a new message. (I wonder if this might be related to the dodgy multi-line-resizing hack that started breaking the other day? https://github.com/matrix-org/matrix-react-sdk/blob/57ffc2d2e26f5f060277c2272b220d85cf096348/src/components/views/rooms/MessageComposer.js#L260)

Barf attached. I think I'll turn off the multi-line resizing on my local develop to see if that helps.

barf2.txt

@ara4n
Copy link
Member Author

ara4n commented Mar 20, 2016

...and again, just after you posted the ALOT img in Internal. I hadn't turned of multiline resizing yet.

barf3.txt

@richvdh
Copy link
Member

richvdh commented Mar 20, 2016

The odd thing in those barfs is all the scroll events (ones that result in 'saved scroll state ...' rather than 'ignoring scroll echo'). Unless you are manually scrolling, we wouldn't expect to see any scroll events other than scroll echoes, and their presences reinforces the idea of the DOM reshuffling itself in a way that confuses us.

Looking at my console around the same time, I also see these spurious events, though in my case it ended up sorting itself out, rather than causing a jump.

@ara4n ara4n added this to the v1 - Private Preview milestone Mar 21, 2016
@richvdh
Copy link
Member

richvdh commented Mar 21, 2016

for the record:

  • turning off multi-line resizing didn't help
  • @kegsay observed it this morning in a room with no images at all
  • For anybody wanting to follow along at home, a breakpoint at the top of ScrollPanel._saveScrollState will help catch the bug in flight. Irritatingly this will also fire when changing rooms or when scrolling manually, so use of ctrl-f8 (deactivate breakpoints) is recommended when trying to scroll manually.

@richvdh
Copy link
Member

richvdh commented Mar 21, 2016

With much gratidute to @kegsay, repro steps are:

  • scroll up a bit
  • scroll down quite quickly (but not by clicking on the arrow)
  • say something (or have someone else say something)

I'm assuming the images were a total red herring, and merely appeared related because you're more likely to scroll up to see what went before if there is an image.

The problem is that we are ignoring the final downward scroll event, because we think it is an echo of our last autoscroll.

@richvdh
Copy link
Member

richvdh commented Mar 21, 2016

(you have to scroll down quite quickly, otherwise the penultimate scroll event is close enough to the bottom for us to switch to stuck-to-bottom mode)

@richvdh richvdh changed the title Sending a message on a page with an image thumbnail on it may cause the timeline to jump up 30-40px New events may cause the timeline to jump up 30-40px after scrolling down Mar 21, 2016
richvdh added a commit to matrix-org/matrix-react-sdk that referenced this issue Mar 21, 2016
When the user scrolls up, and scrolls back to where they were, we want to save
the final scroll state. We were ignoring it because it looked the same as the
last autoscroll.

Fixes element-hq/element-web#1162
@richvdh
Copy link
Member

richvdh commented Mar 21, 2016

Hopefully fixed by matrix-org/matrix-react-sdk#237

@richvdh richvdh closed this as completed Mar 21, 2016
richvdh added a commit to matrix-org/matrix-react-sdk that referenced this issue Mar 22, 2016
richvdh added a commit to matrix-org/matrix-react-sdk that referenced this issue Mar 22, 2016
The most recent problem was that we were setting _lastSetScroll whenever we
wrote to scrollTop (and ignoring the next scroll event which matched that
offset), but if there was no change to scrollTop, we wouldn't actually get a
scroll event, so would ignore some future scroll event instead.

Make sure that we only set _lastSetScroll if there's a change to scrollTop.

(Fixes element-hq/element-web#1162, more)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 S-Tolerable Low/no impact on users T-Defect
Projects
None yet
Development

No branches or pull requests

2 participants