Inspector: Fix Firefox error reading child window bounds after close#18389
Conversation
|
Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s). |
|
Self code review completed by the code-review skill. No issues found. |
|
Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s). |
There was a problem hiding this comment.
Pull request overview
Fixes a Firefox-specific runtime error in the shared ChildWindow HOC used by Inspector/tooling UIs, where reading window-bound properties during React effect cleanup can throw after the popout window has already closed.
Changes:
- Cache the last-known child window bounds via a
getBounds()helper andlastBoundsvariable. - Update
lastBoundson initial open and again in abeforeunloadlistener to capture bounds while the window is still valid. - Persist
lastBoundstolocalStorageduring dispose, only re-reading live bounds when the window is still open.
|
Snapshot stored with reference name: Test environment: To test a playground add it to the URL, for example: https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/18389/merge/index.html#WGZLGJ#4600 Links to test your changes to core in the published versions of the Babylon tools (does not contain changes you made to the tools themselves): https://playground.babylonjs.com/?snapshot=refs/pull/18389/merge To test the snapshot in the playground with a playground ID add it after the snapshot query string: https://playground.babylonjs.com/?snapshot=refs/pull/18389/merge#BCU1XR#0 If you made changes to the sandbox or playground in this PR, additional comments will be generated soon containing links to the dev versions of those tools. |
|
You have changed file(s) that made possible changes to the sandbox. https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/SANDBOX/refs/pull/18389/merge/ |
|
You have made possible changes to the playground. https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/PLAYGROUND/refs/pull/18389/merge/ The snapshot playground with the CDN snapshot (only when available): Note that neither Babylon scenes nor textures are uploaded to the snapshot directory, so some playgrounds won't work correctly. |
|
I am working on fixing the issue with waiting for unneeded vis-tests. I will reference this PR in the fix. |
## Problem When `AFFECTED_TAGS` computes to `NONE` (e.g. a PR that only touches `shared-ui-components` or other non-vis-test code), the WebGL2 and WebGPU visualization test jobs were **skipped entirely** via job-level conditions: ```yaml condition: and(succeeded(), ne(dependencies.Build.outputs['ComputeVisTags.AFFECTED_TAGS'], 'NONE')) ``` This meant Azure DevOps never reported a status for these jobs. Since they are **required checks**, the PR was blocked from merging — even though there were no relevant tests to run. **Example:** #18389 — WebGL2 job shows as "expected" but never runs: [build logs](https://dev.azure.com/babylonjs/ContinousIntegration/_build/results?buildId=52627&view=logs&jobId=41c91092-7737-59ad-bca4-0f81503fea43&j=e5cc6727-28b8-51a4-d48e-c84cfcad289b) ## Fix - **Remove the job-level `NONE` condition** — both vis-test jobs now always run, so Azure DevOps always reports a status (satisfying the required check). - **Add step-level conditions** on `npm install` and the BrowserStack test step: `condition: ne(variables['AFFECTED_TAGS'], 'NONE')` — skips the expensive work when there are no affected tests. - **Add an echo step** (`"Skip — no affected tests"`) that runs when `NONE` — produces a clear green status in the build log. The job runs, reports green, and the required check is satisfied. No BrowserStack sessions are wasted. ## Changes - `.azure-pipelines/ci-monorepo.yml` — WebGL2 and WebGPU job conditions
…child-window-firefox-error
|
You have changed file(s) that made possible changes to the sandbox. https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/SANDBOX/refs/pull/18389/merge/ |
|
You have made possible changes to the playground. https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/PLAYGROUND/refs/pull/18389/merge/ The snapshot playground with the CDN snapshot (only when available): Note that neither Babylon scenes nor textures are uploaded to the snapshot directory, so some playgrounds won't work correctly. |
Problem
In
ChildWindow(packages/dev/sharedUiComponents/src/fluent/hoc/childWindow.tsx), the dispose action that persists the child window's bounds tolocalStoragereadsscreenX/screenY/innerWidth/innerHeightdirectly off the childWindow. In Firefox, accessing these properties after the window has closed throws — which is exactly what happens when the user closes the popout (theunloadhandler clearschildWindowstate, the effect re-runs, and the cleanup fires whilechildWindow.closed === true).Fixes https://forum.babylonjs.com/t/introducing-inspector-v2/60937/206
Fix
Cache the bounds while the window is still alive and use the cached value at dispose time:
getBounds()helper.lastBoundseagerly when the window opens, so a value is always available even if no later capture happens.lastBoundsin abeforeunloadlistener (fires before the window is destroyed in every close path).lastBoundstolocalStorage.This eliminates the throw in Firefox while preserving the existing bounds-restore behavior in all browsers.
Files changed
packages/dev/sharedUiComponents/src/fluent/hoc/childWindow.tsx