Skip to content
This repository was archived by the owner on Nov 25, 2020. It is now read-only.

router: fix for popstate event on page load in Safari #154

Merged
merged 5 commits into from
Mar 9, 2016

Conversation

alex-ionochkin
Copy link
Contributor

@sinan, can you please review this fix?
Currently we have a problem that Safari fires popstate event after the page is loaded. In some cases it causes double rendering since router handler is called one more time.
This issue is mentioned on MDN (https://developer.mozilla.org/ru/docs/Web/Events/popstate):

Browsers tend to handle the popstate event differently on page load. Chrome (prior to v34) and Safari always emit a popstate event on page load, but Firefox doesn't.

The idea of this fix is taken from here - http://stackoverflow.com/a/18126524. The most important thing here is that Safari fires popstate right after window is loaded. So we catch this moment and skip popstate handling. Note that if popstate event is triggered before page is loaded (for example, page is loading slowly and user clicks Back button) or after that, behavior isn't changed.

/cc @gokmen

@koding-bot koding-bot changed the title router: fix for popstate event on page load in Safari TMS-2476 router: fix for popstate event on page load in Safari Mar 3, 2016
@alex-ionochkin alex-ionochkin changed the title TMS-2476 router: fix for popstate event on page load in Safari router: fix for popstate event on page load in Safari Mar 3, 2016
@gokmen
Copy link
Contributor

gokmen commented Mar 3, 2016

LGTM 👍

@blockPopstateIfWindowLoaded = no
else
@blockPopstateIfWindowLoaded = yes
window.addEventListener 'load', @bound 'handleWindowLoad'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, if you only edit ::startListenining as below it would be cleaner and enough.

startListening: ->

  return no  if @isListening # make this action idempotent

  # Safari fires extra popstate event right after window is loaded
  # this is to avoid this inconsistent initial firing 
  unless document.readyState is 'complete'
    return document.onreadystatechange = =>
      @startListening()  if document.readyState is 'complete'

  @isListening = yes
  window.addEventListener 'popstate', @bound "popState"

  return yes

@sinan
Copy link
Member

sinan commented Mar 3, 2016

👍 nice catch & fix thx @alex-ionochkin

@alex-ionochkin
Copy link
Contributor Author

@sinan, I agree. Your version is more elegant. However, I'd like to add a few changes:

  • KD.utils.defer is still needed when calling @startListening in readystatechange handler. I checked on my dev VM and without it extra popstate event is still called
  • I think it's better to use document.addEventListener() while binding to event - just in case if we need to listen to readystatechange event in another place of code

@alex-ionochkin
Copy link
Contributor Author

PR is updated

return yes
startListening: do ->

readyStateBinded = no
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

readyStateBound

@alex-ionochkin
Copy link
Contributor Author

Fixed

sinan added a commit that referenced this pull request Mar 9, 2016
router: fix for popstate event on page load in Safari
@sinan sinan merged commit 3482e05 into koding:master Mar 9, 2016
@sinan
Copy link
Member

sinan commented Mar 9, 2016

👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants