Fix scroll-jumps when jumping to a permalink #9340
Conversation
Having turned on scroll debugging, it looks like element-hq/element-web#23393 is caused by scroll events firing while forward-filling is happening, which causes ScrollPanel's onScroll() to lock the scroll position to the bottom of the page via saveScrollState(), rather than honour the permalink's scroll offset. Either we could make saveScrollState() honour permalinks, or just ignore scroll events which fire while filling - as checkFillState() will go and apply the scroll-lock anyway once the filling is complete. This fix attempts the latter; i've manually tested a bunch (~20 times) and it /seems/ to have fixed it? It was pretty hard to repro in the first place, though.
ara4n
added
the
T-Defect
|
btw, there aren't tests on this because I don't think we've figured out how to test scrolling at all yet - which might explain why it's such a mess. one option could be to have a mode which consciously delays most scroll activity (backfilling, etc) in order to make it easier to reason about and test. I've tested this manually to check whether it breaks scroll behaviour when scrollbacking, but it seems to be okay (surprisingly, given, you'd expect scroll events to fire while backpagination is happening, which get discarded, causing jumps. but it might actually have /fixed/ scrolljumps while backpaginating perhaps?) |
| @@ -226,7 +226,15 @@ export default class ScrollPanel extends React.Component<IProps> { | |||
|
|
|||
| private onScroll = ev => { | |||
There was a problem hiding this comment.
Let's ship this 🙇
Merged develop so we can ensure it's still good to go test wise even though there are no scroll tests.
There was a problem hiding this comment.
TypeScript strict CI job is failing but it's complaining about an adjacent unrelated line of code and that job is not required to pass ✅
Cypress flake that passed after the second run ✅
Everything looks okay to merge ⏩
|
from oob: we're not sure if this actually fixes an issue at the moment, and needs a regression test to do so. Writing a test for this will be challenging until element-hq/element-web#23531 is complete. This PR might also conflict with element-hq/element-web#23539 Marking as blocked until one of those two issues is closed, at which point we can re-evaluate this. |
turt2live
added
the
X-Blocked
| if (this.isFilling) { | ||
| debuglog("skipping onScroll due to isFilling", this.getScrollNode().scrollTop); | ||
| return; | ||
| } |
There was a problem hiding this comment.
we're not sure if this actually fixes an issue at the moment, and needs a regression test to do so.
@ara4n thinks the problem might still be reproducible even with this fix but do these changes at least make sense? There could be many points that could cause the problem and this plugs a few of the holes.
In that case, we should merge anyway
Johennes
requested review from
a team,
andybalaam and
florianduros
and removed request for
a team
|
It's true that we don't have the bandwidth to devote to fixing scroll jumpiness properly (which would also include tests). Meanwhile though I don't think we necessarily need to block all contributions that aim at improving the situation. I think this should go through normal review again (because the original one was a year ago) and have a few folks test it. I've resolved the conflicts and requested another review from matrix-org/element-web. |
Johennes
removed
the
X-Blocked
|
Hm, testing the Netlify build, I don't really see any things which got worse. However, also no real improvement. Didn't really have a permalink to jump to, but used the unread message button to jump up. Did only test once, maybe there even was some improvement to that which I didn't notice? At least, it still seems broken.
By the way, with that build (compared to development.element.io), this icon does only show, if I scroll up. Otherwise, it's hidden. Not sure whether this is introduced by this PR, whether this is wanted, or another bug. Just wanted to mention it. |
|
This seems to be causing a read receipts test to not request a read marker which is a distinctly non-obvious and subtle failure mode... |
|
Closing now for as the test failure looks like it could be legitimate and doesn't look trivial to fix. Hopefully we can revisit this later as part of scroll jump investigation. |
Having turned on scroll debugging, it looks like
element-hq/element-web#23393 is caused by scroll events firing while forward-filling is happening, which causes
ScrollPanel'sonScroll()to lock the scroll position to the bottom of the page viasaveScrollState(), rather than honour the permalink's scroll offset.Either we could make
saveScrollState()honour permalinks, or just ignore scroll events which fire while filling - ascheckFillState()will go and apply the scroll-lock anyway once the filling is complete.This fix attempts the latter; i've manually tested a bunch (~20 times) and it /seems/ to have fixed it? It was pretty hard to repro in the first place, though.
Hopefully fixes element-hq/element-web#23393 and chronic bug 89.
Here's what your changelog entry will look like:
🐛 Bug Fixes