change: mouse camera navigation#6
Conversation
WalkthroughAdds a SPACE keyboard modifier and Space-key tracking; updates camera input mappings and wheel modifier checks to include ALT and SPACE; suppresses tileset console logs when no listeners exist; bumps engine package version to 15.0.37. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User as User
participant Input as Input Device
participant SSEH as ScreenSpaceEventHandler
participant Agg as CameraEventAggregator
participant Ctrl as ScreenSpaceCameraController
participant Cam as Camera
User->>Input: Drag / Wheel / Pinch / Key (Space/Shift/Alt)
Input->>SSEH: Raw event (mouse/touch/wheel/keydown/keyup)
SSEH->>SSEH: Track _spaceKeyDown on keydown/keyup
SSEH->>Agg: Normalized event (type + modifier including SPACE)
Agg->>Agg: anyButtonDown / wheelMoved checks (now includes ALT, SPACE)
Agg-->>Ctrl: CameraEvent (type + modifier)
Ctrl->>Ctrl: Match event to mappings (zoom/rotate/tilt/look)
Ctrl->>Cam: Apply camera action
Cam-->>User: Updated view
sequenceDiagram
autonumber
participant Loader as Tile Loader
participant Tileset as Cesium3DTileset
participant Lsn as tileFailed Listeners
participant Log as Console
Loader->>Tileset: Tile load fails (error)
alt Has tileFailed listeners
Tileset-->>Lsn: Raise tileFailed(error)
Tileset->>Log: (may log as before)
else No listeners
Note over Tileset: Suppress prior console logging (no output)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
🔴 There was an error checking the CLA! If this is your first contribution, please send in a Contributor License Agreement. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/engine/Source/Scene/Cesium3DTileset.js (1)
662-666: Update JSDoc to match new no-log behavior (if you keep it silent).The docs promise console logging; current code suppresses it. Align the comment as in the diff above to avoid misleading users.
packages/engine/Source/Scene/CameraEventAggregator.js (1)
385-397: Add or remove SPACE modifier handling
- ScreenSpaceEventHandler has no keydown/keyup or event logic for SPACE (KeyboardEventModifier.SPACE), so CameraEventAggregator’s SPACE bindings (lines 385–397) never fire. Either implement SPACE detection in ScreenSpaceEventHandler or omit SPACE from the modifier loop.
🧹 Nitpick comments (2)
packages/engine/Source/Scene/ScreenSpaceCameraController.js (2)
187-193: Rotate mapping uses SPACE; confirm handler support and update docs.Rotation now uses MIDDLE_DRAG and SPACE+LEFT_DRAG. Update the JSDoc default and ensure SPACE is supported by the event system; MIDDLE button may be unavailable on many devices.
Doc fix:
- * @default {@link CameraEventType.LEFT_DRAG} + * @default [{@link CameraEventType.MIDDLE_DRAG}, { eventType: {@link CameraEventType.LEFT_DRAG}, modifier: {@link KeyboardEventModifier.SPACE} }]Consider keeping LEFT_DRAG (no modifier) in the list for devices without a middle button (behind a flag if necessary).
169-176: Add CHANGES.md and unit/integration tests for gesture remapping.Given the new defaults, add:
- CHANGES.md entry outlining the new gesture map.
- Tests that the controller reacts to WHEEL/ALT/SPACE and modified drags as intended in 2D/CV/3D.
Would you like me to draft CHANGES.md and a few Jasmine specs targeting these mappings?
Also applies to: 187-193, 210-210, 221-224
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
packages/engine/Source/Core/KeyboardEventModifier.js(1 hunks)packages/engine/Source/Scene/CameraEventAggregator.js(1 hunks)packages/engine/Source/Scene/Cesium3DTileset.js(1 hunks)packages/engine/Source/Scene/ScreenSpaceCameraController.js(4 hunks)packages/engine/package.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/engine/Source/Scene/CameraEventAggregator.js (1)
packages/engine/Source/Core/KeyboardEventModifier.js (1)
KeyboardEventModifier(7-39)
packages/engine/Source/Scene/ScreenSpaceCameraController.js (1)
packages/engine/Source/Core/KeyboardEventModifier.js (1)
KeyboardEventModifier(7-39)
| { | ||
| "name": "@mach9/engine", | ||
| "version": "15.0.35", | ||
| "version": "15.0.36", |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Bump version as a behavior change (input remapping), not a patch.
Mouse/keyboard gesture defaults changed; this is user-facing and potentially breaking. Use at least a minor bump (e.g., 15.1.0) or major if adhering to semver strictly.
- "version": "15.0.36",
+ "version": "15.1.0",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "version": "15.0.36", | |
| "version": "15.1.0", |
🤖 Prompt for AI Agents
packages/engine/package.json lines 3-3: The version is set to "15.0.36" but the
change is a user-facing behavior change (input remapping), so update the semver
bump to at least a minor release; change the version string to "15.1.0" (or
higher if you prefer a different minor/major policy) and update any related
changelog/release notes and CI/release tooling to reflect the new version.
| /** | ||
| * Represents the space key being held down. | ||
| * | ||
| * @type {number} | ||
| * @constant | ||
| */ | ||
| SPACE: 3, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Adding SPACE is fine, but wire it end-to-end.
Ensure ScreenSpaceEventHandler detects the Space key and maps it to this constant; otherwise, SPACE-based gestures won’t work.
Would you like a follow-up patch to add SPACE handling to ScreenSpaceEventHandler (keydown/up, preventDefault on canvas to avoid page scroll)?
🏁 Script executed:
#!/bin/bash
set -e
# Locate ScreenSpaceEventHandler source file
file=$(fd ScreenSpaceEventHandler.js -t f)
echo "ScreenSpaceEventHandler file: $file"
# Inspect keydown and keyup handlers around event registration
grep -n -C3 "addEventListener.*keydown" "$file"
grep -n -C3 "addEventListener.*keyup" "$file"
# Search for mapping of SPACE constant
grep -n "KeyboardEventModifier\\.SPACE" "$file"
# Check any preventDefault logic in key handlers
grep -n "preventDefault" -C2 "$file"Length of output: 384
🏁 Script executed:
#!/bin/bash
set -e
# Locate ScreenSpaceEventHandler source file
file=$(fd ScreenSpaceEventHandler.js -t f)
echo "File: $file"
# Check for key event handler methods
echo "\n-- this._onKeyDown references --"
grep -n "this\\._onKeyDown" -C2 "$file" || true
echo "\n-- this._onKeyUp references --"
grep -n "this\\._onKeyUp" -C2 "$file" || true
# Check for listener registrations
echo "\n-- keydown/keyup listeners --"
grep -n "addEventListener.*keydown" -C2 "$file" || true
grep -n "addEventListener.*keyup" -C2 "$file" || true
# Check for SPACE constant usage
echo "\n-- KeyboardEventModifier.SPACE usages --"
grep -n "KeyboardEventModifier\\.SPACE" -C2 "$file" || true
# Check for preventDefault calls in key handlers
echo "\n-- preventDefault calls --"
grep -n "preventDefault" -C2 "$file" || trueLength of output: 1341
🏁 Script executed:
#!/bin/bash
# Display handlers and mapping around SPACE constant in ScreenSpaceEventHandler
sed -n '160,240p' packages/engine/Source/Core/ScreenSpaceEventHandler.jsLength of output: 3319
Wire SPACE key handling in ScreenSpaceEventHandler
ScreenSpaceEventHandler currently has no keydown/keyup listeners for the Space key, so KeyboardEventModifier.SPACE is never activated. Add listeners (e.g. on the canvas) to set/reset this modifier on keydown/keyup for keyCode 32 (or event.key === ' '), and call preventDefault() on Space to avoid page scrolling, ensuring the modifier: KeyboardEventModifier.SPACE mapping in rotateEventTypes works as intended.
🤖 Prompt for AI Agents
In packages/engine/Source/Core/KeyboardEventModifier.js around lines 32 to 38,
the SPACE modifier is defined but ScreenSpaceEventHandler never wires
keydown/keyup for the Space key so the modifier is never set; add keydown and
keyup listeners (attached to the canvas element) that detect Space (event.key
=== ' ' or event.keyCode === 32), set KeyboardEventModifier.SPACE on keydown and
clear it on keyup, call event.preventDefault() on keydown to prevent page
scrolling, and ensure these listeners integrate with the existing modifier
handling so the rotateEventTypes mapping using modifier:
KeyboardEventModifier.SPACE functions correctly.
| !this._update[ | ||
| getKey(CameraEventType.WHEEL, KeyboardEventModifier.ALT) | ||
| ] || | ||
| !this._update[ | ||
| getKey(CameraEventType.WHEEL, KeyboardEventModifier.SPACE) | ||
| ]; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don’t hardcode wheel modifiers; derive from KeyboardEventModifier to stay future-proof.
This avoids touching this block every time a modifier is added.
- const wheelMoved =
- !this._update[getKey(CameraEventType.WHEEL)] ||
- !this._update[
- getKey(CameraEventType.WHEEL, KeyboardEventModifier.SHIFT)
- ] ||
- !this._update[
- getKey(CameraEventType.WHEEL, KeyboardEventModifier.CTRL)
- ] ||
- !this._update[
- getKey(CameraEventType.WHEEL, KeyboardEventModifier.ALT)
- ] ||
- !this._update[
- getKey(CameraEventType.WHEEL, KeyboardEventModifier.SPACE)
- ];
+ const wheelKeys = [undefined, ...Object.values(KeyboardEventModifier)];
+ const wheelMoved = wheelKeys.some(
+ (m) => !this._update[getKey(CameraEventType.WHEEL, m)]
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| !this._update[ | |
| getKey(CameraEventType.WHEEL, KeyboardEventModifier.ALT) | |
| ] || | |
| !this._update[ | |
| getKey(CameraEventType.WHEEL, KeyboardEventModifier.SPACE) | |
| ]; | |
| const wheelKeys = [undefined, ...Object.values(KeyboardEventModifier)]; | |
| const wheelMoved = wheelKeys.some( | |
| (m) => !this._update[getKey(CameraEventType.WHEEL, m)] | |
| ); |
| //console.log(`A 3D tile failed to load: ${url}`); | ||
| //console.log(`Error: ${message}`); | ||
| //console.log(error.stack); | ||
| } |
There was a problem hiding this comment.
Silent failure path now contradicts the documented behavior.
You commented out console logging when there are no tileFailed listeners, but the JSDoc above still states errors will be logged. Fix docs or restore minimal logging.
Option A (update docs to reflect silence):
@@
/**
* The event fired to indicate that a tile's content failed to load.
* <p>
- * If there are no event listeners, error messages will be logged to the console.
+ * If there are no event listeners, no message is logged to the console.
* </p>Option B (keep low-noise logging):
} else {
- //console.log(`A 3D tile failed to load: ${url}`);
- //console.log(`Error: ${message}`);
- //console.log(error.stack);
+ oneTimeWarning(
+ "tileset-tileFailed-no-listeners",
+ "A 3D tile failed to load, but no tileFailed listeners are registered."
+ );
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/engine/Source/Scene/Cesium3DTileset.js around lines 2670-2673, the
code comments out console logging for tile load failures which contradicts the
existing JSDoc that promises errors will be logged; either update the JSDoc to
state that failures are silent when there are no tileFailed listeners (Option A)
or restore minimal low-noise logging (Option B) — if you choose Option A, edit
the JSDoc text to explicitly say no logging occurs when there are no listeners;
if you choose Option B, reintroduce a single minimal console.error/console.warn
call that logs the tile URL and error message (and stack if available) only when
there are no tileFailed listeners, keeping it concise and low-noise.
| this.zoomEventTypes = [ | ||
| CameraEventType.RIGHT_DRAG, | ||
| CameraEventType.WHEEL, | ||
| CameraEventType.PINCH, | ||
| { | ||
| eventType: CameraEventType.RIGHT_DRAG, | ||
| modifier: KeyboardEventModifier.SHIFT, | ||
| }, | ||
| ]; |
There was a problem hiding this comment.
Zoom mapping change: doc and compatibility.
Defaults changed to require SHIFT+RIGHT_DRAG for zoom (instead of plain RIGHT_DRAG). Update the JSDoc default and consider a feature flag to preserve legacy mappings for existing users.
Doc fix:
- * @default [{@link CameraEventType.RIGHT_DRAG}, {@link CameraEventType.WHEEL}, {@link CameraEventType.PINCH}]
+ * @default [{@link CameraEventType.WHEEL}, {@link CameraEventType.PINCH}, { eventType: {@link CameraEventType.RIGHT_DRAG}, modifier: {@link KeyboardEventModifier.SHIFT} }]Optional: expose an inputScheme: "legacy" | "space-alt" (default "space-alt") to switch mappings.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| this.zoomEventTypes = [ | |
| CameraEventType.RIGHT_DRAG, | |
| CameraEventType.WHEEL, | |
| CameraEventType.PINCH, | |
| { | |
| eventType: CameraEventType.RIGHT_DRAG, | |
| modifier: KeyboardEventModifier.SHIFT, | |
| }, | |
| ]; | |
| /** | |
| * The input event types to use for zooming the camera. | |
| * | |
| * @default [{@link CameraEventType.WHEEL}, {@link CameraEventType.PINCH}, { eventType: {@link CameraEventType.RIGHT_DRAG}, modifier: {@link KeyboardEventModifier.SHIFT} }] | |
| */ | |
| this.zoomEventTypes = [ | |
| CameraEventType.WHEEL, | |
| CameraEventType.PINCH, | |
| { | |
| eventType: CameraEventType.RIGHT_DRAG, | |
| modifier: KeyboardEventModifier.SHIFT, | |
| }, | |
| ]; |
🤖 Prompt for AI Agents
In packages/engine/Source/Scene/ScreenSpaceCameraController.js around lines 169
to 176, the default zoom mapping was changed to require SHIFT+RIGHT_DRAG but the
JSDoc still documents the old plain RIGHT_DRAG behavior and there is no way to
opt into the legacy mapping; update the JSDoc default to reflect the new
SHIFT+RIGHT_DRAG mapping, and add an optional inputScheme option (e.g.
inputScheme: "legacy" | "space-alt" defaulting to "space-alt") to toggle between
the new mapping and the legacy RIGHT_DRAG behavior — implement logic that sets
this.zoomEventTypes based on the inputScheme (legacy -> RIGHT_DRAG, space-alt ->
SHIFT+RIGHT_DRAG) and surface the option in the constructor/API so existing
users can preserve the old mapping.
| modifier: KeyboardEventModifier.CTRL, | ||
| }, | ||
| ]; | ||
| this.tiltEventTypes = [CameraEventType.RIGHT_DRAG, CameraEventType.PINCH]; |
There was a problem hiding this comment.
Tilt mapping narrowed; update docs and check overlap with new zoom mapping.
Tilt is now RIGHT_DRAG (no modifier) and PINCH. Update the JSDoc block accordingly. Verify no conflict with SHIFT+RIGHT_DRAG zoom in practice (it should be fine via modifier-specific handlers).
Doc fix:
- * @default [{@link CameraEventType.MIDDLE_DRAG}, {@link CameraEventType.PINCH}, { ...CTRL combos... }]
+ * @default [{@link CameraEventType.RIGHT_DRAG}, {@link CameraEventType.PINCH}]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| this.tiltEventTypes = [CameraEventType.RIGHT_DRAG, CameraEventType.PINCH]; | |
| /** | |
| * The event types that will be interpreted as tilt | |
| * | |
| * @type {CameraEventType[]} | |
| * @default [{@link CameraEventType.RIGHT_DRAG}, {@link CameraEventType.PINCH}] | |
| */ | |
| this.tiltEventTypes = [CameraEventType.RIGHT_DRAG, CameraEventType.PINCH]; |
| this.lookEventTypes = { | ||
| eventType: CameraEventType.LEFT_DRAG, | ||
| modifier: KeyboardEventModifier.SHIFT, | ||
| modifier: KeyboardEventModifier.ALT, | ||
| }; |
There was a problem hiding this comment.
Look mapping changed to ALT; update the documented default.
Docs still mention SHIFT; change to ALT to avoid confusing integrators.
- * @default { eventType : {@link CameraEventType.LEFT_DRAG}, modifier : {@link KeyboardEventModifier.SHIFT} }
+ * @default { eventType : {@link CameraEventType.LEFT_DRAG}, modifier : {@link KeyboardEventModifier.ALT} }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| this.lookEventTypes = { | |
| eventType: CameraEventType.LEFT_DRAG, | |
| modifier: KeyboardEventModifier.SHIFT, | |
| modifier: KeyboardEventModifier.ALT, | |
| }; | |
| * @default { eventType : {@link CameraEventType.LEFT_DRAG}, modifier : {@link KeyboardEventModifier.ALT} } |
🤖 Prompt for AI Agents
In packages/engine/Source/Scene/ScreenSpaceCameraController.js around lines 221
to 224, the code sets the look modifier to ALT but documentation/comments still
say SHIFT; update the documented default to ALT. Locate any inline comments,
JSDoc, README, or API docs that state the default look modifier as SHIFT and
change those references to ALT (and update any examples showing SHIFT key
usage), and run a quick grep across the repo for "look mapping", "look
modifier", "SHIFT", or "default look" to ensure all user-facing docs and
examples are consistent with the new ALT mapping.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/engine/Source/Core/ScreenSpaceEventHandler.js (3)
34-46: SPACE modifier precedence: confirm intended behavior (single modifier only).As implemented, SPACE is only returned when SHIFT/CTRL/ALT are not pressed. If combos like SHIFT+SPACE are desired, current single-modifier model won’t express them. Please confirm this precedence matches the updated mappings in ScreenSpaceCameraController and CameraEventAggregator.
62-67: Mirror robust Space detection in keyup.Small consistency improvement; no behavior change otherwise.
-function handleKeyUp(screenSpaceEventHandler, event) { - if (event.keyCode === 32) { +function handleKeyUp(screenSpaceEventHandler, event) { + if (event.code === "Space" || event.keyCode === 32 || event.key === " ") { // Space key code screenSpaceEventHandler._spaceKeyDown = false; } }
1022-1023: State init: LGTM; optional JSDoc for clarity.Consider a brief private JSDoc (e.g., “Tracks whether Space is currently pressed”) to aid readers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/engine/Source/Core/ScreenSpaceEventHandler.js(11 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/engine/Source/Core/ScreenSpaceEventHandler.js (1)
packages/engine/Source/Core/KeyboardEventModifier.js (1)
KeyboardEventModifier(7-39)
🔇 Additional comments (1)
packages/engine/Source/Core/ScreenSpaceEventHandler.js (1)
262-267: Passing handler into getModifier: LGTM.Call sites consistently forward screenSpaceEventHandler so SPACE state is respected across mouse, touch, dblclick, and wheel paths.
Also applies to: 291-300, 386-399, 432-437, 476-481, 569-607, 779-817
| function handleKeyDown(screenSpaceEventHandler, event) { | ||
| if (event.keyCode === 32) { | ||
| // Space key code | ||
| screenSpaceEventHandler._spaceKeyDown = true; | ||
| event.preventDefault(); // Prevent page scrolling | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Don’t preventDefault on Space globally—blocks typing and page scroll outside the canvas.
Keydown on document will swallow spaces in inputs/contenteditable and stop page scrolling site-wide. Restrict preventDefault to events originating within the handler element and non-editable targets; also broaden Space detection for robustness.
-function handleKeyDown(screenSpaceEventHandler, event) {
- if (event.keyCode === 32) {
- // Space key code
- screenSpaceEventHandler._spaceKeyDown = true;
- event.preventDefault(); // Prevent page scrolling
- }
-}
+function handleKeyDown(screenSpaceEventHandler, event) {
+ // Robust Space detection across browsers
+ if (event.code === "Space" || event.keyCode === 32 || event.key === " ") {
+ screenSpaceEventHandler._spaceKeyDown = true;
+
+ // Only block default if inside our element and not an editable control
+ const el = screenSpaceEventHandler._element;
+ const t = event.target;
+ const inside =
+ t === el || (el && typeof el.contains === "function" && el.contains(t));
+ const isEditable =
+ t &&
+ (t.isContentEditable ||
+ /^(input|textarea|select)$/i.test(t.tagName));
+ if (inside && !isEditable) {
+ event.preventDefault(); // Prevent page scroll when interacting with the canvas
+ }
+ }
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function handleKeyDown(screenSpaceEventHandler, event) { | |
| if (event.keyCode === 32) { | |
| // Space key code | |
| screenSpaceEventHandler._spaceKeyDown = true; | |
| event.preventDefault(); // Prevent page scrolling | |
| } | |
| } | |
| function handleKeyDown(screenSpaceEventHandler, event) { | |
| // Robust Space detection across browsers | |
| if (event.code === "Space" || event.keyCode === 32 || event.key === " ") { | |
| screenSpaceEventHandler._spaceKeyDown = true; | |
| // Only block default if inside our element and not an editable control | |
| const el = screenSpaceEventHandler._element; | |
| const t = event.target; | |
| const inside = | |
| t === el || (el && typeof el.contains === "function" && el.contains(t)); | |
| const isEditable = | |
| t && | |
| (t.isContentEditable || | |
| /^(input|textarea|select)$/i.test(t.tagName)); | |
| if (inside && !isEditable) { | |
| event.preventDefault(); // Prevent page scroll when interacting with the canvas | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In packages/engine/Source/Core/ScreenSpaceEventHandler.js around lines 54-61,
the current keydown handler unconditionally preventsDefault for Space, which
blocks typing and scrolling site-wide; update it to (1) detect Space robustly
using event.key === ' ' || event.code === 'Space' || event.keyCode === 32, (2)
ensure the event originated from the handler's element (or its descendant)
before acting, and (3) skip calling preventDefault if the event.target is an
editable element (input, textarea, or has contentEditable not "false"); only
when the event is a Space key, comes from inside the handler element, and the
target is non-editable set _spaceKeyDown = true and call preventDefault.
| // Add keyboard event listeners for space key tracking | ||
| registerListener( | ||
| screenSpaceEventHandler, | ||
| "keydown", | ||
| alternateElement, | ||
| handleKeyDown, | ||
| ); | ||
| registerListener( | ||
| screenSpaceEventHandler, | ||
| "keyup", | ||
| alternateElement, | ||
| handleKeyUp, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clear “stuck” SPACE state on window blur/page hide.
If keyup is missed (tab switch, window blur, visibility change), _spaceKeyDown can get stuck true. Add blur/visibilitychange listeners to reset ephemeral keyboard state.
registerListener(
screenSpaceEventHandler,
"keyup",
alternateElement,
handleKeyUp,
);
+
+ // Reset transient keyboard state if focus is lost or page is hidden
+ registerListener(
+ screenSpaceEventHandler,
+ "blur",
+ window,
+ handleBlurOrVisibilityChange,
+ );
+ registerListener(
+ screenSpaceEventHandler,
+ "visibilitychange",
+ document,
+ handleBlurOrVisibilityChange,
+ );Add this helper near other handlers:
function handleBlurOrVisibilityChange(screenSpaceEventHandler, event) {
if (event.type === "blur" || document.visibilityState === "hidden") {
screenSpaceEventHandler._spaceKeyDown = false;
}
}🤖 Prompt for AI Agents
In packages/engine/Source/Core/ScreenSpaceEventHandler.js around lines 190 to
202, add handling to clear a stuck _spaceKeyDown when the page loses focus or
visibility changes: register blur and visibilitychange listeners on the same
element or window and implement a small handler that sets
screenSpaceEventHandler._spaceKeyDown = false when event.type === "blur" or
document.visibilityState === "hidden"; attach this handler alongside the
existing keydown/keyup registrations and remove it on destroy if applicable.
1 participant
Description
Issue number and link
Testing plan
Author checklist
CONTRIBUTORS.mdCHANGES.mdwith a short summary of my changeSummary by CodeRabbit
New Features
Changes
Bug Fixes
Chores