This repository has been archived by the owner on Sep 11, 2024. It is now read-only.
Improved scrolling & pagination #2676
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bwindels
force-pushed
the
bwindels/improvedscrolling
branch
from
39c8441
to
8134147
Compare
|
Need to fix unit test, but otherwise should be ready for review. |
bwindels
force-pushed
the
bwindels/improvedscrolling
branch
from
a9f63b6
to
5066984
Compare
(only where supported, polyfill doesn't give good results)
as they are an order of magnitude faster in most browsers, getBoundingClientRect() tends to cause relayout.
before we would clear it as soon as you were 1px away from the bottom of the timeline, which would still create jumping as the whitespace would around 36px. To play it safe, we only clear it after moving 200px from the bottom. Also include "local echo" scroll events, caused by setting scrollTop
this is not nearly as smooth as using ResizeObserver, as the callback rate is a lot lower, but seems to be quite a bit better than what we have right now, without the 7% cpu hog that the requestAnimationFrame polling fallback has.
bwindels
force-pushed
the
bwindels/improvedscrolling
branch
from
6bdb313
to
4203079
Compare
|
There is a remaining issue with this PR, where while repeatedly scrolling to the top, it gets stuck at the top back-filling until you scroll down again. Moving it out of review because of this. |
as we're approaching from the last node, if we're scrolled up, the first node we encounter would be below the bottom of the viewport change the logic to stop at the first node that has its top above the viewport bottom. When completely scrolled up, this was causing nodes way below the viewport to be selected as the reference for the pixelOffset, and when pagination came in, it would immediately try to apply the big negative pixelOffset, scrolling to a negative scrollTop, thus going to the top again, and triggering another pagination, entering in an infinite pagination loop until you scrolled down.
bwindels
force-pushed
the
bwindels/improvedscrolling
branch
from
8c1c923
to
c920dd2
Compare
karma seems to be giving priority to a location where an old version is installed.
|
FTR: should not be part of 1.0.2-rc, so ready to review, but don't merge for now. |
jryans
approved these changes
Feb 28, 2019
| @@ -935,6 +935,11 @@ var TimelinePanel = React.createClass({ | |||
| {windowLimit: this.props.timelineCap}); | |||
|
|
|||
| const onLoaded = () => { | |||
| // clear the timeline min-height when | |||
There was a problem hiding this comment.
Such a low line length for comments! 😉
| for (let i = 0; i < 1000; ++i) { | ||
| threshold.push(i / 1000); | ||
| } | ||
| threshold.push(1); |
There was a problem hiding this comment.
Nit: Could also do i <= 1000 in the loop, if you like.
15 tasks
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Based off #2671
element-hq/element-web#8565
The main point of this PR is to observe the timeline for size changes, and restore the scroll position to keep the last tile aligned to the bottom of the viewport. Observing the timeline for size changes uses ResizeObserver where available (for now Chrome >= 64), completely preventing scroll jank (as ResizeObserver triggers between layout and paint), and falls back to IntersectionObserver (Edge & Firefox) which can still be a bit janky depending on how fast you scroll (the callback is called a lot less frequent than ResizeObserver), but still feels better than not restoring the scroll position on resize at all.
Apart from that it also tries to improve the block-shrinking behaviour (setting a
min-heighton the timeline when showing typing notifications, to prevent jumping when they are hidden again), but clearing it on timeline resets (this was the cause of the white wall of doom), and clear it not immediately when scrolling up, which also caused jumping.I also removed the fix for element-hq/element-web#528, it makes the code harder to understand, it's for a 3 years old version of Chrome, and richvdh thought the workaround might make other things less reliable. (Discussion here)