In getting IE version, prefer Trident version over MSIE version.

[cfeltner]

Embedded IE browser control includes higher Trident version than MSIE version, so updated getIEVersion to prefer Trident version over MSIE version.

[ghost]

All CLA requirements met.

[MSNev]

This is by design as by default an embedded IE (WebBrowser) actually runs in IE7 mode, so detecting based on the Trident version would result in potentially the wrong values being used.

Returning a X-UA-Compatible met tag in your HTML (should) cause IE to update the "version" of MSIE that it's "running" as.

[cfeltner]

We are sending a X-UA-Compatible meta tag with the following contents:
http-equiv="X-UA-Compatible" content="IE=edge"

But this is the User Agent string that we are getting back:
Mozilla/4.0 (compatible; MSIE 7.0; Windows NT 6.2; Win64; x64; Trident/7.0; .NET4.0C; .NET4.0E)

It is my understanding that this should cause it to use the highest document mode supported which in this case should be IE 11 which matches up with the Trident version (7+4=11) but not the MSIE version. This is why I proposed changing it to prefer the Trident version which takes into account the X-UA-Compatible meta tag.

If the X-UA-Compatible meta tag is not used, then it would run in the default IE7 mode.

[MSNev]

A better approach for this (might be) to add code to review the document.documentMode and document.compatMode, but generally this has not been required and the extra code would be unnecessary for most users.

[cfeltner]

A better approach for this (might be) to add code to review the document.documentMode and document.compatMode, but generally this has not been required and the extra code would be unnecessary for most users.

Are you proposing changing getieversion to return document.documentMode instead of Trident and MSIE versions? Seems like this would be the same as returning the Trident version.

[MSNev]

In your current case because you have the X-UA-Compat meta tag this is probably returning the same values, but for other users that are not including the header this would fall back to the current "mode".

And the returned value of this function is used to switch some features on/off (both in this codebase and another internal variant), so just changing to always use Trident (could) end up causing exceptions because some code would start to assume things and objects that don't exist.

So I guess the real question is why do you need to know this? As most of the code only uses the response as a boolean flag to determine if this is IE or not.

However, I think you have identified what I think is an underlying bug as the Ajax tracking looks like it will always be disabled in this case.

So, to come back to your initial question, yes adding a documentMode check would work around this using something like this so it still falls back to the current behavior. Which I think would then fix the above, handle your case and also ensure that other users without (or a different) meta-tag are also handled correctly.

I'm thinking something like this to minimize the code impact and handle cases where the SDK is not used in a full browser environment where the "document' may not exist or is not available.

if (strContains(ua, strMsie)) {
        let doc = getDocument();
        return Math.Max(parseInt(ua.split(strMsie)[1]), (doc ? (doc.documentMode || 0) : 0));
    } else if (strContains(ua, strTrident)) {

[cfeltner]

I updated the change to use the documentMode as you suggested.

So I guess the real question is why do you need to know this? As most of the code only uses the response as a boolean flag to determine if this is IE or not.

However, I think you have identified what I think is an underlying bug as the Ajax tracking looks like it will always be disabled in this case.

Yes, the reason this was needed was to allow the Ajax monitoring. The following section of code only allows Ajax monitoring for IE >= 9.

./extensions/applicationinsights-dependencies-js/src/ajax.ts

function _supportsAjaxMonitoring(ajaxMonitorInstance:AjaxMonitor): boolean {
:
    let ieVer = getIEVersion();
    if (ieVer && ieVer < 9) {
        result = false;
    }
:

[MSNev]

👍

[MSNev]

Created Bug #1735 for this so that the release notes can include the details on why this change is being added.

[MSNev]

Looks good to me @cfeltner do you have any additional issues you want to address before we commit?

Note: while I've tagged this for the next release which is not scheduled until the end of Jan (early feb availability) once this is committed it will end up in the next nightly build/publish (which we have not documented yet as still working out a few kinks in the pipeline, but it is available via NPM and CDN versions). The nightly build/publish is not meant to be used for production but rather for early testing/validation.

[cfeltner]

@MSNev, great I have no additional changes to this PR. Thanks for the info.