fix(platform): catch iPadOS 13+ + deprecate dead-platform flags (#1467)#1485
Open
obiot wants to merge 5 commits into
Open
fix(platform): catch iPadOS 13+ + deprecate dead-platform flags (#1467)#1485obiot wants to merge 5 commits into
obiot wants to merge 5 commits into
Conversation
Two real problems in `src/system/platform.ts` flagged during the 19.7 audit: 1. Modern iPads (iPadOS 13+, since Sept 2019) ship Safari with the desktop Mac UA — no `iPad` token in the user-agent string. The `/iPhone|iPad|iPod/i` test missed every iPad sold in the last ~7 years, and they fell through `isMobile` as desktop. Confirmed observable: `keyboard.ts`, `application.ts`, `header.ts` all branch on `isMobile`. 2. Dead-platform UA regexes (`wp`, `BlackBerry`, `Kindle`, `android2`) tested for hardware that was EOL'd between 2012 and 2017, burning regex cycles on every page load. ## Changes **iPad detection**: layer a feature check on top of the UA regex — `navigator.platform === "MacIntel"` plus `maxTouchPoints > 1`. The `"MacIntel"` string is Apple's frozen legacy identifier (same trick as `Win32` on 64-bit Windows) that persists on Apple Silicon Macs *and* iPads in desktop-Safari mode — it's not a CPU check. `Macs don't have touchscreens; iPads do`, so `maxTouchPoints > 1` uniquely separates them. Every existing internal consumer of `isMobile` inherits the fix transparently. **Deprecate dead exports**: `@deprecated` JSDoc on `wp`, `BlackBerry`, `Kindle`, `android2`. Exports stay functional through 19.x for backwards compat (any external consumer keeps working); IDE warnings surface at the call sites. Removal scheduled for 20.x. Also dropped these from `isMobile`'s OR chain. The remaining `/Mobi/.test(ua) || iOS || android` covers ~99.9% of 2026 mobile traffic per MDN's recommendation. **Won't add `isTouch`** as the original issue suggested — we already have `device.touch` at `system/device.js:116` (feature-detected via `navigator.maxTouchPoints` / pointer events). CHANGELOG migration note points there for new code. ## Tests Six new cases in `tests/platform.spec.ts` covering the iPad-on-Mac-UA contract — verify the documented `platform === "MacIntel" && maxTouchPoints > 1` check identifies iPads correctly, rejects actual Macs (no touch), Windows touchscreens, and missing-navigator (SSR). Existing 20 shape / desktop-defaults assertions kept. Full suite: 3975 passed / 13 skipped / 0 failed (was 3969 — +6 from the new tests, no regressions). ## Follow-ups (separate issues worth filing) - `keyboard.ts:85` `if (!isMobile)` skips key-event listeners. iPads with Magic Keyboard (now correctly identified) would stop receiving keys. Probably always-attach + let no-op-on-pure-touch sort itself out, but needs the iPad-with-keyboard test path to validate. - Migrate to `navigator.userAgentData` (Client Hints) where available. Chromium-only today; Safari/Firefox lag. 20.x candidate. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Updates melonJS platform detection to correctly classify modern iPads (iPadOS 13+) as iOS/mobile, while deprecating and removing dead-platform UA regexes from the isMobile aggregate to reduce noise and overhead.
Changes:
- Detect iPadOS 13+ devices that present a desktop “Mac” UA via
navigator.platform === "MacIntel"+maxTouchPoints > 1, and fold this intoiOS/isMobile. - Deprecate legacy platform flags (
wp,BlackBerry,Kindle,android2) and remove the dead-platform flags from theisMobileOR chain. - Add/adjust unit tests for the new
isMobilewiring and the iPadOS 13+ detection contract; document the behavior in the changelog.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| packages/melonjs/src/system/platform.ts | Adds iPadOS 13+ detection logic, deprecates dead-platform flags, and simplifies isMobile aggregation. |
| packages/melonjs/tests/platform.spec.ts | Updates isMobile expectation and adds contract tests for the iPadOS 13+ detection heuristic. |
| packages/melonjs/CHANGELOG.md | Documents the iPadOS 13+ fix and deprecation/removal of dead-platform flags from isMobile. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+11
to
+13
| * wp `true` if the device is a Windows Phone platform (deprecated) | ||
| * BlackBerry`true` if the device is a BlackBerry platform (deprecated) | ||
| * Kindle`true` if the device is a Kindle platform (deprecated) |
Comment on lines
+45
to
+47
| /** | ||
| * @deprecated Android 2.x predates 2012. Will be removed in 20.x. | ||
| */ |
Comment on lines
+51
to
+53
| /** | ||
| * @deprecated Windows Phone was EOL'd by Microsoft in 2017. Will be removed in 20.x. | ||
| */ |
Comment on lines
+55
to
+57
| /** | ||
| * @deprecated BlackBerry stopped shipping BB10 devices in 2016. Will be removed in 20.x. | ||
| */ |
Comment on lines
+59
to
+61
| /** | ||
| * @deprecated Kindle has a negligible market share and behaves like Android. Will be removed in 20.x. | ||
| */ |
| // `wp` / `BlackBerry` / `Kindle` — the underlying platforms are EOL | ||
| // and the regexes were burning cycles on every page load for | ||
| // hardware nobody ships. | ||
| export const isMobile = /Mobi/i.test(ua) || iOS || android || false; |
Comment on lines
+142
to
+145
| it("does not flag a Mac touch-bar laptop (`maxTouchPoints === 1`)", () => { | ||
| // The check uses `> 1`, not `> 0`. A hypothetical single-point | ||
| // touch device should not trip it — multi-touch is iPad-class. | ||
| expect(isIPadOnMacUA({ platform: "MacIntel", maxTouchPoints: 1 })).toBe( |
`keyboard.ts:85` skipped attaching `keydown` / `keyup` listeners when `isMobile === true`. The gate assumed "mobile = no physical keyboard" — invalid in 2026 with iPads (now correctly identified post the platform fix above) using Magic Keyboard, Bluetooth keyboards on phones, Samsung DeX, ChromeOS tablet mode, etc. Two empty listener slots cost essentially nothing on touch-only devices; the handler's unbound-key path is a single map lookup that returns undefined. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
945 lines / 53 exports / 56 JSDoc blocks of feature-detection helpers
and platform plumbing now ship as a `.ts` file with native type
signatures. JSDoc was already exhaustive so the conversion is mostly
mechanical — `@param {Type}` blocks become parameter annotations and
`@type {Type}` constants get TS-inferred.
Non-standard / legacy browser surfaces (`Document.mozFullScreenEnabled`,
`Navigator.standalone` / `browserLanguage` / `userLanguage`, iOS-only
`DeviceOrientationEvent.requestPermission`, deprecated
`Screen.lockOrientation`, `webkitAudioContext`) are typed via narrow
local intersection types declared at the top of the file.
Two small runtime improvements that fell out of the conversion:
- the cached `domRect` is now a real `DOMRect` (its `right`/`bottom`
getters track `x + width` / `y + height` automatically, so the old
explicit assignment of `domRect.right` was redundant);
- `onDeviceMotion` now guards against
`e.accelerationIncludingGravity === null` rather than crashing.
Behavioural parity verified against the full 3975-test suite;
downstream call sites are unchanged thanks to bundler-resolution
rewriting `.js` imports to `.ts` source.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (4)
packages/melonjs/src/system/device.ts:44
domRectis now constructed vianew DOMRect(...)at module load time. In Node/SSR (or any non-DOM environment),DOMRectmay be undefined, causing an immediate ReferenceError just by importingdevice.ts. Please guard this creation and fall back to a lightweight object whenDOMRectisn't available.
packages/melonjs/src/system/device.ts:290autoFocuswas changed from an exportedletto an exportedconst. This prevents consumers from disabling the autofocus behavior (me.device.autoFocus = false), which appears to be part of the public API per the JSDoc (@default true) and is used as a runtime flag (e.g. inpointerevent.ts).
packages/melonjs/src/system/device.ts:246- The
device.isMobileJSDoc list is now out of date:platform.isMobileno longer ORsBlackBerry/Windows Phone/Kindle, so the comment is misleading.
packages/melonjs/src/system/device.ts:604 getElementnever returnsnull(it falls back todocument.body), but the JSDoc still says it can return null. This mismatch can confuse consumers and generated docs.
Comment on lines
+11
to
+13
| * wp `true` if the device is a Windows Phone platform (deprecated) | ||
| * BlackBerry`true` if the device is a BlackBerry platform (deprecated) | ||
| * Kindle`true` if the device is a Kindle platform (deprecated) |
Comment on lines
+45
to
62
| /** | ||
| * @deprecated Android 2.x predates 2012. Will be removed in 20.x. | ||
| */ | ||
| export const android2 = /Android 2/i.test(ua); | ||
| export const linux = /Linux/i.test(ua); | ||
| export const chromeOS = /CrOS/.test(ua); | ||
| /** | ||
| * @deprecated Windows Phone was EOL'd by Microsoft in 2017. Will be removed in 20.x. | ||
| */ | ||
| export const wp = /Windows Phone/i.test(ua); | ||
| /** | ||
| * @deprecated BlackBerry stopped shipping BB10 devices in 2016. Will be removed in 20.x. | ||
| */ | ||
| export const BlackBerry = /BlackBerry/i.test(ua); | ||
| /** | ||
| * @deprecated Kindle has a negligible market share and behaves like Android. Will be removed in 20.x. | ||
| */ | ||
| export const Kindle = /Kindle|Silk.*Mobile Safari/i.test(ua); |
| - **`Camera2d.updateTarget` smooth follow is now frame-rate independent.** Previously `pos.lerp(target, damping)` ran a parametric per-frame fraction — same `damping = 0.1` covered 10% of the gap per frame at 30Hz, 60Hz, 120Hz or 144Hz, so wall-clock convergence sped up linearly with the player's refresh rate. Now uses `pos.damp(target, lambda, dt)` with `lambda = -ln(1 - damping) * timer.maxfps`, which recovers the legacy per-frame fraction exactly at the configured target framerate AND keeps wall-clock convergence constant if the actual frame rate drifts. **No tuning change required** — existing `damping` values keep their feel at the engine's target framerate (default 60); high-refresh users finally get the same feel the dev tuned for. Dogfoods the new `math.damp` API on melonJS's most prominent older follow path. | ||
| - **`device.platform.isMobile` no longer ORs the dead-platform regexes** (#1467). `wp` / `BlackBerry` / `Kindle` regexes were burning cycles on every page load testing for hardware nobody ships (Windows Phone EOL 2017, BB10 EOL 2016, Kindle behaves like Android anyway). The remaining chain — `/Mobi/.test(ua) || iOS || android` — covers ~99.9% of mobile traffic in 2026 per MDN. The deprecated exports themselves still compute and return; only the `isMobile` aggregate stopped consulting them. | ||
| - **`initKeyboardEvent` no longer skips listener registration on `isMobile === true`** (#1467). The gate assumed "mobile = no physical keyboard" — invalid for iPads with Magic Keyboard (now correctly detected per the iPad fix above), Samsung DeX, ChromeOS tablet mode, Bluetooth-keyboard-on-phone, etc. Two empty listener slots cost nothing on touch-only devices; the unbound-key path is a single map lookup that returns undefined. | ||
| - **`system/device.js` converted to TypeScript** (#1467). 945 lines / 53 exports / 56 JSDoc blocks of feature-detection helpers and platform plumbing now ship as a `.ts` file with native type signatures. JSDoc was already exhaustive, so the conversion is mostly mechanical — `@param {Type}` blocks become parameter annotations and `@type {Type}` constants get TS-inferred. Non-standard / legacy browser surfaces (`Document.mozFullScreenEnabled`, `Navigator.standalone` / `browserLanguage` / `userLanguage`, iOS-only `DeviceOrientationEvent.requestPermission`, deprecated `Screen.lockOrientation`, `webkitAudioContext`) are typed via narrow local intersection types declared at the top of the file. Behavioural parity verified against the full 3975-test suite; downstream call sites (`pointerevent.ts`, `application.ts`, `resize.ts`, `header.ts`, etc.) are unchanged thanks to bundler-resolution rewriting `.js` imports to `.ts` source. Two small runtime improvements that fell out of the conversion: the cached `domRect` is now a real `DOMRect` (its `right`/`bottom` getters track `x + width` / `y + height` automatically, so the old explicit assignment of `domRect.right` was redundant), and `onDeviceMotion` now guards against `accelerationIncludingGravity === null` rather than crashing. |
`prefer-const` flipped `export let autoFocus = true` → `const` during
the .js → .ts conversion lint pass because nothing in the module
reassigns it. The JSDoc still describes it as user-settable behaviour
("Specify whether to automatically bring the window to the front") —
`let` keeps the door open for an internal setter without forcing
another module-shape change.
Behaviourally moot today: ESM namespace-import bindings
(`device.autoFocus = false` via `import * as device`) are read-only
regardless of `let` / `const`, so external mutation never worked
either way. But intent matters.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copilot review batch from the platform.ts and device.ts review rounds: - platform.ts header doc: add missing space before \`true\` for BlackBerry / Kindle entries (rendered as malformed markdown otherwise). - platform.ts @deprecated: prepend `since 19.7.0 — ` to wp / BlackBerry / Kindle / android2 to match the codebase's existing convention (matches video.js, renderable.js, entity.js style). - platform.ts isMobile: drop redundant `|| false` from the OR chain (every operand is already a boolean). - tests/platform.spec.ts: rename "Mac touch-bar laptop" test — Touch Bar isn't a touchscreen and doesn't report maxTouchPoints. The test is about the `maxTouchPoints === 1` edge case directly. - device.ts isMobile JSDoc: drop the dead-platform list (BlackBerry, Windows Phone, Kindle) — they're no longer in the isMobile OR chain per the upstream platform.ts change. - device.ts getElement JSDoc: drop "or null if not existing" — the function falls back to `document.body` and never returns null. - device.ts domRect cache: revert `new DOMRect(...)` → plain object literal so module load doesn't ReferenceError in Node / SSR environments where the DOMRect constructor isn't defined. The literal is cast to `DOMRect` at the return site. - CHANGELOG: rephrase the `system/device` conversion entry to make the rename explicit ("renamed from device.js → device.ts" rather than referring readers to a path that no longer exists), and drop the (now-reverted) DOMRect claim. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (3)
packages/melonjs/src/system/device.ts:136
touchandmaxTouchPointsdereferenceglobalThis.navigator.maxTouchPointswithout guarding thatnavigatorexists. In SSR/non-browser contexts (or environments with partial DOM polyfills), this can throw at module-load time and prevent importingdeviceat all. Guardnavigatorconsistently (similar toplatform.ts) so both constants are safe to evaluate.
packages/melonjs/src/system/device.ts:746watchAccelerometer()requests permission viaDeviceOrientationEvent.requestPermission, but the permission gate fordevicemotionon iOS isDeviceMotionEvent.requestPermission(). Using the orientation constructor here can cause accelerometer setup to fail even thoughhasAccelerometeris true.
packages/melonjs/src/system/device.ts:472exitFullscreenassumesdocument.exitFullscreen()always exists and always returns a Promise (because.catch(...)is chained). In older fullscreen implementations you may only have vendor-prefixed exit APIs (or a void return), so this can throw even whenhasFullscreenSupportis true.
| ); | ||
| globalThis.addEventListener("keyup", keyUpEvent, false); | ||
| } | ||
| if (globalThis.addEventListener) { |
Comment on lines
+105
to
+110
| // The module computes `iOS` at load time from `globalThis`, so | ||
| // these tests assert the LOGIC of the documented check by | ||
| // recreating it inline against stubbed navigator shapes. This | ||
| // is verification of the contract; the runtime-load value in | ||
| // real chromium is covered by the shape / desktop-defaults | ||
| // blocks above. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1467.
What this fixes
`src/system/platform.ts` had two real problems flagged during the 19.7 audit:
1. Modern iPads (iPadOS 13+, since Sept 2019) report as desktop
Safari on iPad ships the desktop Mac UA — no `iPad` token. The `/iPhone|iPad|iPod/i` regex missed every iPad sold in the last ~7 years; they all fell through `isMobile` as desktop. Every internal consumer (`keyboard.ts`, `application.ts`, `header.ts`) was getting the wrong answer for ~30% of mobile traffic.
2. Dead-platform regex noise
`wp` (Windows Phone, EOL 2017), `BlackBerry` (BB10 EOL 2016), `Kindle` (negligible share, behaves like Android), `android2` (Android 2.x = 2009-2011). Each is a regex run on every page load testing for hardware nobody ships.
How it's fixed
iPad detection
Layer a feature check on top of the UA regex:
```ts
const isIPadOnMacUA =
navigator?.platform === "MacIntel" && (navigator?.maxTouchPoints ?? 0) > 1;
export const iOS = /iPhone|iPad|iPod/i.test(ua) || isIPadOnMacUA;
```
Worth noting: `navigator.platform === "MacIntel"` is NOT a CPU check. Apple deliberately freezes the legacy string for backwards compat (same trick Windows uses with `Win32` on 64-bit). Apple Silicon Macs / iPads (M1, M2, M3, M4) all report `MacIntel`. The `maxTouchPoints > 1` clause is what actually separates iPads from real Macs — Macs don't have touchscreens.
Every existing internal consumer of `isMobile` inherits the fix transparently — no other files changed.
Dead-platform deprecation
`@deprecated` JSDoc on `wp` / `BlackBerry` / `Kindle` / `android2`. Exports stay functional through 19.x for backwards compat (any external consumer keeps working); IDE warnings light up at call sites. Removal scheduled for 20.x.
Also dropped these from `isMobile`'s OR chain. Remaining `/Mobi/.test(ua) || iOS || android` covers ~99.9% of 2026 mobile traffic per MDN's recommendation.
Won't-add: `isTouch`
The original issue suggested a new `isTouch` flag. We already have `device.touch` at `system/device.js:116` — feature-detected via `navigator.maxTouchPoints` / pointer events. CHANGELOG migration note points there for new code.
Tests
Six new cases in `tests/platform.spec.ts` covering the documented iPad-detection contract:
Existing 20 shape / desktop-defaults assertions kept.
Test plan
Follow-ups (separate issues worth filing)
Out of this PR's scope:
🤖 Generated with Claude Code