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

Safari sometimes interrupts trackpad scrolling during fast swipes #160

Closed
martinpesout opened this issue Mar 17, 2016 · 52 comments
Closed
Labels

Comments

@martinpesout
Copy link

I see one problem in Safari browser (9.0.3) on my Macbook. I'm trying this example https://bvaughn.github.io/react-virtualized/ with 10000 of rows. If I try to scroll quickly using touchpad, I'll see that from some time is smooth elastic scrolling gone and everything became really choppy.

bre 17 2016 09 31

It looks like there is triggered some event which prevents scrolling.

@vincent-io
Copy link

Same here, also on latest Chrome (49.0.2623.87) on Mac.

@bvaughn
Copy link
Owner

bvaughn commented Mar 17, 2016

Hi @martinpesout, @vincent-io,

I'm not able to reproduce what you describe. (Scrolling is fast for me in Safari, even with 10k rows.)

I have however noticed an occasional issue with Safari that might be what you're seeing (not sure?). Basically if I quickly swipe up or down on the track pad while also swiping a little to the right or left (usually by accident) it causes the scroll animation to come to a sudden stop. This doesn't happen with a mouse wheel (obviously) or with the scrollbar. Only with the track pad.

Could this be what you're seeing?

@martinpesout
Copy link
Author

I did a few tests and this is probably source of this problem. Is there any locking of scrolling when you swipe to the side? Maybe increasing detection limit for swiping to the sides can solve this situation?

If I swipe down and just a bit to the left, scrolling suddenly stops. When I repeat this situation a many times (swipe down and just a bit to the left), I have to wait a while because in that moment I'm not able to scroll down without animated scrolling (it looks that this scrolling animation is being stopped). After while problem is gone and I can continue swiping down with animated scrolling.

This problem really doesn't happen with a mouse wheel.

@vincent-io
Copy link

Yes, this could very well be the problem. Hard to be 100% sure, as I can't see if I'm actually swiping sideways, but considering the sensitivity of the trackpad this is very plausible.

This is much easier to reproduce on Safari than on Chrome by the way.

@bvaughn
Copy link
Owner

bvaughn commented Mar 18, 2016

This is much easier to reproduce on Safari than on Chrome by the way.

Agreed. I've only observed this on Safari. Anecdotally I've only observed it while swiping really quickly too. Slow swipes are fine. And slow swipes that build to really fast swipes are always fine. Starting from a dead stop and swiping quickly sometimes causes the problem though.

Is there any locking of scrolling when you swipe to the side? Maybe increasing detection limit for swiping to the sides can solve this situation?

I'm not locking or manually managing scrolling at all. I'm just reacting to scroll events by querying the updated/new scrollLeft and scrollTop values. So I'm not sure how I can affect this behavior.

@vincent-io
Copy link

Agreed. I've only observed this on Safari. Anecdotally I've only observed it while swiping really quickly too. Slow swipes are fine. And slow swipes that build to really fast swipes are always fine. Starting from a dead stop and swiping quickly sometimes causes the problem though.

+1 This exactly describes the behaviour I'm experiencing.

@bvaughn
Copy link
Owner

bvaughn commented Mar 18, 2016

This is a behavior that's been around for at least a month for what it's worth. And I'd love to fix it but I don't have any ideas currently. Would welcome ideas if you guys think of any. :)

@bvaughn bvaughn changed the title Performance problems during fast scrolling Safari sometimes interrupts trackpad scrolling during fast swipes Mar 19, 2016
@martinpesout
Copy link
Author

Thanks for your response. Keep this issue open, I'll try to find some solution in next 14 days.

@andevsoftware
Copy link

First off, thanks for the great work on how this project is set up! Upon testing in Safari I was running into this issue too. For me it doesn't happen in Chrome (49.0.2623.110). When I'm using the trackpad on my Macbook in Chrome the problem doesn't show because it won't let me scroll diagonally. In Safari it does allow me to scroll diagonally and there the problem arises like already mentioned. Personally I don't care to much about diagonally scrolling, so I'm going to test what happens when I allow either vertical or horizontal scrolling based on which offset is the greatest when the scroll event is fired. Will keep you posted.

