Delay calling pushState() until the xhr succeeds #334

AlexHill · 2013-12-06T10:08:24Z

With this change the address bar only changes once the pjax request has come back successfully. This stops the incorrect HTTP_REFERER from being sent with fallback requests.

Have also added a test for this and re-jigged some of the existing tests to match the new behaviour.

Fixes #319

AlexHill · 2013-12-06T10:15:38Z

jquery.pjax.js

@@ -451,7 +457,7 @@ function onPjaxPopstate(event) {
      // scroll position.
      container[0].offsetHeight
    } else {
-      locationReplace(location.href)
+      hardLoad(location.href, true)


It's not clear to me when this branch would be reached, but since it's in the popstate handler I'm assuming window.location has already been updated. This is (I think) the only place where it's necessary to hard load the page without pushing the history stack.

AlexHill · 2013-12-06T10:32:52Z

cc @josh

Pym · 2013-12-12T20:25:22Z

jquery.pjax.js

+    window.history.replaceState(null, "", "#")
+    window.location.replace(url)
+  } else{
+      window.location.assign(url)


I see 2 extra spaces here.

Otherwise, this is a great modification IMHO!

AlexHill · 2013-12-12T23:06:38Z

Nice catch @Pym, fixed.

AlexHill · 2014-04-15T07:03:28Z

@defunkt or @josh - any chance you could take a quick look at this PR? I'd like to get it in while it still merges cleanly.

AlexHill · 2014-05-27T08:24:10Z

Paging @mislav - would really appreciate a glance over this change! All tests pass with this patch applied to latest master.

mislav · 2014-05-27T15:35:50Z

Not sure if we want to proceed with this. The changes seem plentiful and not compatible with how pjax was previously designed.

I think that the reason why pjax changes the URL immediately is beause that's what browsers actually do on native requests. Then, when you're on a bad network and your response fails, you can force reload on that new URL.

Your endless redirect loop with HTTP_REFERER is an unfortunate side-effect of your own application's design, I'm afraid.

AlexHill · 2014-05-27T16:39:22Z

Thanks for having a look at this! I'll try to make my case.

The changes seem plentiful

I've renamed locationReplace to hardLoad, which creates a lot of noise in the patch. Happy to revert that change if it would help?

I think that the reason why pjax changes the URL immediately is beause that's what browsers actually do on native requests.

Firefox and Chrome don't update the address bar until they get a response, so jquery-pjax with this patch is much closer to native behaviour than without it in these browsers. This mismatch is the original reason #319 was opened.

I notice that Safari does update before receive a response, though. Can't test IE right now, sorry.

Then, when you're on a bad network and your response fails, you can force reload on that new URL.

True, though PJAX will hard load the URL on failure, with the same outcome.

Your endless redirect loop with HTTP_REFERER is an unfortunate side-effect of your own application's design, I'm afraid.

My redirect loop is one example of what can go wrong, but all server-side code that makes use of HTTP_REFERER is affected. Right now, when a request fails and jquery-pjax initiates a hard load, the server will always receive the incorrect value for this header.

Thanks again for your time, really appreciate it.

josh · 2014-05-27T18:24:48Z

The current pushState timing is very deliberate. It determines when the page is screen shotted for back button animations.

👎

AlexHill · 2014-05-28T04:08:27Z

Would you mind elaborating on that a little? The problem isn't clear to me. If you describe what this will break I will add a failing test and see if the patch can be modified to pass it while still fixing the bug.

AlexHill · 2014-05-28T04:17:10Z

Testing in IE shows the same native loading behaviour as Chrome and Firefox: the address bar isn't updated until the response is received.

To sum up my position, I feel that

the HTTP_REFERER issue is a genuine bug, and
fixing it has the positive side-effect of more closely mimicking native loading behaviour in most browsers.

mislav · 2014-05-28T12:07:21Z

Please follow the following URL in Chrome or Firefox: http://dobrowserschangetheurlwithoutresponse.com/not/really/

Did you receive a response? Probably not, since I made the site up. Did the browser change the URL? I'm seeing yes.

AlexHill · 2014-05-28T13:50:00Z

Hmm, yes, it does. I think it changes when DNS resolution fails, so pretty much instantly.

Try this link with 5s delay added to see what I mean. Safari changes the URL straight away, but Chrome, Firefox and IE don't until the five seconds are up.

mislav · 2014-05-30T05:56:50Z

Hm, that's interesting. Chrome indeed doesn't change the URL until delay is up.

It's true that DNS resolution above caused it to fail fast. However, just because Chrome and Firefox implement some behavior and not Safari, I'm still not convinced that we should match the implementation details of those browsers.

I've just realized that the HTTP_REFERER for fallback requests bug you've described indeed sounds like broken behavior. Do you think it would work if we explicitly set the referrer header with the old URL for fallback requests? Could you maybe try submitting a PR for that first?

AlexHill · 2014-06-04T06:06:34Z

Sorry it's taken a while to follow this up.

Do you think it would work if we explicitly set the referrer header with the old URL for fallback requests?

Should work, but since the fallback page load isn't an XHR I don't think you can explicitly set the header – I think we would have to call history.replaceState with the original URL before calling location.replace.

That said, while testing I've come across a few more issues that are also resolved by delaying pushState. These issues arise when clicking back while a PJAX request is in progress.

Say you're on page n. You click a link to n+1 and then change your mind and click back, before n+1 has loaded. All browsers' native behaviour here is to take you back to page n-1. But with PJAX, there are a few issues:

You return to page n instead of n-1.
Clicking back didn't cancel the pending XHR, so the response arrives and PJAX takes you to page n+1 anyway.
At that point, because replaceState was called in the XHR success handler, clicking back will take you not to n but to n-1. Page n has been removed from your history.

These can all be solved by

moving both cachePush and pushState into the XHR success handler, and
aborting any existing XHR at the top of onPjaxPopstate.

I will update this PR with these changes and tests to demonstrate the behaviour. Please let me know if anything needs clarification, or if you'd like any of these changes split into separate pull requests, etc.

mislav · 2014-06-04T08:57:43Z

On Wed, Jun 4, 2014 at 1:06 PM, Alex Hill [email protected] wrote:

Say you're on page n. You click a link to n+1 and then change your mind
and click back, before n+1 has loaded. All browsers' native behaviour
here is to take you back to page n-1. But with PJAX, there are a few
issues

Yeah, this behavior might be a valid bug of pjax. But I would appreciate if
you fixed such things in separate PRs, so we have bugfixes separate from
changing of features. Fewer implementation changes and more added tests
increases the probabilty for a PR to get merged.

I'm fine for now if the browser Back button returns you from n+1 to n even
if you clicked it before n+1 has finished loading. But, I definitely agree
xhr should be canceled.

AlexHill · 2014-06-05T02:21:57Z

No worries. I've opened a PR just to add an XHR abort to the top of the popstate handler here: #395

AlexHill · 2014-06-05T06:03:39Z

And here's a PR fixing the HTTP referer bug without moving pushState: #397

In preparation for the next commit which will delay pushState until pjax success, here we remove assertions that the address bar changes immediately, and add a failing test of the desired behaviour.

AlexHill · 2015-03-09T09:39:20Z

Hi @mislav, I'd like to restart this discussion and hopefully get this change in. I've just rebased against current master and cleaned up a bit. Since you merged #395 this patch has become much clearer.

See the most recent commit for the actual functionality changes.

To summarise, I think the success handler is the best place for cachePush and pushState. This change

Makes PJAX behaviour mirror browser behaviour in more cases
Fixes the incorrect HTTP_REFERER bug described above
Fixes the back/forward behaviour bug described above
Fixes No event is fired before current contents are cached #451 by calling cachePush after firing pjax:start

Would love to hear your thoughts :)

