cowboy / jquery-hashchange

This jQuery plugin enables very basic bookmarkable #hash history via a cross-browser HTML5 window.onhashchange event.
http://benalman.com/projects/jquery-hashchange-plugin/
GNU General Public License v2.0
1.22k stars 258 forks source link

$.browser.msie -> !$.support.boxModel #30

Open nporteschaikin opened 11 years ago

nporteschaikin commented 11 years ago

$.browser is deprecated in jQ 1.9

JangoSteve commented 11 years ago

Thanks for this. Worked for me.

crowdmatt commented 11 years ago

Please merge this!

KeithHenry commented 10 years ago

I don't get it - when people say browser detection is bad they don't mean: just pick another feature supported as random that happens to fail in the same browsers.

If this is to change to not be a browser check it needs to detect support for the iframe history hack working, not that box model is not supported, surely?

JangoSteve commented 10 years ago

@KeithHenry I didn't even notice that this wasn't actually detecting a needed feature, I had just skimmed right over it. But I agree. Since this line is already checking for if onhashchange is supported, what else is it in IE6/7 that doesn't work?

KeithHenry commented 10 years ago

The check on IE6&7 is not because they don't support hash change events, but that they can support this workaround. It's actually a hack - navigating a hidden IFRAME in a way specifically weird to IE.

Really the check should be something like: !supports_onhashchange && supports_iframehack, but there's no easy way to check that a browser supports the frame hackiness.

JangoSteve commented 10 years ago

@KeithHenry Yeah, I should have glanced at the source. Not sure I see the difficulty in checking support of the hack though. Looking through it, it's not incredibly complicated. I'm guessing there's something specific in there that old IE just can't do meaning it either throws an error, or it simply doesn't work (returns no data where there should be some).

If the former, then check for whether that function or property that throws the error is defined (or at worse, try it inside a try/catch block which it seems to already do for one thing in there). If the latter, then what's the harm in just running the code? Either it'll work, or it won't which would be the same as not running the code.

The only things I've seen in the past that was difficult to check for was when a browser needlessly disabled or sandboxed something that was otherwise defined in the name of security. If that's the case, then yeah, that could be hard to detect, and I'd probably just go back to detecting explicitly for IE6/7, not some other random property (at least then, it'd be clear what we're checking for).

konsumer commented 10 years ago

Yeh, I don't think we should check for a random property, but if there is a simple way to detect what is we need, I like that better than adding iframes. It was a quick way to target the browser, using existing codebase. I add it to the top of things that use hashchange, and they work. Not ideal, but still better than adding iframes to my source, in my opinion. I think supports_onhashchange is the the right route, checking for the actual feature. Do we need the other IE check? if so, why not use document.documentMode?

JangoSteve commented 10 years ago

For what it's worth, here's how jQuery 1.8 detected msie. We could just re-implement this:

var ieMatch = /(msie) ([\w.]+)/.exec(navigator.userAgent),
     ieVersion = ieMatch && ieMatch[2];
konsumer commented 10 years ago

Yeh, that still doesn't seem like the right direction. The whole idea of checking for features, rather than browser-sniffing is this:

http://ejohn.org/blog/future-proofing-javascript-libraries/

You can just use jquery-migration, if you want to support deprecated methods. It seems simple to me: Either supports_onhashchange fully supports checking if browser can do it, or it doesn't. If it doesn't then a check for what it's not supporting should be included. Since document.documentMode does that, seems like the problem is solved, if that is needed, too. Am I missing something?

I think I might just start using modernizr, and forget this. It's other methodologies follow my philosophy better, I will use it's other checks, and it adds support for HTML5 History & onhashchange.

JangoSteve commented 10 years ago

@konsumer Agreed, but it from what others are saying here, this is a separate check from the hashchange support check. They're saying that the issue is with older IE browsers that have a given property defined but which doesn't work when you try to use it.

Honestly, if that's the case, I would go with my original suggestion and just execute the iframe hack and let it do nothing for those browsers (which is no different than what happens right now where it simply doesn't execute the iframe hack for those browsers).

My most recent suggestion though, was more along the lines of if doing so would cause errors in such browsers. If that's the case, then if what we're trying to do is check for old IE and there's no way around that, then we should at least explicitly check for it, and not check for some random feature that just happens to be unsupported in the browser versions we're trying to check for.

