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

window.scroll with options object used in scrollToHash is not supported in IE, Edge, Safari and older versions of Chrome #71

Closed
szimek opened this issue Mar 23, 2019 · 9 comments

Comments

@szimek
Copy link

szimek commented Mar 23, 2019

In scrollToHash function window.scroll method is called with options object. Unfortunately, this syntax is not supported by IE, Edge, Safari and older versions of Chrome (MDN docs), which only support the old syntax, i.e. window.scroll(x-coord, y-coord).

Top 3 JS errors on our site reported by TrackJS are caused by this issue :/ Unfortunately some browsers that don't support it don't throw any error and simply scroll to 0, 0, but e.g. older versions of Chrome (e.g. 55, even though it should work according to MDN) throw the following error:

Failed to execute 'scroll' on 'Window': No function was found that matched the signature provided.
@szimek szimek changed the title window.scroll with options object used in scrollToHash is not supported in IE, Edge and Safari window.scroll with options object used in scrollToHash is not supported in IE, Edge, Safari and older versions of Chrome Mar 23, 2019
@benjaminwood
Copy link

I also ran into this to this when testing on an Android device (chrome browser, I think).

@jamesknelson
Copy link
Collaborator

This is definitely something that needs fixing then. I hadn't realized that such recent browsers didn't support this.

Perhaps the best approach is to just use the old syntax, and leave smooth-scrolling to the user through a scroll-behavior css property. Does this sound reasonable?

@szimek
Copy link
Author

szimek commented Mar 24, 2019

Yeah, I think it’s going to be the simplest solution.

@danielnixon
Copy link

Does iamdustan's smoothscroll polyfill fix this issue in older Chrome, IE, Edge, etc?

If so, you might want to:

  • allow Navi users to specify scroll behavior (with default to auto)
  • document browser compatibility issues with smooth scroll behavior (and recommend smoothscroll polyfill?) should the user opt for smooth scrolling
  • call the old form (window.scroll(x-coord, y-coord)) if behavior is auto (just to avoid passing an object argument in this case)
  • try/catch when behavior is set to smooth and fall back on auto behavior in the catch block

This is basically the approach I think I'm settling on in https://github.com/oaf-project (see https://github.com/oaf-project/oaf-side-effects/blob/master/src/index.ts#L294)

@jamesknelson
Copy link
Collaborator

AFAIK it does fix the issue. This sounds like a good solution. iirc, I already have a configuration option to allow users to switch from smoothscroll to "auto". Flipping it so that auto is the default would make a lot of sense.

@danielnixon
Copy link

You might also consider testing for prefers-reduced-motion with window.matchMedia. If the user has indicated that they prefer reduced motion and a Navi library user (i.e. a developer) has specified smooth scrolling, it might be wise to override the developer and go with auto scrolling.

@jamesknelson
Copy link
Collaborator

I hadn't heard of prefers-reduced-motion before, but that makes a lot of sense too.

I won't have time to implement this for at least a few weeks, but would def be open to a PR.

@remotealex
Copy link

Would love a fix for this soon. Getting errors in production <3

@jamesknelson
Copy link
Collaborator

Navi 0.12.8 will now use the old form of window.scroll by default.

To opt in to smooth scrolling, you have a couple options:

  • pass a hashScrollBehavior='smooth' prop to <Router> or <NaviProvider>
  • wrap your app in a <HashScroll behavior='smooth'> component

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

5 participants