Chrome, messing around with trackpad. Could not manage to scroll diagonally:
chrome

Safari, scrolling diagonally:
safari

@bvaughn
Copy link
Owner

bvaughn commented Apr 5, 2016

Thanks @olivierandriessen :)

One more data point that may be worth mentioning is that I am unable to reproduce this behavior (or at least much less frequently) using Safari Technology Preview than vanilla Safari. Seems like STP behaves more consistently with Chrome and Firefox.

@andevsoftware
Copy link

@bvaughn Thanks for mentioning, sounds this problem will resolve itself in the near future then. I might leave it for now and test tomorrow in STP too.

@andevsoftware
Copy link

Looks like STP doesn't allow me to scroll diagonally. Still, when scrolling horizontal I notice some issues. Perhaps it's just performance, will let you know if I find out. It's hard to notice but at the end I scroll right and you see it wiggle after that I scroll left.

stp

@martinpesout
Copy link
Author

That's interesting insight guys. I confirm that I also don't have this problem in Safari Technology Preview. So if I understand STP well, we should expect the same behavior in Safari in the future, right?

@andevsoftware
Copy link

@martinpesout Although the it can still change I think we can expect that the behavior will be the same in the next version.

@andevsoftware
Copy link

One thing to note is that the issue in Safari only arises when inertial scrolling sets in (where the scrolling continues a bit and then gradually comes to a halt when you lift your fingers off the trackpad). So moving with fingers letting go is the problem, moving stopping then letting go not.

@andevsoftware
Copy link

By caching all the elements being rendered and implementing shouldComponentUpdate I could bring down the time it took to render the grid from approx. 6ms to 0.1-0.6ms, so definitely recommend doing that. I almost notice no lag in STP now.

@bvaughn
Copy link
Owner

bvaughn commented Apr 6, 2016

Very interesting @olivierandriessen.

