Skip to content

Don't send mouse reports with non-finite coordinates - #6204

Open
MocA-Love wants to merge 1 commit into
xtermjs:masterfrom
MocA-Love:fix/non-finite-mouse-report-coords
Open

MocA-Love wants to merge 1 commit into
xtermjs:masterfrom
MocA-Love:fix/non-finite-mouse-report-coords

Conversation

@MocA-Love

Copy link
Copy Markdown

Drop mouse reports whose coordinates are NaN or infinite, instead of sending them to the pty.

Fixes #6108

Related: microsoft/vscode#328696

Problem

With mouse reporting on, xterm.js can send reports with NaN coordinates:

  • SGR: the report contains the text NaN, e.g. \x1b[<0;NaN;NaNm.
  • DEFAULT: the report contains NUL bytes, because String.fromCharCode(NaN) is \0.

There are two known paths:

  1. The terminal element is removed from the DOM while a mouse button is held. The mouseup listener is still on document. getComputedStyle on the detached element returns an empty padding, and parseInt('') is NaN. This is the ^[[<0;NaN;NaNm in Terminal sends \x1b[<0;NaN;NaNm to the pty when a file link is activated by ctrl+click microsoft/vscode#328696.
  2. Inertia scrolling dispatches an event without clientX (_inertia in src/browser/scrollable/touch.ts), as described in Inertial fling sends NaN mouse coordinates when mouse reporting is on #6108.

Cause

  • src/browser/input/Mouse.ts:6-15: getCoordsRelativeToElement subtracts parseInt(padding) and uses clientX as is. Either one can make the result NaN.
  • src/browser/services/MouseCoordsService.ts:33-46: getMouseReportCoords clamps with Math.min / Math.max. Both return NaN for NaN.
  • src/browser/services/MouseService.ts:497-502: the range check e.col < 0 || e.col >= cols || ... is false for NaN, so the event passes. The encoders in src/common/services/MouseStateService.ts:125-150 then build the report.

Fix

  1. getMouseReportCoords returns undefined when the element-relative coordinates are not finite. Both callers (_sendEvent and _handleTouchScrollAsWheel) already stop on !pos.
  2. _triggerMouseEvent also rejects non-finite values, so other callers are covered too:
    • The col/row check is now !(e.col >= 0 && e.col < cols && e.row >= 0 && e.row < rows), which fails for NaN.
    • x and y (used by SGR_PIXELS) have no range check, so they must be finite.

How this relates to #6059

#6059 takes option 1 in #6108: it stops touch.ts from creating an inertia event without clientX. This PR takes options 2 and 3: the reporting path rejects bad coordinates, whatever the source. It also covers the detached-element path, which #6059 does not touch. This PR does not change touch.ts, so the two PRs do not conflict and can land in any order.

Testing

  • New tests:
    • MouseService _triggerMouseEvent > non-finite coords: NaN or ±Infinity in col, row, x or y sends no report, with the DEFAULT, SGR and SGR_PIXELS encodings.
    • MouseCoordsService.test.ts (new file): an event without clientX / clientY, or with NaN / Infinity, returns undefined. A normal event still returns the cell and pixel coordinates.
  • The 4 new failure-case tests fail without the fix and pass with it.
  • jsdom returns "0" for the padding of a detached element, so the unit tests cover the detached case through the missing-clientX path, not through an empty padding.
  • npm run test-unit: 2408 passing. npm run lint: passes.
  • I did not run the Playwright integration tests, and I did not reproduce the bug in a browser.

Repro (not verified in a browser)

  1. Open the demo and write \x1b[?1000;1002;1006h to the terminal (button-event tracking, SGR encoding).
  2. Press a mouse button in the terminal and keep it held.
  3. In the devtools console, run term.element.remove().
  4. Release the button.

Expected before: the data handler receives \x1b[<0;NaN;NaNm. Expected after: no report.

Notes

  • In the detached-element case, the button release is now not reported at all. The application does not get a release for that press. This is the same as the existing behavior when the char size is not valid: getMouseReportCoords already returns undefined there, and no report is sent.

getMouseReportCoords could return NaN coordinates, for example when the
terminal element is detached from the DOM while a button is held (the
computed padding is empty, so parseInt returns NaN) or when an inertia
scroll event has no clientX. The range check in _triggerMouseEvent let
NaN through, so SGR reports contained "NaN" and DEFAULT reports
contained NUL bytes.

- getMouseReportCoords returns undefined for non-finite coordinates
- _triggerMouseEvent rejects non-finite col/row/x/y

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Inertial fling sends NaN mouse coordinates when mouse reporting is on

1 participant