Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix: Add detection to for iPadOS in IS_IPAD const #6319

Merged
merged 2 commits into from
Nov 19, 2019
Merged

Fix: Add detection to for iPadOS in IS_IPAD const #6319

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4ac3dfe
Add detection to for iPadOS in IS_IPAD const
marcodeltorob Nov 15, 2019
42adf24
IS_IPAD in single line
marcodeltorob Nov 15, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 39 additions & 39 deletions src/js/utils/browser.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,27 +9,6 @@ const USER_AGENT = window.navigator && window.navigator.userAgent || '';
const webkitVersionMap = (/AppleWebKit\/([\d.]+)/i).exec(USER_AGENT);
const appleWebkitVersion = webkitVersionMap ? parseFloat(webkitVersionMap.pop()) : null;

/**
* Whether or not this device is an iPad.
*
* @static
* @const
* @type {Boolean}
*/
export const IS_IPAD = (/iPad/i).test(USER_AGENT);

/**
* Whether or not this device is an iPhone.
*
* @static
* @const
* @type {Boolean}
*/
// The Facebook app's UIWebView identifies as both an iPhone and iPad, so
// to identify iPhones, we need to exclude iPads.
// http://artsy.github.io/blog/2012/10/18/the-perils-of-ios-user-agent-sniffing/
export const IS_IPHONE = (/iPhone/i).test(USER_AGENT) && !IS_IPAD;

/**
* Whether or not this device is an iPod.
*
Expand All @@ -39,15 +18,6 @@ export const IS_IPHONE = (/iPhone/i).test(USER_AGENT) && !IS_IPAD;
*/
export const IS_IPOD = (/iPod/i).test(USER_AGENT);

/**
* Whether or not this is an iOS device.
*
* @static
* @const
* @type {Boolean}
*/
export const IS_IOS = IS_IPHONE || IS_IPAD || IS_IPOD;

/**
* The detected iOS version - or `null`.
*
Expand Down Expand Up @@ -183,15 +153,6 @@ export const IE_VERSION = (function() {
*/
export const IS_SAFARI = (/Safari/i).test(USER_AGENT) && !IS_CHROME && !IS_ANDROID && !IS_EDGE;

/**
* Whether or not this is any flavor of Safari - including iOS.
*
* @static
* @const
* @type {Boolean}
*/
export const IS_ANY_SAFARI = (IS_SAFARI || IS_IOS) && !IS_CHROME;

/**
* Whether or not this is a Windows machine.
*
Expand All @@ -212,3 +173,42 @@ export const TOUCH_ENABLED = Dom.isReal() && (
'ontouchstart' in window ||
window.navigator.maxTouchPoints ||
window.DocumentTouch && window.document instanceof window.DocumentTouch);

/**
* Whether or not this device is an iPad.
*
* @static
* @const
* @type {Boolean}
*/
export const IS_IPAD = (/iPad/i).test(USER_AGENT) || (IS_SAFARI && TOUCH_ENABLED);
Copy link

@Viktor286hearts Viktor286hearts Dec 23, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could it be that IS_SAFARI && TOUCH_ENABLED also will include iPhone cases as well?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is a good question.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, looks like we should add a !! IS_IPHONE or something.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could be out of scope, sorry for mixing two things. But, is it necessary to use regexp here to detect substring? Could it better for performance to use includes or indexOf? If you agree, i could make a PR with refactor of this utlis browser.js module.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR here #6371

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, includes isn't available on all platforms that we support and I'm not sure that the extra code that would arise from using indexOf is worth it, especially since it may end up being slower. Is there a specific benefit to using include or indexOf?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea was to avoid regex because it much more performance demanding while indexOf or include could be more optimized (now or in later js engines). Although, it's not a huge optimization anyway.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I don't think benchmarks are that useful, this one does show that the behavior is different depending on the browser https://jsperf.com/startswith-vs-indexof-vs-regex/4. For example in Chrome indexOf is much faster while on Firefox startsWith is faster with the Regexp#test being on par. Also, given that this runs once, I'm not sure it really matters what is being done and shouldn't ever be in the critical path, so I'd be inclined to just leave it as is.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds reasonable! Thanks for the shared info. For me it also performed differently for mobile and for desktop with that benchmark http://jsben.ch/d4o38


/**
* Whether or not this device is an iPhone.
*
* @static
* @const
* @type {Boolean}
*/
// The Facebook app's UIWebView identifies as both an iPhone and iPad, so
// to identify iPhones, we need to exclude iPads.
// http://artsy.github.io/blog/2012/10/18/the-perils-of-ios-user-agent-sniffing/
export const IS_IPHONE = (/iPhone/i).test(USER_AGENT) && !IS_IPAD;

/**
* Whether or not this is an iOS device.
*
* @static
* @const
* @type {Boolean}
*/
export const IS_IOS = IS_IPHONE || IS_IPAD || IS_IPOD;

/**
* Whether or not this is any flavor of Safari - including iOS.
*
* @static
* @const
* @type {Boolean}
*/
export const IS_ANY_SAFARI = (IS_SAFARI || IS_IOS) && !IS_CHROME;