I have an open issue (#162) and an ongoing branch (batch-rendering) where I've been experimenting with reducing the number of times I render as well as caching rendered cells. It doesn't quite work right yet though (eg. scroll-to-index is broken) and I haven't had time to dig into deeper it yet.

I'm also vaguely concerned about caching cells having an unexpected impact on the way people are already using virtualized. Not sure there. Maybe I could make it something that could be enabled/disabled...

@superlaziness
Copy link

Hi @bvaughn, thanks for the project!

I'm testing FlexTable component.
After update to 6.1.1 it wiggles more than before while scrolling in Safari (9.0.3). This can be clearly seen when you scroll slow.

virtuilized-safari

In Chrome it looks smooth, but uses more CPU after update.

@bvaughn
Copy link
Owner

bvaughn commented Apr 9, 2016

@superlaziness Which version are you comparing 6.1.1 to? (It "uses more CPU" than which version?)

@superlaziness
Copy link

@bvaughn with 6.0.4

@bvaughn
Copy link
Owner

bvaughn commented Apr 9, 2016

Hm. So the only things that changed between those versions were aria-related (usability) features in the 6.0.x range and the renderCellRanges property. I wouldn't expect either of those to have an impact on performance.

@superlaziness
Copy link

Oops, sorry. I also updated the React to 15.0.1. This may be because of this.

@bvaughn
Copy link
Owner

bvaughn commented Apr 9, 2016

That would be good to know! I'm moving today so I'm pretty swamped. (Just taking a break from loading up the truck.) If you get the chance to compare performance with 6.1.1 and React 0.14 vs 15 - let me know?

@superlaziness
Copy link

Ok. I made some measurements. There is no difference between v.6.1.1 and 6.0.4.

render time cell grid
15.0.1 ~26ms ~85ms
0.14.6 ~8ms ~20ms

@gaearon
Copy link

gaearon commented Apr 11, 2016

Are you measuring production builds?

@bvaughn
Copy link
Owner

bvaughn commented Apr 11, 2016

My goodness, Dan. You're almost creepy fast. :)

@superlaziness
Copy link

@Gaeron no I measured it with Perf on dev builds.

@superlaziness
Copy link

But I've just made some tests on production builds: 11ms on 0.14.6 vs. 24ms on 15.0.1

@superlaziness
Copy link

Anyway, I can't see any visual difference between versions in Chrome. And it is not usable in Safari on both of versions yet.

And yes, those wiggles only visible when you use trackpad with inertial scroll.

@bvaughn
Copy link
Owner

bvaughn commented Apr 11, 2016

it is not usable in Safari on both of versions yet.

Could you clarify what you mean by this @superlaziness? Aside from occasionally interruption of fast scrolling (for reasons described above) react-virtualized works fine for me in Safari with both React 0.14 and 15.0. (I do notice a bit of the jitter you mention above for React 15 dev build though.)

@superlaziness
Copy link

This is clearly seen on gif I attached above. @olivierandriessen described the same issue.

When you scroll with trackpad and then lift your fingers off, Safari continues to scroll grid with inertia. At this time, the grid starts to twitch. It looks really bad :(

I'll test it in STP tomorrow.

@andevsoftware
Copy link

@superlaziness Currently I'm managing to keep the user experience acceptable by keeping the render times as low as possible. shouldComponentUpdate helps a lot with this.

@superlaziness
Copy link

I guess I found something in componentDidUpdate method:

    if (
        scrollTop >= 0 &&
        scrollTop !== prevState.scrollTop &&
        scrollTop !== this.refs.scrollingContainer.scrollTop
      ) {
       this.refs.scrollingContainer.scrollTop = scrollTop;
      }

This block updates scroll position to one from state. State's scrollTop contains value from previous animation frame (_setNextState method). By assigning old scroll value it returns scroll several pixels back and we see those twitches.

Without this block inertial scroll doesn't work in Safari.

@superlaziness
Copy link

@olivierandriessen yes, this also should be part of the solution. Can you show your workaround?

@bvaughn
Copy link
Owner

bvaughn commented Apr 12, 2016

This block updates scroll position to one from state. State's scrollTop contains value from previous animation frame (_setNextState method). By assigning old scroll value it returns scroll several pixels back and we see those twitches.

Great observation @superlaziness. I've been focusing my free time efforts on the branch for #182 but I'll try to play around a bit with what you've pointed out. Maybe I can drop the animation-frame loop without negatively impacting performance? In previous testing, I've anecdotally observed that it usually doesn't have much of an impact.

@superlaziness
Copy link

superlaziness commented Apr 19, 2016

I forgot to tell that I made a quick-duct-tape-fix for this problem:
this.refs.scrollingContainer.scrollTop = scrollTop; —> this.refs.scrollingContainer.scrollTop++;
It works fine for me. I do not know how it affects issue #2.

@bvaughn
Copy link
Owner

bvaughn commented Apr 19, 2016

Doesn't that assume that scrolling is always in the downward direction (and at a constant rate)?

@superlaziness
Copy link

Nope! It shifts scroll by one pixel than expected. That is allow browser to continue scroll animation. And 1px shift is actually invisible.

@bvaughn
Copy link
Owner

bvaughn commented Apr 19, 2016

Hm. I'm wary of that approach because of its potential impact for issues like #2 and also because of its potential impact on things like 1pixel borders. It's interesting to note though (eg. if you had a top border of 1 pixel you would never be able to see it with this patch).

I don't have the bandwidth to dive back into scrolling issues at the moment so I'm going to leave this sit for a while. We'll see if others have insight. :)

@bvaughn
Copy link
Owner

bvaughn commented May 30, 2016

Safari 9.1- I'm specifically testing with 9.1 (11601.5.17.1)- seems to have improved this issue. I can still reproduce it but it works better than Safari 9.0 did.

@jfrolich
Copy link

jfrolich commented Jun 21, 2016

Also having this issue, just updated from 7.6.0 -> 7.8.1 and the issue looks to have been getting worse. Will also test using the tech preview. Chrome works fine. Amazing work btw, love the lib! I'm on safari 9.1.1 btw.

@bvaughn
Copy link
Owner

bvaughn commented Jun 21, 2016

Hey @jfrolich,

Yeah, the issue is still around for Safari. I actually tested it just earlier this morning to see if an update had fixed it yet (since it's fixed in Safari Technology Preview for some time now). I'd be surprised if the issue got any worse from 7.6 to 7.8 to be honest. I think it's just the sort of thing that's sometimes easy to reproduce and sometimes harder.

Thanks for the compliment on the lib! :)

@superlaziness
Copy link

@jfrolich, if you also have updated to React 15.x, try to run it in production.

@bvaughn
Copy link
Owner

bvaughn commented Jun 21, 2016

Hard to overstate the different between Prod and Dev for React 15 yeah

@jfrolich
Copy link

@superlaziness: I run it in production. Probably indeed it just looked worse, do not have any data :)

@bvaughn
Copy link
Owner

bvaughn commented Aug 2, 2016

I am closing this issue because I have no planned course of action to take. It seems to impact only Safari and it seems to have been fixed in the Safari Tech Preview. There's some good discussion in this thread ^ and I'll happily discuss anything ongoing with folks on the thread if anything new develops. For the moment though it seems like there's not much to do (that I'm aware of) other than wait for a Safari update.