konsumer commented 10 years ago

Agreed. That is the point of document.documentMode and It returns correct info for browsers that support it, so you can check that param, rather than a fragile regex or inserting iframes.

In modernizr: https://github.com/Modernizr/Modernizr/blob/master/feature-detects/hashchange.js

I think you are clinging to the strawman of "it's bad to just detect boxmodel, as it's unrelated" I totally agree, but it's not more bad than regex, or the old $.browser.msie.

JangoSteve commented 10 years ago

@konsumer I'm confused as to what you're suggesting. document.documentMode is already being used in the check for supporting hashchange. We're not talking about that. We're talking about checking the browser to see if it supports the iframe hackiness, i.e. the hashchange fallback for when the browser doesn't support hashchange. No one is arguing that document.documentMode can't be used to check for hashchange support.

konsumer commented 10 years ago

Ah, ok. I guess I misunderstood. Support matrix for those that support hashchange is here: http://caniuse.com/#search=hashchange

Of those, how many would not support the hack? If the answer is "none" then it seems to me we still don't need any other checks. If the answer is "only IE supports the hack" (which it is, right?) then the answer is "use document.documentMode".

basically pseudocode is this:

if (!window.onhashchange && document.documentMode){
  // IE specific hash-hack
}else{
  // use built-in hashchange stuff
}

right?

Also, since modernizr lists this project as the best pollyfill for supporting this, at least in most of my jquery-based stuff, I am more motivated to make it work better without having to include jquery-migration.

KeithHenry commented 10 years ago

I think the main point of feature sniffing is future technology support - save you having to rewrite stuff for a new user agent string or because IE10 finally supports something.

Here we're talking about a backwards compatibility shim that will only ever be needed, or even work, for IE6-7.

Does it matter how BC code detects legacy shims, just so long as it's pretty terse and doesn't use $.browser? On 3 Sep 2013 20:29, "David Konsumer" notifications@github.com wrote:

Ah, ok. I guess I misunderstood. Support matrix for those that support hashchange is here: http://caniuse.com/#search=hashchange

Of those, how many would not support the hack? If the answer is "none" then it seems to me we still don't need any other checks. If the answer is "only IE supports the hack" (which it is, right?) then the answer is "use document.documentMode".

basically pseudocode is this:

if (!window.onhashchange && document.documentMode){ // IE specific hash-hack }else{ // use built-in hashchange stuff }

right?

Also, since modernizr lists this project as the best pollyfill for supporting this, at least in most of my jquery-based stuff, I am more motivated to make it work better without having to include jquery-migration.

— Reply to this email directly or view it on GitHubhttps://github.com/cowboy/jquery-hashchange/pull/30#issuecomment-23740036 .

JangoSteve commented 10 years ago

@KeithHenry Completely agreed, which is why I was suggesting explicitly checking for those browsers if needed. By using something like a random boxModel check, it's not clear what the intention of the code is, and you run the risk of some other browser not supporting it and erroneously being targeted.

konsumer commented 10 years ago

I am not proposing a random boxModel check, if this is to be properly implemented, I am just saying it's better than adding an iframe, and in my usage better than loading jquery-migrate. I also find it a bit frustrating that I have to make other browsers search their use-agent in the top-level check for hashchange support, when they have it.

If the only browsers this hack can help are identified by document.documentMode, then I don't understand why not to use it, as that is what it's for. If the hack works for browsers that don't set document.documentMode, then I am totally fine with a regex, as that seems to really be the only way to do it.

In IE7, I just went to URL: javascript:alert(document.documentMode) and it alerted "undefined", so yeah, let's do that IE regex as a secondary check. sounds good.

konsumer commented 10 years ago

I'm going to fork and do that, so it's more clear.

KeithHenry commented 10 years ago

Yeah, that's basically what I did in #34 - just a simple regex check. On 3 Sep 2013 22:12, "David Konsumer" notifications@github.com wrote:

I'm going to fork and do that, so it's more clear.

— Reply to this email directly or view it on GitHubhttps://github.com/cowboy/jquery-hashchange/pull/30#issuecomment-23747313 .

konsumer commented 10 years ago

36 totes. just slightly less checking in top-level, if browser has hashchange