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.

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

Support for navigator in Safari on iPad with iOS 13 or later #660

Merged
merged 3 commits into from
Mar 24, 2025

Conversation

ma2yama
Copy link
Contributor

@ma2yama ma2yama commented Mar 18, 2025

⚠️ All PRs have to be done versus 'dev' branch, so be aware of that, or we'll close your issue ⚠️

What kind of change does this PR introduce?

bugfix

Can it be referenced to an Issue? If so what is the issue # ?

No

How can we test it?

Using location-based examples in Safari on iPad with iOS 13 or later.

Summary

The navigator in Safari on iPads with iOS 13 or later has changed.
Fixed to support the new navigator.

Does this PR introduce a breaking change?

No

Please TEST your PR before proposing it. Specify here what device you have used for tests, version of OS and version of Browser

iPad (9th generation), mobile Safari, iOS 18.3.1

Other information

https://stackoverflow.com/questions/60482650/how-to-detect-ipad-useragent-on-safari-browser

@nickw1
Copy link
Collaborator

nickw1 commented Mar 18, 2025

Hi @ma2yama thanks for your latest PR.

I did a bit of research on your fix and it does indeed seem to be a widespread way of detecting iPad with iOS >=13.
For example see https://stackoverflow.com/questions/57765958/how-to-detect-ipad-and-ipad-os-version-in-ios-13-and-up

I will merge this, but - as it affects files outside of the location-based code - @kalwalt do you want to have a quick check too before I do so? It's just adding an iPad check to a few OS/browser detection statements - it all looks good to me but just thought I'd double check with you first. Thanks.

@kalwalt
Copy link
Member

kalwalt commented Mar 20, 2025

Hi @ma2yama thanks for your latest PR.

I did a bit of research on your fix and it does indeed seem to be a widespread way of detecting iPad with iOS >=13. For example see https://stackoverflow.com/questions/57765958/how-to-detect-ipad-and-ipad-os-version-in-ios-13-and-up

I will merge this, but - as it affects files outside of the location-based code - @kalwalt do you want to have a quick check too before I do so? It's just adding an iPad check to a few OS/browser detection statements - it all looks good to me but just thought I'd double check with you first. Thanks.

i have red the article, unfortunately i have any iPad on my hands, so i can not test. Anyway i'm going to take a trip in few days for 3 weeks, at least i can test on my devices.
I see in arjs-profile.js and gps-new-camera.js ad maybe in other parts:

(navigator.userAgent.match(/Macintosh/i) &&
     navigator.maxTouchPoints != null &&
     navigator.maxTouchPoints > 1) || // for iPad Safari 

look different form the test suggested in the article you mentioned, instead in webvr-polyfill.js is the same (as in the article).

@nickw1
Copy link
Collaborator

nickw1 commented Mar 20, 2025

@kalwalt from what I can make out these are two different ways of detecting iPads with newer iOS, but I agree it makes sense to do it the same way throughout the code for consistency.

@ma2yama do you want to modify the PR to always use navigator.platform as suggested in the Stack Overflow article? (Unless there is a reason you are using two different approaches, please let us know if so).

I can test on my own Android device.

@ma2yama
Copy link
Contributor Author

ma2yama commented Mar 21, 2025

@nickw1 @kalwalt thank you for your review.

do you want to modify the PR to always use navigator.platform as suggested in the Stack Overflow article? (Unless there is a reason you are using two different approaches, please let us know if so).

'navigator.platform' is still available, but has been deprecated.
https://developer.mozilla.org/en-US/docs/Web/API/Navigator/platform

So, where I'm using 'navigator.platform' now, I'd like to use 'navigator.userAgent', okay?

@nickw1
Copy link
Collaborator

nickw1 commented Mar 21, 2025

@ma2yama Thanks for your reply. Yes, it would be great if you could update the code to use the non-deprecated version throughout.

As soon as you've done this and tested it I will test on my own device (Chrome, Android).

@kalwalt if it works on my device (I don't have access to an iPad either) after the changes, are you happy if I merge it?

@kalwalt
Copy link
Member

kalwalt commented Mar 21, 2025

@ma2yama Thanks for your reply. Yes, it would be great if you could update the code to use the non-deprecated version throughout.

As soon as you've done this and tested it I will test on my own device (Chrome, Android).

@kalwalt if it works on my device (I don't have access to an iPad either) after the changes, are you happy if I merge it?

I agree with you stick with the non deprecated version, @nickw1 of course you can merge it after testing, i think it will be no issues on other devices.

@ma2yama
Copy link
Contributor Author

ma2yama commented Mar 24, 2025

@nickw1 Thanks for the reply.

The fix to not use navigator.platform has been completed.

@nickw1
Copy link
Collaborator

nickw1 commented Mar 24, 2025

Hi @ma2yama many thanks - have tested location-based examples on my Android device and all working as expected.
It needs a rebuild of the library but I will merge and do that myself.

@nickw1 nickw1 merged commit f48e3af into AR-js-org:dev Mar 24, 2025
1 check passed
nickw1 added a commit that referenced this pull request Mar 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants