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

Freezing main thread on iOS Safari #130

Open
coreyward opened this issue Nov 15, 2018 · 13 comments
Open

Freezing main thread on iOS Safari #130

coreyward opened this issue Nov 15, 2018 · 13 comments

Comments

@coreyward
Copy link

Using Gatsby v2, this library is causing an issue with scrolling in a container on iOS Safari in a particular situation.

<div class="nav">
  <div class="item">Some content</div>
  <div class="item">Some content</div>
</div>
body {
  position: fixed;
  overflow: hidden;
 }

.nav {
  position: fixed;
  top: 0;
  left: 0;
  bottom: 0;
  right: 0;
  overflow-y: auto;
  -webkit-overflow-scroll: touch; 
}

This is a fairly basic navigation overlay with scrolling content. Assuming the child elements are long enough, this should scroll up and down while the <body> remains fixed.

It works generally, but if I'm at the top of the .nav element and try to scroll up further (swiping down, which would normally give you a peak above the document w/ the springboard animation), the page doesn't scroll at all and scrolling down (swiping up) immediately after is similarly “frozen”. After ~1s with no interaction the behavior resets and scrolling down (swiping up) works again. The same behavior occurs when at the bottom of the .nav container, only in reverse: scrolling down further (swiping up) causes the freeze.

I checked out the timeline to observe and found that this method is getting called excessively during the periods where behavior is “frozen”, but not at all otherwise:

_onWindowScroll = () => {
// It's possible that this scroll operation was triggered by what will be a
// `POP` transition. Instead of updating the saved location immediately, we
// have to enqueue the update, then potentially cancel it if we observe a
// location update.
if (!this._saveWindowPositionHandle) {
this._saveWindowPositionHandle = requestAnimationFrame(
this._saveWindowPosition,
);
}
if (this._windowScrollTarget) {
const [xTarget, yTarget] = this._windowScrollTarget;
const x = scrollLeft(window);
const y = scrollTop(window);
if (x === xTarget && y === yTarget) {
this._windowScrollTarget = null;
this._cancelCheckWindowScroll();
}
}
};

I suspect that this is somewhat expensive, and deferring the action using requestAnimationFrame is amplifying the problem, locking the main thread. I'm not sure what the intent is for this code, and as such, I'm not sure the best course of action to mitigate the issue.

@taion
Copy link
Owner

taion commented Nov 15, 2018

Why would collapsing events with rAF make the problem worse?

And you say this only happens if you have a scrollable element inside a not-otherwise-scrollable window?

@coreyward
Copy link
Author

coreyward commented Nov 15, 2018

As mentioned, I'm not really familiar with scroll-behavior, but as best I can tell the above is true. Happy to provide any other data you need to dive in.

Oh, and just realized what you meant by “rAF” (requestAnimationFrame). Again, don't know what this code is doing, but performing an expensive calculation on every paint means more, not fewer, events. Is it necessary to do this? Is it possible to use a passive event listener?

@coreyward
Copy link
Author

After some more testing I found that the originally described behavior does not occur if the page is not already “docked” at the top or bottom. I.e. if there's a scroll in progress it's completely possible to scroll past the top or bottom of the document with springing action per usual. It's only once the page comes to a rest and a scroll in an unexpected direction (above the top of the document or below the bottom) that the issue comes into play.

I also did some additional rough timing and it appears that the minimum duration between the last interaction and the end of the “frozen” behavior is closer to 200ms than it is the originally reported ~1s. This is still significant since it's the duration from the last interaction, so as long as a user is trying to swipe the page will be locked.

@coreyward
Copy link
Author

Any thoughts on what's happening here, @taion?

@taion
Copy link
Owner

taion commented Nov 27, 2018

I don't really know, sorry. Can you explain your reasoning why requestAnimationFrame would make things worse? It seems strictly better to defer the work – and note the use of the callback collapses events, such that things run at most once per frame.

You could try to add some logging to see what's getting called.

@coreyward
Copy link
Author

I explained previously, but to be clear I don't know if rAF is the primary issue, it just caught my eye. This likely has something to do with the way iOS Safari handles scrolling and scrolling past bounds, and seems specific to scrolling within overflow: [scroll | auto] containers. It may have something to do with the way you are capturing and forcibly setting scroll position, too.

I narrowed in on this specific call to _onWindowScroll being repeatedly called (not debounced or throttled) via the timeline recorder/debugger. You can observe the same behavior here on an iOS device (simulator may work as well, but desktop Safari does not behave the same):

  1. Open the nav menu (icon in the top right)
  2. Click on the search icon and search for “design”
  3. Swipe down (scroll up) while at the top of the page, then immediately try to go the other direction
  4. The page will not respond to the input from the previous step

If you record this interaction you'll see something like this:

screen shot 2018-11-27 at 10 06 27 am

The calls shown in purple occur only during the “freeze” scenario; once it alleviates, scrolling normally produces the composites shown in green in the second grouping where there are no calls to the _onWindowScroll method. A single cycle of this looks like this:

screen shot 2018-11-27 at 10 14 21 am

@taion
Copy link
Owner

taion commented Nov 28, 2018

I'm not seeing this on the Figma search on my phone.

Note also that we're not setting scroll position in the _saveWindowPosition callback. We're just saving the current position to local storage. We only update the window scroll position on something like a triggered navigation event.

Again, the point of the rAF call is exactly to defer and debounce these updates. You can test a more aggressive debouncing scheme if you'd like, but I'm not seeing the issue you're reporting.

@coreyward
Copy link
Author

Hmm, just to confirm, you went into the search and tried scrolling in the results on iOS and didn't see the same behavior? I can reproduce this reliably (100% success) on multiple iOS devices (iPhone XS Max, iPhone 8+, iPad Pro 12.9"), all relatively up to date.

@coreyward
Copy link
Author

coreyward commented Nov 28, 2018

I recorded a quick video to show you what I mean. It doesn't show taps, but the sequence of events is: open nav, tap into search, type “desi”, tap “Done”, try to scroll up and then repeatedly try to scroll down; pause for a moment, then scroll down and back up (past the top). Pause again and repeat the scroll up and then down repeatedly motion.

https://streamable.com/zoupu

@taion
Copy link
Owner

taion commented Nov 28, 2018

Not seeing it on my iPhone X: https://streamable.com/fb7me.

In any event, I'm happy to leave this issue open for you or others to further pursue what's happening, but I'm not going to pursue this further at the moment.

@coreyward
Copy link
Author

You didn’t pause at the top before scrolling up so the event never fired.

@macoughl
Copy link

Hey @tiaon, I work at Figma and I'm experiencing the same issue described by @coreyward. It's happening to me on an iPhoneX and iPhone8 on Safari

@slk333
Copy link

slk333 commented Jan 28, 2019

the problem is back with iOS 12.2 Beta, but maybe it will be fixed by Apple..

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

No branches or pull requests

4 participants