Alex

mislav · 2015-03-13T08:32:17Z

I'll review when I have a chance; thanks

olemoign · 2015-04-15T10:18:09Z

Sorry @AlexHill, but your PR does not fix #451 (there's still no event fired before caching in popstate events).

AlexHill · 2015-04-15T11:12:58Z

Hi @olemoign, can you confirm you're definitely running my code – no caching issues etc?

Have a look for yourself: the only place cachePush is called is in the AJAX success handler.

Which event are you handling?

olemoign · 2015-04-15T11:24:26Z

Hello @AlexHill. cachePush is not the only function caching the container, cachePop does as well.

So on a popstate event (back/forward), you still can't act on the current container before caching, as no event is fired before the cachePop call.

AlexHill · 2015-04-15T12:43:39Z

Right you are, thanks for the clarification. Didn't read closely enough, sorry :)

I've just tried moving some things around in the popstate handler, so that cachePop() is only called after pjax:popstate and pjax:start (if that is called). Test all passing locally, give it a shot.

AlexHill reviewed Dec 6, 2013
View reviewed changes

Pym reviewed Dec 12, 2013
View reviewed changes

mislav force-pushed the master branch from 216b7f5 to efe7c5c Compare September 15, 2014 20:42

mislav mentioned this pull request Oct 14, 2014

Why not pushState only on 'success'? #418

Closed

mislav changed the title ~~Delay calling pushState() until the xhr succeeds. Fixes #319~~ Delay calling pushState() until the xhr succeeds Oct 14, 2014

mislav mentioned this pull request Oct 14, 2014

Location bar shouldn’t update until success #319

Closed

mislav mentioned this pull request Dec 23, 2014

URL preserving option #405

Closed

AlexHill force-pushed the delay_pushstate branch from 527d2ed to 469040a Compare March 9, 2015 09:09

AlexHill added 5 commits March 9, 2015 17:11

Test for correct HTTP_REFERER after failure

e4a3b0f

Test history when clicking back during load

00b886f

Test that URL only changes on PJAX success

d0031b1

In preparation for the next commit which will delay pushState until pjax success, here we remove assertions that the address bar changes immediately, and add a failing test of the desired behaviour.

Add basic browser history test

c1106b2

Make XHR cancellation test more robust

830ae33

Delay cachePush and pushState until XHR success

186c198

AlexHill force-pushed the delay_pushstate branch from 469040a to 186c198 Compare March 9, 2015 09:11

Move cachePop to after pjax:start and pjax:popstate

b2a0e92

Delay calling pushState() until the xhr succeeds #334

Are you sure you want to change the base?

Delay calling pushState() until the xhr succeeds #334

Uh oh!

Conversation

AlexHill commented Dec 6, 2013

Uh oh!

AlexHill Dec 6, 2013

Choose a reason for hiding this comment

Uh oh!

AlexHill commented Dec 6, 2013

Uh oh!

Pym Dec 12, 2013

Choose a reason for hiding this comment

Uh oh!

AlexHill commented Dec 12, 2013

Uh oh!

AlexHill commented Apr 15, 2014

Uh oh!

AlexHill commented May 27, 2014

Uh oh!

mislav commented May 27, 2014

Uh oh!

AlexHill commented May 27, 2014

Uh oh!

josh commented May 27, 2014

Uh oh!

AlexHill commented May 28, 2014

Uh oh!

AlexHill commented May 28, 2014

Uh oh!

mislav commented May 28, 2014

Uh oh!

AlexHill commented May 28, 2014

Uh oh!

mislav commented May 30, 2014

Uh oh!

AlexHill commented Jun 4, 2014

Uh oh!

mislav commented Jun 4, 2014

Uh oh!

AlexHill commented Jun 5, 2014

Uh oh!

AlexHill commented Jun 5, 2014

Uh oh!

AlexHill commented Mar 9, 2015

Uh oh!

mislav commented Mar 13, 2015

Uh oh!

olemoign commented Apr 15, 2015

Uh oh!

AlexHill commented Apr 15, 2015

Uh oh!

olemoign commented Apr 15, 2015

Uh oh!

AlexHill commented Apr 15, 2015

Uh oh!

Uh oh!