The global, $is_IE, doesn't properly detect Microsoft Edge

[gregrickaby]

Because Microsoft Edge has incorporated a few different browser engines, the user agent reports itself as "Mozilla", "Chrome", "Safari", "Gecko", and finally "Edge".

User agent:

Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/42.0.2311.135 Safari/537.36 Edge/12.<OS build number>

Source: ​https://msdn.microsoft.com/en-us/library/hh869301(v=vs.85).aspx

Attached is a fix so that Microsoft Edge will only trip the $is_IE conditional. Tested on CrossBrowserTesting.com.

[dd32]

I'm not completely caught up with Microsofts new browser, but it could quite possibly be expected for the $is_IE global to not be true for it's new rendering engines.

The reasoning being, is that the globals are mostly used for CSS detection, serving CSS designed for older IE versions to the latest IE isn't going to be in the best interest of the user - the browser is specifically sending it's new UA so that it does avoid that old css and/or gets the css designed for webkit/others.

This could be a case where we should instead introduce a global of $is_IE_ancient or something, or just deprecate it entirely.

[gregrickaby]

With all due respect, the very reason for this patch was to help target Microsoft Edge on a current project of mine.

I needed to fix an overflow bug: -ms-overflow-style: none;

New rendering engine(s) or not, people (e.g., front-end devs like me) will still need to target M$FT browsers. I'm for anything that makes this process easiest; just know, the current global returns true for $is_chrome (when it's clearly not). That's not in the best interest of anyone!

As an aside: this entire file needs to be overhauled. No yoda conditionals, endless if/else/else ifs, etc. Maybe a PHP switch is in order? This one does it pretty well: ​https://github.com/cbschuld/Browser.php/blob/master/lib/Browser.php

Thanks for hearing me out @dd32 and I'm happy to help...

[dd32]

That specific rule could probably be applied regardless of if the browser is IE Edge, but I do understand where you're coming from. Like it or not, browser targeting is still required sometimes. Perhaps a $is_IE_Edge def is better suited, but at the end of the day, perhaps just sticking with $is_IE and not caring about which rendering engine it is is the best way to go.

[iseulde]

If the example UA string on this ticket is right, we may still need to adjust our browser detection. With this UA $is_chrome would be true, which may or may not be okay.

[atomicjack]

Microsoft Edge is a completely different browser, and we should treat it as such - otherwise we may as well treat Chrome as Firefox, Safari as Opera, etc.

IE and Edge have different support for CSS and JavaScript; combining them together is just not going to be suitable.

[gregrickaby]

You are correct, as of now, Edge = Chrome (according to the current script). Ok. Updated this patch to add $is_edge global.

[gregrickaby]

Fix capitol letter

[atomicjack]

I will reiterate what I said previously, $is_IE should *not* detect Edge. Edge is an entirely different browser.

[johnbillion]

33193.3.patch​ simplifies the patch by moving the check for Edge to the beginning of our nice big block of if conditions. It also moves wp_is_mobile() into wp-includes/functions.php so we can implement unit tests.

I added a bunch of user agent strings for testing. User agent strings are so messed up it's not even funny any more.

[gregrickaby]

@johnbillion - this is awesome.

Would any one be opposed to turning that nice big block of if conditions into a switch? Not only would it be easier to read/follow, but it's a possible performance boost.

[johnbillion]

In 33927:

Introduce a new $is_edge global for the Microsoft Edge browser.

The $is_IE and $is_chrome globals no longer return true for Edge, which is expected as Edge is its own browser and should not be treated like IE. While it might be beneficial for Edge to be treated like Chrome on the client side, it benefits nobody for the $is_chrome global to return true.

See #33193
Props gregrickaby, johnbillion

[johnbillion]

I've held off on committing the unit tests for now as it involves moving the wp_is_mobile() function, which relies on the globals in wp-includes/vars.php, and therefore shouldn't be defined any earlier than it is.

These client globals are all pretty nasty and if it was up to me I'd remove the lot of them, but we're stuck with them now.

Related: #33704

[johnbillion]

Closing as fixed for now. If #33704 ends up going in, we can add in these tests.