Enhance scrolling behavior #111

Merged
BehindTheMath merged 4 commits from fix/scroll into master 2018-01-22 20:32:58 -05:00
4 changed files with 67 additions and 11 deletions

View File

@@ -377,6 +377,10 @@ It's called every time a page is switched, even for history buttons.
Value (in px) to scrollTo when a page is switched. Value (in px) to scrollTo when a page is switched.
##### `scrollRestoration` (Boolean, default true)
When set to true, attempt to restore the scroll position when navigating backwards or forwards.
##### `cacheBust` (Boolean, default true) ##### `cacheBust` (Boolean, default true)
When set to true, When set to true,

View File

@@ -13,7 +13,6 @@ var defaultSwitches = require("./lib/switches")
var Pjax = function(options) { var Pjax = function(options) {
this.firstrun = true
this.state = { this.state = {
numPendingSwitches: 0, numPendingSwitches: 0,
href: null, href: null,
@@ -35,6 +34,7 @@ var Pjax = function(options) {
opt.title = st.state.title opt.title = st.state.title
opt.history = false opt.history = false
opt.requestOptions = {}; opt.requestOptions = {};
opt.scrollPos = st.state.scrollPos
if (st.state.uid < this.lastUid) { if (st.state.uid < this.lastUid) {
opt.backward = true opt.backward = true
} }
@@ -160,9 +160,21 @@ Pjax.prototype = {
return return
} }
// push scroll position to history
var currentState = window.history.state || {}
window.history.replaceState({
url: currentState.url || window.location.href,
title: currentState.title || document.title,
uid: currentState.uid || newUid(),
scrollPos: [document.documentElement.scrollLeft || document.body.scrollLeft,
document.documentElement.scrollTop || document.body.scrollTop]
},
document.title, window.location)
// Clear out any focused controls before inserting new page contents. // Clear out any focused controls before inserting new page contents.
document.activeElement.blur() document.activeElement.blur()
var oldHref = href
if (request.responseURL) { if (request.responseURL) {
if (href !== request.responseURL) { if (href !== request.responseURL) {
href = request.responseURL href = request.responseURL
@@ -174,6 +186,17 @@ Pjax.prototype = {
else if (request.getResponseHeader("X-XHR-Redirected-To")) { else if (request.getResponseHeader("X-XHR-Redirected-To")) {
href = request.getResponseHeader("X-XHR-Redirected-To") href = request.getResponseHeader("X-XHR-Redirected-To")
} }
// Add back the hash if it was removed
var a = document.createElement("a")
a.href = oldHref
var oldHash = a.hash
a.href = href
if (oldHash && !a.hash) {
a.hash = oldHash
href = a.href
}
this.state.href = href this.state.href = href
this.state.options = clone(options) this.state.options = clone(options)
@@ -218,13 +241,13 @@ Pjax.prototype = {
var state = this.state var state = this.state
if (state.options.history) { if (state.options.history) {
if (this.firstrun) { if (!window.history.state) {
this.lastUid = this.maxUid = newUid() this.lastUid = this.maxUid = newUid()
robinnorth commented 2018-01-22 16:26:00 -05:00 (Migrated from github.com)
Review

This also should be [0, 0], per my other comment

This also should be `[0, 0]`, per my other comment
BehindTheMath commented 2018-01-22 17:26:41 -05:00 (Migrated from github.com)
Review

Good point.

Good point.
this.firstrun = false
window.history.replaceState({ window.history.replaceState({
url: window.location.href, url: window.location.href,
title: document.title, title: document.title,
uid: this.maxUid uid: this.maxUid,
scrollPos: [0, 0]
}, },
document.title) document.title)
} }
@@ -235,7 +258,8 @@ Pjax.prototype = {
window.history.pushState({ window.history.pushState({
url: state.href, url: state.href,
title: state.options.title, title: state.options.title,
uid: this.maxUid uid: this.maxUid,
robinnorth commented 2018-01-22 16:31:15 -05:00 (Migrated from github.com)
Review

This will only run if state.options.history is falsy, which I don't think is what you intended?

Perhaps something like

else if (state.options.scrollTo !== false) {
  // Update window scroll position
  if (Array.isArray(state.options.scrollTo)) {
    window.scrollTo(state.options.scrollTo[0], state.options.scrollTo[1])
  }
  else if (typeof state.options.scrollTo === "number") {
    window.scrollTo(0, state.options.scrollTo)
  }
  else {
    window.scrollTo(state.options.scrollPos[0], state.options.scrollPos[1])
  }
}

instead?

This would require the default value of this.options.scrollTo to change to true as well -- currently it's set to 0 if one is not provided by the user, meaning that the window will always be scrolled back to the top. If we didn't want to break backwards compatibility, then we could accept 'auto' as a value for the option to trigger scroll restoration, but it's probably a behavior that users would expect and prefer to happen -- what do you think?

This will only run if `state.options.history` is falsy, which I don't think is what you intended? Perhaps something like ```js else if (state.options.scrollTo !== false) { // Update window scroll position if (Array.isArray(state.options.scrollTo)) { window.scrollTo(state.options.scrollTo[0], state.options.scrollTo[1]) } else if (typeof state.options.scrollTo === "number") { window.scrollTo(0, state.options.scrollTo) } else { window.scrollTo(state.options.scrollPos[0], state.options.scrollPos[1]) } } ``` instead? This would require the default value of `this.options.scrollTo` to change to `true` as well -- currently it's set to `0` if one is not provided by the user, meaning that the window will always be scrolled back to the top. If we didn't want to break backwards compatibility, then we could accept `'auto'` as a value for the option to trigger scroll restoration, but it's probably a behavior that users would expect and prefer to happen -- what do you think?
BehindTheMath commented 2018-01-22 17:31:56 -05:00 (Migrated from github.com)
Review

No, I believe my code is correct. options.history is true when we're navigating to a page, and false when the initiator was popstate. This is because you don't want to call pushState() when you're in middle of a popstate event.

So we only want to use the scroll position stored in history when we're responding to a popstate event, in which case options.history is false. Otherwise, when it's true, we want to either use the hash, or scrollTo.

See earlier in #46, where we decided the default when clicking a link should be to scroll to the top.

No, I believe my code is correct. `options.history` is true when we're navigating to a page, and false when the initiator was `popstate`. This is because you don't want to call `pushState()` when you're in middle of a `popstate` event. So we only want to use the scroll position stored in history when we're responding to a `popstate` event, in which case `options.history` is false. Otherwise, when it's true, we want to either use the hash, or `scrollTo`. See earlier in #46, where we decided the default when clicking a link should be to scroll to the top.
robinnorth commented 2018-01-22 18:18:31 -05:00 (Migrated from github.com)
Review

Oh dear, that's a comprehension fail on my part, sorry! You are correct.

One thing that would be good to change though is to only restore the scroll position on popstate if this.options.scrollTo is not false -- another thing from #46 was a decision to let developers to decide on what is the best UX for their app, so we should make sure that this scroll restoration can be opted out of, don't you think? This matches the current behaviour when switching.

Oh dear, that's a comprehension fail on my part, sorry! You are correct. One thing that would be good to change though is to only restore the scroll position on popstate if `this.options.scrollTo` is not `false` -- another thing from #46 was a decision to let developers to decide on what is the best UX for their app, so we should make sure that this scroll restoration can be opted out of, don't you think? This matches the current behaviour when switching.
BehindTheMath commented 2018-01-22 18:26:16 -05:00 (Migrated from github.com)
Review

options.scrollTo is only documented as taking a number value. From the README:

scrollTo (Integer, default to 0)

I'm not even sure why the check for options.scrollTo !== false exists at all.

I think we should add a scrollRestoration option instead, similar to the HTML spec (see #40).

`options.scrollTo` is only documented as taking a `number` value. From the README: > `scrollTo` (Integer, default to 0) I'm not even sure why the check for `options.scrollTo !== false` exists at all. I think we should add a `scrollRestoration` option instead, similar to the HTML spec (see #40).
robinnorth commented 2018-01-22 18:42:46 -05:00 (Migrated from github.com)
Review

Yes, I wondered why it hadn't been documented.

Adding a scrollRestoration option sounds fine though.

Yes, I wondered why it hadn't been documented. Adding a `scrollRestoration` option sounds fine though.
scrollPos: [0, 0]
}, },
state.options.title, state.options.title,
state.href) state.href)
@@ -250,15 +274,41 @@ Pjax.prototype = {
state.options.analytics() state.options.analytics()
// Scroll page to top on new page load if (state.options.history) {
if (state.options.scrollTo !== false) { // First parse url and check for hash to override scroll
if (state.options.scrollTo.length > 1) { var a = document.createElement("a")
window.scrollTo(state.options.scrollTo[0], state.options.scrollTo[1]) a.href = this.state.href
if (a.hash) {
var name = a.hash.slice(1)
name = decodeURIComponent(name)
var curtop = 0
var target = document.getElementById(name) || document.getElementsByName(name)[0]
if (target) {
// http://stackoverflow.com/questions/8111094/cross-browser-javascript-function-to-find-actual-position-of-an-element-in-page
if (target.offsetParent) {
do {
curtop += target.offsetTop
target = target.offsetParent
} while (target)
}
}
window.scrollTo(0, curtop);
} }
else { else if (state.options.scrollTo !== false) {
window.scrollTo(0, state.options.scrollTo) // Scroll page to top on new page load
if (state.options.scrollTo.length > 1) {
window.scrollTo(state.options.scrollTo[0], state.options.scrollTo[1])
}
else {
window.scrollTo(0, state.options.scrollTo)
}
} }
} }
else if (state.options.scrollRestoration && state.options.scrollPos) {
window.scrollTo(state.options.scrollPos[0], state.options.scrollPos[1])
}
this.state = { this.state = {
numPendingSwitches: 0, numPendingSwitches: 0,

View File

@@ -24,6 +24,7 @@ module.exports = function(options) {
this.options.cacheBust = (typeof this.options.cacheBust === "undefined") ? true : this.options.cacheBust this.options.cacheBust = (typeof this.options.cacheBust === "undefined") ? true : this.options.cacheBust
this.options.debug = this.options.debug || false this.options.debug = this.options.debug || false
this.options.timeout = this.options.timeout || 0 this.options.timeout = this.options.timeout || 0
this.options.scrollRestoration = (typeof this.options.scrollRestoration !== "undefined") ? this.options.scrollRestoration : true
// we cant replace body.outerHTML or head.outerHTML // we cant replace body.outerHTML or head.outerHTML
// it create a bug where new body or new head are created in the dom // it create a bug where new body or new head are created in the dom

View File

@@ -53,6 +53,7 @@ tape("test parse initalization options function", function(t) {
t.deepEqual(body1.options.scrollTo, 0); t.deepEqual(body1.options.scrollTo, 0);
t.deepEqual(body1.options.cacheBust, true); t.deepEqual(body1.options.cacheBust, true);
t.deepEqual(body1.options.debug, false); t.deepEqual(body1.options.debug, false);
t.deepEqual(body1.options.scrollRestoration, true)
t.end(); t.end();
}); });