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

Position should reset after route changes #1655

Closed
v4n opened this Issue Feb 21, 2017 · 25 comments

Comments

Projects
None yet
7 participants
@v4n

v4n commented Feb 21, 2017

I don't believe this was happening in v0.2 but in v1 when a route changes the page is opened from the previous page scroll position, that's bad UX.

Should probably be solvable with:

history.listen(_ => {
    window.scrollTo(0, 0)
})
@ycadaner-merkos302

This comment has been minimized.

ycadaner-merkos302 commented Feb 21, 2017

It's a rather opinionated decision on the router's part. There are times it's undesired, like when paning a sub-section of a page

@v4n

This comment has been minimized.

v4n commented Feb 21, 2017

@ycadaner-merkos302 would it be reasonable to say when a <a oncreate={m.route.link}></a> is clicked the behaviour should be just like a <a></a> with exception oncreate={m.route.link} causes a view change rather a full page load.

@ycadaner-merkos302

This comment has been minimized.

ycadaner-merkos302 commented Feb 21, 2017

@v4n If the url is saving state, say photo id in a gallery, this wouldn't be the desired behavior.

@barneycarroll

This comment has been minimized.

Member

barneycarroll commented Feb 21, 2017

This is a chronic problem. In traditional websites, getting back to the position where you left the last page was a very useful feature, but with modern single page applications where Javascript controls most aspects of the user experience it's less straightforward whether the behaviour is desirable or not.

When the HTML5 History API was implemented, Chrome and Firefox presented a difficulty in reliably preventing this behaviour because they triggered the scroll reset at different times, and it was impossible to reliably work out when this happened.

There's a host of Stackoverflow Q&As and libraries out there aiming to find solutions to this problem — Googling for "popstate prevent scroll" yields some promising results…

@v4n

This comment has been minimized.

v4n commented Feb 21, 2017

@ycadaner-merkos302 if a link with href="#image-4 and oncreate={m.route.link} acts exactly like a normal link then indeed scroll to top shouldn't happen (unless there is an element with image-4 id at the top). That's what I mean by my previous comment.

@v4n

This comment has been minimized.

v4n commented Feb 21, 2017

@barneycarroll imo about 99% of the cases when navigating to a new page it doesn't make sense for scroll to start from the previous page state. Reload yes. But '/home' to '/about' makes no sense.

It's like going through a book and each time you move to a new page you start reading at the middle of it.

Any thoughts on why this behaviour didn't occurred on v0.2?

@pygy

This comment has been minimized.

Member

pygy commented Feb 21, 2017

There was a global.scrollTo(0, 0)hardcoded on route. Some people complained about it or picked another framework because it was unwanted.

If it bothers you you can wrap m.route.set and m.route.link to do it for you.

We may want to add a note in the migration guide about this.

@v4n

This comment has been minimized.

v4n commented Feb 21, 2017

@pygy what scenarios did people complained about the global.scrollTo(0, 0)?

I can only think of anchor links and updates to the url without causing a view change, both which can be achieved without the usage of m.route.link.

I imagine the scenarios are less common than navigating a page. If so I don't think Mithril should make people have to monkey patch to deal with a very common situation.

@tivac

This comment has been minimized.

Member

tivac commented Feb 21, 2017

@inta

This comment has been minimized.

inta commented Feb 21, 2017

I don't think resetting the scroll position is that common for SPA. You might repeatedly want to change the URL and only parts of the app/page (I needed that several times and it was quite a pain with v0.2). Changing the URL without the router is not a nice option if you need some components to get initialized.

@v4n

This comment has been minimized.

v4n commented Feb 21, 2017

@tivac this is more subjective but feels like scroll position when navigating different pages is more important to handle off the bat than:

  • back/forwards button scroll state is lost
  • hash usage with infinite scrolling with two page sections that have different scrolling points

@inta interesting. So if I understand correctly you had two views where only part of the pages were different - componentB <-> componentC in the following example.

view1 = <div>
  <componentA></componentA>
  <componentB></componentB>
</div>
view2 = <div>
  <componentA></componentA>
  <componentC></componentC>
</div>

