-
Notifications
You must be signed in to change notification settings - Fork 7.5k
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.
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
Fix: Add detection to for iPadOS in IS_IPAD const #6319
Conversation
src/js/utils/browser.js
Outdated
export const IS_IPAD = (/iPad/i).test(USER_AGENT) || | ||
(IS_SAFARI && TOUCH_ENABLED); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can just be a single line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about Chrome on iPadOS?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
chrome is unaffected.
* @const | ||
* @type {Boolean} | ||
*/ | ||
export const IS_IPAD = (/iPad/i).test(USER_AGENT) || (IS_SAFARI && TOUCH_ENABLED); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR here #6371
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Description
New iPadOS User-agent does not contain "iPad" string in order to detect the device
Specific Changes proposed
Detect IS_SAFARI && TOUCH_ENABLED we can conclude is and iPad Device.