Opened 8 years ago
Closed 8 years ago
#33193 closed defect (bug) (fixed)
The global, $is_IE, doesn't properly detect Microsoft Edge
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 4.4 | Priority: | normal |
Severity: | normal | Version: | |
Component: | General | Keywords: | has-patch |
Focuses: | Cc: |
Description
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.
Attachments (4)
Change History (19)
#2
@
8 years ago
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...
#3
@
8 years ago
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.
#4
@
8 years ago
- Version trunk deleted
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.
#5
@
8 years ago
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.
@
8 years ago
You are correct, as of now, Edge = Chrome (according to the current script). Ok. Updated this patch to add $is_edge
global.
#6
@
8 years ago
- Keywords has-patch needs-testing added
- Milestone changed from Awaiting Review to 4.4
#7
@
8 years ago
I will reiterate what I said previously, $is_IE should *not* detect Edge. Edge is an entirely different browser.
#8
@
8 years ago
- Keywords dev-feedback added; needs-testing removed
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.
#9
@
8 years ago
@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.
#11
@
8 years ago
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
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.