And when the route changed between view1 and view2 you wanted the scroll position to be in the same place?

@inta

This comment has been minimized.

inta commented Feb 22, 2017

@v4n correct, the second component is something like an overlay with its own scroll area which does not entirely cover the first component. One oft those second components is for the detail view and another for the legal notice. The first one is the main content with a list things, where you don't want to loose focus while viewing some details. I think the user experience is quite nice, so there should be at least an option to route without scroll to top.

@v4n

This comment has been minimized.

v4n commented Feb 22, 2017

@inta aha makes sense.

In my case I use history push state to change the url when a modal is opened and since our main layout has <main>{window.modal ? window.modal : ''}</main> I simply add the modal component to window.modal when needed.

Your case might be more complex so I can understand the need of wanting to avoid scroll to top.

It would be nice having something like:

  • m.route.link which is used when linking to new pages and scrolls to top
  • m.route.update update existing page and don't scroll to top

If we don't provide fix out the box then we just increase the amount of things people need to do to get Mithril working reasonably on an app. Do you imagine navigating stackoverflow or twitter while the scroll position gets restored to the one of the previous page.

@ycadaner-merkos302

This comment has been minimized.

ycadaner-merkos302 commented Feb 22, 2017

I would expect a flag. Perhaps adding a boolean scrollToTop flag to m.route.link

@inta

This comment has been minimized.

inta commented Feb 22, 2017

I think the right position to configurate this should be route setup, not the trigger/link. I can't think oft a situation where you sometimes want to scroll to top and sometimes not. That would be really confusing for the user.

@pygy

This comment has been minimized.

Member

pygy commented Feb 22, 2017

An what happens when route resolution is deferred?

This IMO would best be handled as a routeResolver wrapper that injects a new- or wraps an existing onmatch hook.

m.route(root, '/', {
  '/': scrollTop(RootComponent)
}

With logic in scrollTop to deal with components, routeResolvers with and without an onmatch hook.

@inta

This comment has been minimized.

inta commented Feb 22, 2017

That's somehow what I meant with "route setup", though I'm not yet familiar with the v1 routing and did not know what the right hook would be.

@barneycarroll

This comment has been minimized.

Member

barneycarroll commented Feb 22, 2017

@pygy I'll defer to your experience on this but based on my attempts to paper over the differences between route components and route resolvers, the idea of a wrapper that patches consistent view lifecycle behaviour into any given route endpoint sounds terrifying if not impossible to write.

Besides, when @inta says

I think the right position to configurate this should be route setup

That chimes with me in that this behaviour should really be something that sits at an app-wide level, above individual routes (which are stateless inasmuch as they are ignorant of previous route resolutions). Besides, composing routes quickly gets cumbersome and difficult to rationalise as more concerns are forced into route endpoints, eg profile : ScrollReset( MainLayout( Authenticate( ProfileComponentOrResolver ) ).

I find a better pattern for execugting side effects in time with Mithril's lifecycle is to mount a component outside of the document and encapsulate the logic in a way that doesn't require patching orthogonal parts of the application:

m.mount(
  // Don't attach to the document
  document.createDocumentFragment(),
  {
    // We need a valid view for Mithril to behave
    view : () => '',

    // Will execute on the DOM ready phase of every draw
    onupdate(){
      const route = m.route.get()

      if(route != this.route)
        scrollTo(0, 0)

      this.route = route
    }
  }
)

// Route definitions are separate from routing behaviour configuration
m.route(/* ... */)
@pygy

This comment has been minimized.

Member

pygy commented Feb 22, 2017

Something along this will go a long way (bring your own Object.assign(), and beware, this was typed in the GitHub text box, not tested at all).

// always return a new RouteResolver with both onmatch and render
function identity(vnode) {return vnode}
var noop = Function.prototype
function normalizeRoute(endpoint) {
  if (endpoint == null || typeof endpoint !== 'function' && typeof endpoint !== 'object') {
    throw new error ("Component or RouteResolver expected")
  }
  if (typeof endpoint === 'function' || typeof endpoint.view === 'function') {
    return {onmatch: function() {return endpoint}, render: identity}
  } else {
    return = Object.assign(Object.create(endpoint), {
      onmatch: typeof endpoint.onmatch === 'function' ? endpoint.onmatch : noop,
      render: typeof endpoint.render === 'function' ? endpoint.render : identity
    })
  }
}

// then

function scrollTop(endpoint) {
  endpoint = normalizeRoute(endpoint)
  var onmatch = endpoint.onmatch
  endpoint.onmatch = function() {
    return Promise.resolve(onmatch.apply(endpoint, arguments))
      .then(function(){window.scrollTo(0, 0)})
  }
  return endpoint
}
@v4n

This comment has been minimized.

v4n commented Feb 22, 2017

@barneycarroll based on your proposal if I had to add the 5 lines logic to our app's views (roughly 31) we are talking about 155 additional lines of code to handle an expected behaviour when navigating to a new page.

We could DRY things up and reduce it to 2-3 lines. But when creating views there is high likelihood handling this will be overlooked.

@pygy leaving it up to people to add a routeResolver for a behaviour expected for usable apps (which other libraries fail at helping with) feels like a bad way to resolve this.

Either people will have to come up with a similar code you wrote.
Or google, dig through docs/GH issues, find the code, and copy paste into their codebase. All of that just so that page navigation works as one would expect.

@ycadaner-merkos302

This comment has been minimized.

ycadaner-merkos302 commented Feb 22, 2017

This approach is maintaining that a url will always either desire a scrollToTop or will not. However, sometimes the transition to the new url will scrollToTop and sometimes not. I think this should be at the discretion of the invoker of the request.

Truth be told, we can have all transitions scrollToTop, and augment m.route.set to m.route.set('url', 'preventScroll'). This will allow to attach an onclick event to an <a></a> and override the default for m.route.link.

@pygy

This comment has been minimized.

Member

pygy commented Feb 22, 2017

@v4n these could become NPM packages (both normalizeRoute and scrollTop, and the latter could be mentioned in the migration guide).

@ycadaner-merkos302 You can modify the scrollToTop wrapper above to work conditionally. You'd need to wrap m.route.set and m.route.link as well and have all three wrappers communicate (a common variable in the parent closure would do).

@v4n

This comment has been minimized.

v4n commented Feb 23, 2017

@pygy would the RouteResolver solution force a view to always start from the top including reload and going back?

Making the scroll decision when using m.route.link would make scrolling behaviour work as expected for all scenarios: new page, reload, back button and the more complex scenarios as mentioned in the previous comments.

@pygy

This comment has been minimized.

Member

pygy commented Feb 23, 2017

For delayed resolution, the scrollTo(0, 0) call must happen after the resolution completes otherwise the old page will scroll up when you click the link and wait while the new page data is loading. During that time, the user would be free to scroll down again. Bad UX/UI.

You probably want something like this:

var shouldScrollToTop = false

function scrollTop(endpoint) {
  endpoint = normalizeRoute(endpoint)
  var onmatch = endpoint.onmatch
  endpoint.onmatch = function() {
    return Promise.resolve(onmatch.apply(endpoint, arguments))
      .then(function(){
        if(shouldScroll) {
          shouldScrollToTop = false
          window.scrollTo(0, 0)
        }
      })
  }
  return endpoint
}

m.route.linkTop = function() {
  shouldScrollToTop = true
  return m.route.link.apply(this, arguments)
}

m.route.setTop = function() {
  shouldScrollToTop = true
  return m.route.set.apply(this, arguments)
}

Possibly with wrappers around the real set and link that set shouldScrollToTop to false explicitly,

Possible caveat re. picking back the page position on reload/coming back to the page: the presence of onmatch on routeResolvers currently triggers an async first render (that's why the normalizeRoute helper here doesn't bother telling Promises and other values apart and just wraps the value of the previous onmatch in Promise.resolve().

I don't know if that allows the page to pick back its old position if the first paint occurs after DOMContentLoaded.

@isiahmeadows

This comment has been minimized.

Collaborator

isiahmeadows commented Mar 27, 2017

Closing, since this issue has not been updated in over a month. If you feel something in Mithril needs added or changed, please file a new issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment