-
-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
History native calls #3006
Merged
Merged
History native calls #3006
Changes from all commits
Commits
Show all changes
20 commits
Select commit
Hold shift + click to select a range
6ed0cdd
add in addEventListener shim from @wyuenho (0844b00c) to remove jQuery
akre54 1115997
add window.popstate checks for IE7 backwards compat
akre54 0101416
cleanup oldIE sniff code and iframe creation per @jdalton
akre54 5d305bf
remove first window argument from add/removeEventListener
akre54 4ab05ad
check for onpopstate in window instead of window.onpopstate
akre54 573070f
Remove `_oldIE` variable.
braddunbar 87dce15
Pass `useCapture` to `removeEventListener`.
braddunbar 501d7bf
Merge pull request #2 from braddunbar/history
akre54 18e15e2
Remove `isExplorer` RegEx.
braddunbar e5eeea4
Merge pull request #3 from braddunbar/history
akre54 2c50dc5
Check `documentMode` and combine checks.
braddunbar 91ef885
improve inline comments
akre54 feeab3f
Merge pull request #4 from braddunbar/history
akre54 b1bcf0a
Remove `onpopstate` check.
braddunbar a91f6ac
Merge pull request #6 from braddunbar/history
akre54 a717a71
Merge pull request #3011 from braddunbar/clear-interval
jashkenas f9f7131
Add documentation to the iframe approach.
braddunbar dc6a697
Merge pull request #9 from braddunbar/history
akre54 674f101
Create iframe conditional on options and hashchange and pushstate sup…
wyuenho 16b6f1e
Merge pull request #10 from wyuenho/history-native-hooks
akre54 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1334,9 +1334,6 @@ | |
// Cached regex for stripping leading and trailing slashes. | ||
var rootStripper = /^\/+|\/+$/g; | ||
|
||
// Cached regex for detecting MSIE. | ||
var isExplorer = /msie [\w.]+/; | ||
|
||
// Cached regex for removing a trailing slash. | ||
var trailingSlash = /\/$/; | ||
|
||
|
@@ -1391,27 +1388,39 @@ | |
this.options = _.extend({root: '/'}, this.options, options); | ||
this.root = this.options.root; | ||
this._wantsHashChange = this.options.hashChange !== false; | ||
this._hasHashChange = 'onhashchange' in window; | ||
this._wantsPushState = !!this.options.pushState; | ||
this._hasPushState = !!(this.options.pushState && this.history && this.history.pushState); | ||
var fragment = this.getFragment(); | ||
var docMode = document.documentMode; | ||
var oldIE = (isExplorer.exec(navigator.userAgent.toLowerCase()) && (!docMode || docMode <= 7)); | ||
|
||
// Add a cross-platform `addEventListener` shim for older browsers. | ||
var addEventListener = window.addEventListener || function (eventName, listener) { | ||
return attachEvent('on' + eventName, listener); | ||
}; | ||
|
||
// Normalize root to always include a leading and trailing slash. | ||
this.root = ('/' + this.root + '/').replace(rootStripper, '/'); | ||
|
||
if (oldIE && this._wantsHashChange) { | ||
var frame = Backbone.$('<iframe src="javascript:0" tabindex="-1">'); | ||
this.iframe = frame.hide().appendTo('body')[0].contentWindow; | ||
// Proxy an iframe to handle location events if the browser doesn't | ||
// support the `hashchange` event, HTML5 history, or the user wants | ||
// `hashChange` but not `pushState`. | ||
if (!this._hasHashChange && this._wantsHashChange && (!this._wantsPushState || !this._hasPushState)) { | ||
var iframe = document.createElement('iframe'); | ||
iframe.src = 'javascript:0'; | ||
iframe.style.display = 'none'; | ||
iframe.tabIndex = -1; | ||
var body = document.body; | ||
// Using `appendChild` will throw on IE < 9 if the document is not ready. | ||
this.iframe = body.insertBefore(iframe, body.firstChild).contentWindow; | ||
this.navigate(fragment); | ||
} | ||
|
||
// Depending on whether we're using pushState or hashes, and whether | ||
// 'onhashchange' is supported, determine how we check the URL state. | ||
if (this._hasPushState) { | ||
Backbone.$(window).on('popstate', this.checkUrl); | ||
} else if (this._wantsHashChange && ('onhashchange' in window) && !oldIE) { | ||
Backbone.$(window).on('hashchange', this.checkUrl); | ||
addEventListener('popstate', this.checkUrl, false); | ||
} else if (this._wantsHashChange && this._hasHashChange && !this.iframe) { | ||
addEventListener('hashchange', this.checkUrl, false); | ||
} else if (this._wantsHashChange) { | ||
this._checkUrlInterval = setInterval(this.checkUrl, this.interval); | ||
} | ||
|
@@ -1448,7 +1457,16 @@ | |
// Disable Backbone.history, perhaps temporarily. Not useful in a real app, | ||
// but possibly useful for unit testing Routers. | ||
stop: function() { | ||
Backbone.$(window).off('popstate', this.checkUrl).off('hashchange', this.checkUrl); | ||
// Add a cross-platform `removeEventListener` shim for older browsers. | ||
var removeEventListener = window.removeEventListener || function (eventName, listener) { | ||
return detachEvent('on' + eventName, listener); | ||
}; | ||
|
||
if (this._hasPushState) { | ||
removeEventListener('popstate', this.checkUrl, false); | ||
} else if (this._wantsHashChange && this._hasHashChange && !this.iframe) { | ||
removeEventListener('hashchange', this.checkUrl, false); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Need to pass |
||
// Some environments will throw when clearing an undefined interval. | ||
if (this._checkUrlInterval) clearInterval(this._checkUrlInterval); | ||
History.started = false; | ||
|
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@braddunbar Just wondering, but do we have to get rid of the
iframe
here? It looks like it just hangs around in the DOM with no listeners afterstop
is called, and creates a new one on eachstart
. Potential memory leak?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep yep, we should remove it. Good catch!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We only store a reference to the iframe's
contentWindow
. Any idea how to turn this into a reference we can remove from the DOM?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm...let's punt on it for this issue and address it separately. It's not a regression in any case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okey #3015