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

Avoid error calling addEventListener in browsers that don't support it #174

Merged
merged 1 commit into from Sep 6, 2016

Conversation

Projects
None yet
4 participants
@javan
Member

javan commented Sep 3, 2016

Fix for #173

window.history.pushState? and window.requestAnimationFrame?
window.history.pushState? and
window.requestAnimationFrame? and
window.addEventListener?

This comment has been minimized.

@javan

javan Sep 3, 2016

Member

Not technically needed to fix the issue, but seems reasonable to add for clarity.

@pageIsLoaded()
pageIsLoaded: ->
@pageLoaded or document.readyState is "complete"

This comment has been minimized.

@javan

javan Sep 3, 2016

Member

Added an additional check for document.readyState to cover the theoretical case where turbolinks is loaded async after the load event.

This comment has been minimized.

@nateberkopec

nateberkopec Sep 3, 2016

Contributor

👍 Similar to what JQuery does.

@javan javan merged commit c73e134 into master Sep 6, 2016

@javan javan deleted the fix-ie8-error branch Sep 6, 2016

@jmckible jmckible referenced this pull request Oct 14, 2016

Closed

Minor version bump #197

@printercu

This comment has been minimized.

printercu commented Mar 10, 2017

Is there plan to release this fix? I've found that addEventListener("load", ...) is not firing event sometimes in iOS simulator, so history.back() doesn't work. This patch seems to fix the issue.

@dennisreimann

This comment has been minimized.

dennisreimann commented on src/turbolinks/history.coffee in e90e4c4 Aug 23, 2017

Can you elaborate on the introduction of or document.readyState is "complete" with this commit? I'm seeing this break the back button functionality in Safari (at least in version 10), as document.readyState is "complete" is true when navigating back, which leads to the handling of popstate and thus loading the page twice.

This comment has been minimized.

Member

javan replied Aug 23, 2017

If you're navigating back, then presumably your JavaScript and Turbolinks have already been loaded. That means @pageLoaded will be true and the document.readyState half of the condition won't matter.

Turbolinks should be handling popstate when navigating back.. Can you explain what "loading the page twice" means in your case?

This comment has been minimized.

dennisreimann replied Aug 23, 2017

In this concrete case the user navigates away from a page that was handled via Turbolinks. The user navigates to the next page via a non-Turbolinks link. When the back button is hit, the user gets back to the page that contains Turbolinks. This page is already set up and can be shown from the cache – the user also lands at the correct scroll position when going back. (Everything is fine until then)

The page the user navigated back to does not have set @pageLoaded (it is undefined in this case), but its document.readyState is "complete". This leads to the page handling the popstate, which triggers re-rendering and kicks the user back to the top of the page.

This only occurs in Safari (desktop and mobile), Firefox and Chrome (current versions) do not receive the popstate call in this case.

This comment has been minimized.

Member

javan replied Aug 23, 2017

Got it. Mind opening an issue with those details? (Or open a PR that fixes it 😬)

This comment has been minimized.

dennisreimann replied Aug 23, 2017

Yep, I'll take care of this tomorrow. Thanks for providing feedback so quickly!

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