Skip to content

picking.js: unguarded #fly-btn deref throws on canvas pointerdown during walk/fly #97

Description

@chertsa

picking.js:104-105 dereferences the removed pill button without a null guard:

A.canvas.addEventListener('pointerdown', (e) => {
  ...
  if (A.flyActive || A.walkMode) {
    A.flyActive = false;
    A.walkMode = false;
    A.walkPath = [];
    document.getElementById('fly-btn').style.background = '#444';   // ← null at this SHA
    document.getElementById('fly-btn').style.color = '#fff';
    document.getElementById('walk-speed-btn').style.display = 'none';
  }

#fly-btn has no HTML element anywhere in deploy/dev at 55900e2d — the same removal
tour.js:9 documents as §S280: may be null (pill removed button). tour.js guards every
access to it (if (btn) at :27 and :43, if (_speedBtn) at :45); picking.js guards none.
It looks like one file was updated for the pill refactor and this one was missed —
picking.js:90-91 shows the author's normal discipline (const el = …; if (el) …).

Reproduction (headless Chromium, Duplex model, this SHA):

  1. Open dev/index.html?db=/buildings/Duplex_extracted.db&lib=/buildings/Duplex_library.db
  2. Start the fly-around tour — window.toggleFlyAround(), the l key, or the ✈ pill.
    tour.js:348 sets A.walkMode = true and A.flyActive = true.
  3. Press the pointer down anywhere on #canvas.

Result: #fly-btn is confirmed absent at runtime (getElementById → null), and the tap
raises one uncaught page error — Cannot read properties of null (reading 'style'). Since
the throw is mid-handler, everything after line 105 in that pointerdown is skipped,
including the long-press → volume-card path.

Worth noting for whoever picks this up: walk mode and the tour set different flags.
toggleWalkMode + setWalkAnchor (what 03-walk-sitecam-cycle exercises) leaves
walkModeActive = true with walkMode and flyActive both false, so it never enters
this branch. Only the tour path does. That is likely why the suite is green here — and
03-walk-sitecam-cycle doesn't attach a pageerror listener either, though
helpers/console-capture.js already provides assertNoErrors() for exactly this.

(#walk-speed-btn on line 106 does exist, so that line is fine; only the two fly-btn
lines throw.)


Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions