Enhance scrolling behavior #111
@@ -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.
|
||||
|
||||
##### `scrollRestoration` (Boolean, default true)
|
||||
|
||||
When set to true, attempt to restore the scroll position when navigating backwards or forwards.
|
||||
|
||||
##### `cacheBust` (Boolean, default true)
|
||||
|
||||
When set to true,
|
||||
|
||||
72
index.js
@@ -13,7 +13,6 @@ var defaultSwitches = require("./lib/switches")
|
||||
|
||||
|
||||
var Pjax = function(options) {
|
||||
this.firstrun = true
|
||||
this.state = {
|
||||
numPendingSwitches: 0,
|
||||
href: null,
|
||||
@@ -35,6 +34,7 @@ var Pjax = function(options) {
|
||||
opt.title = st.state.title
|
||||
opt.history = false
|
||||
opt.requestOptions = {};
|
||||
opt.scrollPos = st.state.scrollPos
|
||||
if (st.state.uid < this.lastUid) {
|
||||
opt.backward = true
|
||||
}
|
||||
@@ -160,9 +160,21 @@ Pjax.prototype = {
|
||||
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.
|
||||
document.activeElement.blur()
|
||||
|
||||
var oldHref = href
|
||||
if (request.responseURL) {
|
||||
if (href !== request.responseURL) {
|
||||
href = request.responseURL
|
||||
@@ -174,6 +186,17 @@ Pjax.prototype = {
|
||||
else if (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.options = clone(options)
|
||||
|
||||
@@ -218,13 +241,13 @@ Pjax.prototype = {
|
||||
var state = this.state
|
||||
|
||||
if (state.options.history) {
|
||||
if (this.firstrun) {
|
||||
if (!window.history.state) {
|
||||
this.lastUid = this.maxUid = newUid()
|
||||
|
|
||||
this.firstrun = false
|
||||
window.history.replaceState({
|
||||
url: window.location.href,
|
||||
title: document.title,
|
||||
uid: this.maxUid
|
||||
uid: this.maxUid,
|
||||
scrollPos: [0, 0]
|
||||
},
|
||||
document.title)
|
||||
}
|
||||
@@ -235,7 +258,8 @@ Pjax.prototype = {
|
||||
window.history.pushState({
|
||||
url: state.href,
|
||||
title: state.options.title,
|
||||
uid: this.maxUid
|
||||
uid: this.maxUid,
|
||||
|
This will only run if Perhaps something like instead? This would require the default value of 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?
No, I believe my code is correct. So we only want to use the scroll position stored in history when we're responding to a 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.
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 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.
I'm not even sure why the check for I think we should add a `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).
Yes, I wondered why it hadn't been documented. Adding a Yes, I wondered why it hadn't been documented.
Adding a `scrollRestoration` option sounds fine though.
|
||||
scrollPos: [0, 0]
|
||||
},
|
||||
state.options.title,
|
||||
state.href)
|
||||
@@ -250,15 +274,41 @@ Pjax.prototype = {
|
||||
|
||||
state.options.analytics()
|
||||
|
||||
// Scroll page to top on new page load
|
||||
if (state.options.scrollTo !== false) {
|
||||
if (state.options.scrollTo.length > 1) {
|
||||
window.scrollTo(state.options.scrollTo[0], state.options.scrollTo[1])
|
||||
if (state.options.history) {
|
||||
// First parse url and check for hash to override scroll
|
||||
var a = document.createElement("a")
|
||||
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 {
|
||||
window.scrollTo(0, state.options.scrollTo)
|
||||
else if (state.options.scrollTo !== false) {
|
||||
// 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 = {
|
||||
numPendingSwitches: 0,
|
||||
|
||||
@@ -24,6 +24,7 @@ module.exports = function(options) {
|
||||
this.options.cacheBust = (typeof this.options.cacheBust === "undefined") ? true : this.options.cacheBust
|
||||
this.options.debug = this.options.debug || false
|
||||
this.options.timeout = this.options.timeout || 0
|
||||
this.options.scrollRestoration = (typeof this.options.scrollRestoration !== "undefined") ? this.options.scrollRestoration : true
|
||||
|
||||
// we can’t replace body.outerHTML or head.outerHTML
|
||||
// it create a bug where new body or new head are created in the dom
|
||||
|
||||
@@ -53,6 +53,7 @@ tape("test parse initalization options function", function(t) {
|
||||
t.deepEqual(body1.options.scrollTo, 0);
|
||||
t.deepEqual(body1.options.cacheBust, true);
|
||||
t.deepEqual(body1.options.debug, false);
|
||||
t.deepEqual(body1.options.scrollRestoration, true)
|
||||
t.end();
|
||||
});
|
||||
|
||||
|
||||
This also should be
[0, 0], per my other commentGood point.