@mtchllbrrn
Copy link

mtchllbrrn commented Aug 24, 2016

I'm also having this same issue, and using Safari Tech Preview hasn't solved it. It improves performance, but the list is still unusably janky. This seems to indicate the problem won't be fixed by a Safari update. In light of this, what should we do?

fwiw, I've done some performance profiles in-browser, and it seems the problem is related to JS computation within React, rather than repaints/reflows. I've dived into the code a bit, done some testing with react-addons-perf, but I haven't turned up any obvious causes so far.

@bvaughn
Copy link
Owner

bvaughn commented Aug 24, 2016

@mtchllbrrn What browser are you seeing "unusably janky" performance on? Which react and react-virtualized versions? Are you sure you're running React in production mode (minified JS from CDN)?

Safari Tech Preview performs pretty flawlessly for me every time I've tested it. It's on-par with Chrome and Firefox. I have never once seen the interrupted scroll issue there. Scrolling "jankiness" is different. I'm also not seeing that- but I want to point out that that's not what this issue is about. :D

@mtchllbrrn
Copy link

After reading some of the previous posts more carefully, I see now that my issue might be something different. The problem I'm seeing is a twitching behavior, and it occurs even when my fingers are on the trackpad (unlike the issue discussed above).

Maybe this gif will help: https://cl.ly/0d0B1k1E3W1F

It's difficult to see here due to the framerate limitations on the gif, but if you look at the scrollbar, you'll see that it jitters within the first few seconds . This is the problem I'm having, and it occurs in both Safari and Safari Tech Preview. fwiw, React is in production mode, and I'm using the latest version of react-virtualized.

If you like, I could try to put together an example repo that demonstrates the problem and open another issue for it.

@bvaughn
Copy link
Owner

bvaughn commented Aug 24, 2016

Hm. I see it! Can you share this with me so I can run it? Don't think it's related to this issue.

@jfrolich
Copy link

jfrolich commented Sep 1, 2016

@mtchllbrrn: Same here. Happens in both current safari and tech preview.

@mtchllbrrn
Copy link

@jfrolich Check out #357 and #370. It seems our problem might be unrelated to this one.

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

No branches or pull requests

8 participants