-
-
Notifications
You must be signed in to change notification settings - Fork 987
Fix iPad detection in wasm-gc #5695
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
Conversation
|
Could you check if this patch works? I deployed it in the GH pages. https://ryanking13.github.io/temp-pyodide-nightly-runtime/console.html |
| + // AND if the device has multi-touch capabilities (navigator.maxTouchPoints > 1) | ||
| + let isIOS = globalThis.navigator && ( | ||
| + /iPad|iPhone|iPod/.test(navigator.userAgent) || | ||
| + (navigator.platform === 'MacIntel' && typeof navigator.maxTouchPoints !== 'undefined' && navigator.maxTouchPoints > 1) |
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.
FYI undefined > 1 is always false so that a check for a type as undefined is bloat but, most importantly, a check that access the property might have side-effects ... it's a long story but if you really want to check the existence, you are better off with 'maxTouchPoints' in navigator instead of typeof navigator.maxTouchPoints as in operator signals the presence of either accessors or values but it doesn't reach these (lazy accessors can backfire, I have learned this long time ago)
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.
Also, I think comparing typeof something to "undefined" should only be done for a top level name because x === undefined raises an error in strict mode. But a.b === undefined does not. They are distinct checks since if you set a.b = undefined then "b" in a is true.
But this check only happens once on startup so it's not all that important exactly how it happens as long as it filters out all the iDevices.
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 fact in produces true but the value is undefined won't invalidate what I've said because undefined > 1 is always false but, logically speaking, the check should be typeof navigator.maxTouchPoints === 'number' && navigator.maxTouchPoints > 1 if we want to be sure that typeof has a reason to exist at all in there ... anyway, it's a nit but the current check is both redundant and not clean in its intent ... glad it shipped though 🥳
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.
Yes, thanks for the heads up @WebReflection! Since upstream PR is merged as-is (that was fast 🙄), I would like to merge this as-is to prevent future conflict. This patch is a hack anyways, so let's drop it when Apple delivers a fix.
|
@ryanking13 nit a part, it works like a charm: |
|
Thanks @ryanking13 ! |

Description
Hopefully resolve #5670
Checklist