Browser ie11 #2569

Merged
merged 8 commits into from Mar 17, 2014

Projects

None yet

9 participants

@timwienk
MooTools member

Updates for specs and docs, and an optional last commit which is explained below:

Apparently after we decided on just doing the Browser.modernie thing in all builds, some people suddenly started having issues with it anyway. I don't feel like going through all of this again, so the last commit in my branch supports a second option we have.

Summary:

  • Current state (1.4.5):

    • IE11 detection broken
  • Desired state (1.5):

    • Option 1:
      • For all builds: Differentiate between IE>=10 (Browser.ie) and IE<=11 (Browser.modernie). This is to protect people from "just" using Browser.ie and treating them equally bad.
    • Option 2:
      • For no-compat build: Fix IE11 detection (i.e. all IE is once again Browser.ie). This is technically+semantically correct.
      • For compat build: Differentiate between IE>=10 (Browser.ie) and IE<=11 (Browser.modernie). This is to make sure we don't break existing projects that (implicitly or explicitly) rely on Browser.ie being falsy for IE11.
    • Option 3:
      • For all builds: Fix IE11 detection (i.e. all IE is once again Browser.ie). Breaking change, so this is not feasible, the compat build should remain compatible.
    • Option 4:
      • For no-compat build: Browser.ie is true.
      • For compat build: Browser.ie is undefined.
  • State of pull request:

    • Implemented option 4: Least amount of changes since 1.4. Fixes the IE11 issue in the most conservative way, until a decision is reached regarding the other option(s).
    • Ready to be pulled.
@kentaromiura
MooTools member

@timwienk do you mind let me investigate how much work is to remove all the sniffing from core first ?
(we can move it temporally in more if it break too much stuff there)

@SergioCrisostomo
MooTools member

I have no decision here, go for what is more correct/decided.

My thoughts:

1.4 code (and 1.5 compat) should see IE11 as Browser.ie to not break BC
1.5 code (ie 1.5 no compat) would have a "back door" using Browser.modernie

In this case maybe:

if (Browser.name == 'ie'){
    if (Browser.version >= '11') {
        Browser.modernie = true;
    } else {
        Browser.ie = true;
    }
    //<1.4compat>
    Browser.ie = true;
    //</1.4compat>
} else {
    Browser[Browser.name] = true;
}
@timwienk
MooTools member

This is getting completely ridiculous (excuse my annoyance, but we discussed this at length the past days already).

The problem is that Browser.ie is currently NOT true in 1.4, IE11 detection is broken in 1.4. If it were not broken, this whole problem/discussion would not have come up.

What we want to avoid is that we suddenly "fix" it in 1.5, and accidentally activate legacy code for IE11 where people expect it to not be active.

So, @SergioCrisostomo, your suggestion does the exact opposite of what this whole discussion is about.

@timwienk
MooTools member

@kentaromiura I'm very much hoping you/we can get that done. And if you/we do, we can change the docs to immediately say it's moved to the compat-build (rather than saying it'll be moved there in the future).

So yes, please investigate! 👍

@SergioCrisostomo
MooTools member

@timwienk I trust you better than me on this, I was just commenting.
If the decided was that go for it! I have nothing to opose.
Will be nice to see this done.

👍

@timwienk
MooTools member

Let's just focus on the issue at hand, I'll update this in the Pull Request description as well:

  • Current state (1.4.5):

    • IE11 detection broken
  • Desired state (1.5):

    • Option 1:
      • For all builds: Differentiate between IE>=10 (Browser.ie) and IE<=11 (Browser.modernie). This is to protect people from "just" using Browser.ie and treating them equally bad.
    • Option 2:
      • For normal build: Fix IE11 detection (i.e. all IE is once again Browser.ie). This is semantically correct.
      • For compat build: Differentiate between IE>=10 (Browser.ie) and IE<=11 (Browser.modernie). This is to make sure we don't break existing projects that (implicitly or explicitly) rely on Browser.ie being falsy for IE11.
    • Option 3:
      • For all builds: Fix IE11 detection (i.e. all IE is once again Browser.ie). Breaking change, so this is not feasible, the compat build should remain compatible.

So: either Option 1 or Option 2.

@fakedarren
MooTools member
@timwienk timwienk added this to the 1.5 milestone Mar 7, 2014
@timwienk timwienk added the IE label Mar 7, 2014
@ibolmo
MooTools member
  • Option 4 (basically same of Option 2 without the need for modernie):
    • For compat build (default build) Browser.ie is null.
    • For noncompat build (for us ninja folks) Browser.ie is true. (No need for Browser.modernie).
@anutron
MooTools member

++ for Option 1.

@gonchuki
@ibolmo
MooTools member

My reason for Option 4 is simple:

For 99.9% of the folks nothing will have changed with the compat build (the default build). For the 100% of the developers like me that want to know if my user is on an Internet Explorer browser all I want to do is:

 if (Browser.ie) alert("I'm a Google Chrome fanboi, why don't you switch.");

Instead of:

if (Browser.ie || Browser.modernie) alert("... sucks that I have to try other flags :-\");

In other terms: our Browser.<name> API just says what browser we're on. Folks that are using it (even we're at fault at some point) for feature detection are using it for the wrong purposes. Yes let's advocate for not using Browser.<name> for feature detection but let's not cripple our API for the rest of us that know what we're doing.

Again, for folks that are not up to date with the best practices and maintain their code can just use the compat version which Browser.ie returns null. No difference from 1.4.5 and 1.5. Now I'm just arguing that we don't need to add unnecessary flags or logic. Just fix Browser.ie as it should be, and put the responsibility of using the right code/practices to the developer -- as it should be.

@gonchuki
@ibolmo
MooTools member

So the question is: do we have any legitimate reasons for why somebody would want to put IE11 in the same bag as old IE?

To me it's about semantics and consistent API. If we welcome the idea of crippling our APIs we might as well start doing $('#this-kind-of-crap').

If not, then what's wrong about making it more difficult for fanboys and bad coders alike?

Also, I'd hate to think we are designing our code so that we avoid fanboys and bad coders. From day one MooTools has been for the power user. Yes we picked up a few bad coders, but they don't justify to bend to their (bad) will.

@ibolmo
MooTools member

Here's an idea of a legitimate use case. Alex Russell has talked about caching feature detection by using user sniffing (http://infrequently.org/2011/01/cutting-the-interrogation-short/). If we cripple Browser.ie someone (in the future) would have to look for Browser.ie and Browser.modernie and to be future proof will likely check for:

  • Browser.modernchrome
  • Browser.modernfirefox
  • Browser.randomPrefixOpera

Again... consistency folks.

@gonchuki
@marcj

So the question is: do we have any legitimate reasons for why somebody
would want to put IE11 in the same bag as old IE?

That is the wrong question. The question is: why on earth should IE11 get a new option? Only because they changed their user agent? Nah.

IE made always a progress, from IE6 to 7, 7 to 8 etc where it got new features/where it became more and more html standard compatible. Only because they made a bigger step in IE11 shouldn't mean they are not a IE anymore. What is in 6 years when IE12, IE13 is there? Is IE11 then still .modernie? That doesn't make sense.

The purpose of Browser.ie is to detect if the user uses Internet Explorer. Not more. If they use it for feature detection then its their fault. They should knew better, that maybe sometimes a IE is released with more features.

I also don't see why Option 3 is breaking change? From IE6 to IE 7 mootools hasn't introduced a .modernie, same with version 8, although IE got new features. Why should IE11 now get a new option? Makes no sense to me.

I would change absolutely nothing. Just make Browser.ie=true for all IE and all are happy.

@gonchuki

@marcj the thing is that if we "fix" browser detection, then 1.5 will break backwards compatibility because it will behave different than 1.4.5 and earlier versions. Splitting this into compat/no-compat builds multiplies the problem by 2 as plugin creators wouldn't even know if they should use Browser.modernie or not because they don't know against which MooTools build they are working (actaully, let's hope no one uses the new flag)

The fact is nobody should use browser sniffing. There are no legitimate reasons to do that and for the last ~10 releases of jQuery browser sniffing has been gone and I don't see a horde of developers in arms because they lost their precious sniffing abilities. We just need to put up a big sign for whoever that didn't get the memo 2-3 years ago when the collective of developers decided to stop the browser sniffing shenanigans and started using feature detection.

So back to square 1: if we go with the "consistent" API then we break MooTools for every bad plugin that exists even when it currently works with IE11 because it fails to detect it as such. I don't want to release code that will somehow break code that exists in the wild.

Our current compat build is not an option, it does a lot more than just set up browser sniffing.
So for Olmo's option 4 this could be the case:

  • a dev is using 1.4.5 no-compat and a bunch of third party plugins, everything works fine.
  • some of those plugins are doing crappy browser sniffing, but they do work with IE11 because it's not detecting it.
  • the dev wants to switch to 1.5 to take advantage of all the fixes we did in these last 2 years but if he goes with the compat build a lot of stuff might break (because compat does some things differently and even defines a bunch of globals), and if he goes with no-compat his plugins will break because it will begin detecting IE11 as regular IE.

Remember we already surveyed the Forge some time ago and there were a lot of plugins in there that are checking Browser.ie without checking for a version, so even before we are out of the gate we know it's going to cause issues with code that has been in the wild for years.

We need to give a sane upgrade path to 1.5 that won't cause regressions or issues like the one I described above, and as we all agreed before, kill browser sniffing on the next major release so this will become a non-issue (this includes warning users that this is deprecated in the release blog post).
If we fail to give this easy upgrade path then adoption of the new version will be low and we will have failed in our mission.

TL;DR: wall of text, I know. But please let's settle this so we can 🚢

@anutron
MooTools member

Historically I've been Mr. No Breaking Changes and a strong advocate for not changing our api with every release (I hate typeOf instead of $type - that shit's arbitrary breakage). But in this case our main goal should be to avoid breaking bunches of stuff (so people don't upgrade because of a "bad" release) even if it means making an inconsistent API for the short term. We're already agreed on deprecating all the browser stuff so investing in api consistency makes little sense. The only reason that Browser.ie is being worked on is to provide compatibility in 1.5.1 and beyond. We need to fix it, yes, but fixing it here isn't making a stronger API; it's NOT breaking code out there in the wild.

@ibolmo
MooTools member
@kentaromiura
MooTools member

while I'm working on removing all the sniffs, I give my twoppence here
.ie means that the browser is IE, this is the only meaning of that. same as .firefox means that is firefox, is not that Nightly is different from FF.
I'm really contrary to introduce a modernIE that has no meaning other than a practical one to differentiate from oldIE and the current version of IE, so the only options here is solution 2, with or without the compat thing (using sniffing is wrong but if someone use IE to choose between oldIE and current IE is doing that wrong^2, maybe here the correct thing to do is breaking things, since compat is for behavioural things and not for bug, you still have the choice to not upgrade).

@gonchuki
@arian arian commented on an outdated diff Mar 9, 2014
Docs/Browser/Browser.md
@@ -12,9 +12,47 @@ Browser.Features {#Browser:Browser-Features}
* Browser.Features.json - (*boolean*) True if the browser has a native JSON object.
* Browser.Features.xhr - (*boolean*) True if the browser supports native XMLHTTP object.
+Browser.Plugins {#Browser:Browser-Plugins}
+------------------------------------------
+
+* Browser.Plugins.Flash - (*object*) - An object with properties corresponding to the `version` and `build` number of the installed Flash plugin. Note: if flash is not installed, both `Browser.Plugins.Flash.version` and `Browser.Plugins.Flash.build` will return zero.
@arian
arian Mar 9, 2014

This Flash stuff is also deprecated. 9609878 / #2327

@kentaromiura kentaromiura and 1 other commented on an outdated diff Mar 15, 2014
Specs/Browser/Browser.js
@@ -78,7 +78,7 @@ describe('Browser', function(){
it('should think it is executed in a browser', function(){
var isPhantomJS = !!navigator.userAgent.match(/phantomjs/i);
- expect(isPhantomJS || Browser.ie || Browser.safari || Browser.chrome || Browser.firefox || Browser.opera).toEqual(true);
+ expect(isPhantomJS || Browser.ie || Browser.modernie || Browser.safari || Browser.chrome || Browser.firefox || Browser.opera).toEqual(true);
@kentaromiura
kentaromiura Mar 15, 2014

this should not be needed anymore after your latest changes.

@SergioCrisostomo
SergioCrisostomo Mar 15, 2014

Exactly. Just remove my Fix specs so it will look for Browser.modernie commit 541ecb8

@timwienk
MooTools member

Ok. This should now be ready to be pulled, in the most simple way (i.e. Olmo's option 4).

A lot of voices in here are in favour of option 1. The reason I changed the PR to just do this approach is to at least have IE11 consistently fixed with the least required changes (compared to 1.4). I would like to see a decision reached regarding the modernie matter, but I didn't want this PR to wait for the decision since it seems to go back and forth.

@SergioCrisostomo
MooTools member

👍

@ibolmo ibolmo merged commit eb3b93d into mootools:master Mar 17, 2014

1 check passed

Details default The Travis CI build passed
@timwienk timwienk deleted the timwienk:browser-ie11 branch Mar 28, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment