Make WordPress Core

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: 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 8 years ago.
33193.patch (2.4 KB) - added by gregrickaby 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.
33193.2.patch (2.4 KB) - added by gregrickaby 8 years ago.
Fix capitol letter
33193.3.patch (8.4 KB) - added by johnbillion 8 years ago.

Download all attachments as: .zip

Change History (19)

@gregrickaby
8 years ago

#1 @dd32
8 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
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 @dd32
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 @iseulde
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.

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

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

@gregrickaby
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.

@gregrickaby
8 years ago

Fix capitol letter

#6 @johnbillion
8 years ago

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

#7 @atomicjack
8 years ago

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

@johnbillion
8 years ago

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

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

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

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


8 years ago

#13 @swissspidy
8 years ago

  • Keywords needs-unit-tests added

#14 @SergeyBiryukov
8 years ago

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

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