-
Notifications
You must be signed in to change notification settings - Fork 258
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
$.browser.msie -> !$.support.boxModel #30
base: master
Are you sure you want to change the base?
Conversation
$.browser is deprecated in jQ 1.9
Thanks for this. Worked for me. |
Please merge this! |
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? |
@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? |
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. |
@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). |
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 |
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]; |
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. |
@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. |
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: 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. |
@konsumer I'm confused as to what you're suggesting. |
Ah, ok. I guess I misunderstood. Support matrix for those that support hashchange is here: 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:
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. |
I think the main point of feature sniffing is future technology support - Here we're talking about a backwards compatibility shim that will only ever Does it matter how BC code detects legacy shims, just so long as it's
|
@KeithHenry Completely agreed, which is why I was suggesting explicitly checking for those browsers if needed. By using something like a random |
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. |
I'm going to fork and do that, so it's more clear. |
Yeah, that's basically what I did in #34 - just a simple regex check.
|
#36 totes. just slightly less checking in top-level, if browser has hashchange |
$.browser is deprecated in jQ 1.9