Make WordPress Core

Opened 10 years ago

Closed 10 years ago

#33193 closed defect (bug) (fixed)

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

Reported by: gregrickaby's profile gregrickaby Owned by: johnbillion's profile johnbillion
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)

patch.diff (1.8 KB) - added by gregrickaby 10 years ago.
33193.patch (2.4 KB) - added by gregrickaby 10 years ago.
You are correct, as of now, Edge = Chrome (according to the current script). Ok. Updated this patch to add $is_edge global.
33193.2.patch (2.4 KB) - added by gregrickaby 10 years ago.
Fix capitol letter
33193.3.patch (8.4 KB) - added by johnbillion 10 years ago.

Download all attachments as: .zip

Change History (19)

@gregrickaby
10 years ago

#1 @dd32
10 years ago

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.

#2 @gregrickaby
10 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 @dd32
10 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 @iseulde
10 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.

Last edited 10 years ago by iseulde (previous) (diff)

#5 @atomicjack
10 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.

@gregrickaby
10 years ago

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

@gregrickaby
10 years ago

Fix capitol letter

#6 @johnbillion
10 years ago

  • Keywords has-patch needs-testing added
  • Milestone changed from Awaiting Review to 4.4

#7 @atomicjack
10 years ago

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

#8 @johnbillion
10 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 @gregrickaby
10 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.

Last edited 10 years ago by gregrickaby (previous) (diff)

#10 @johnbillion
10 years ago

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

#11 @johnbillion
10 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

This ticket was mentioned in Slack in #core by sergey. View the logs.


10 years ago

#13 @swissspidy
10 years ago

  • Keywords needs-unit-tests added

#14 @SergeyBiryukov
10 years ago

  • Owner set to johnbillion
  • Status changed from new to assigned

#15 @johnbillion
10 years ago

  • Keywords dev-feedback needs-unit-tests removed
  • Resolution set to fixed
  • Status changed from assigned to closed

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

Note: See TracTickets for help on using tickets.