Repository navigation
Conversation
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
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.
Drop mouse reports whose coordinates are
NaNor 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
NaNcoordinates:NaN, e.g.\x1b[<0;NaN;NaNm.String.fromCharCode(NaN)is\0.There are two known paths:
mouseuplistener is still ondocument.getComputedStyleon the detached element returns an empty padding, andparseInt('')isNaN. This is the^[[<0;NaN;NaNmin Terminal sends\x1b[<0;NaN;NaNmto the pty when a file link is activated by ctrl+click microsoft/vscode#328696.clientX(_inertiainsrc/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:getCoordsRelativeToElementsubtractsparseInt(padding)and usesclientXas is. Either one can make the resultNaN.src/browser/services/MouseCoordsService.ts:33-46:getMouseReportCoordsclamps withMath.min/Math.max. Both returnNaNforNaN.src/browser/services/MouseService.ts:497-502: the range checke.col < 0 || e.col >= cols || ...is false forNaN, so the event passes. The encoders insrc/common/services/MouseStateService.ts:125-150then build the report.Fix
getMouseReportCoordsreturnsundefinedwhen the element-relative coordinates are not finite. Both callers (_sendEventand_handleTouchScrollAsWheel) already stop on!pos._triggerMouseEventalso rejects non-finite values, so other callers are covered too:!(e.col >= 0 && e.col < cols && e.row >= 0 && e.row < rows), which fails forNaN.xandy(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.tsfrom creating an inertia event withoutclientX. 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 changetouch.ts, so the two PRs do not conflict and can land in any order.Testing
MouseService _triggerMouseEvent>non-finite coords:NaNor±Infinityin col, row, x or y sends no report, with the DEFAULT, SGR and SGR_PIXELS encodings.MouseCoordsService.test.ts(new file): an event withoutclientX/clientY, or withNaN/Infinity, returnsundefined. A normal event still returns the cell and pixel coordinates."0"for the padding of a detached element, so the unit tests cover the detached case through the missing-clientXpath, not through an empty padding.npm run test-unit: 2408 passing.npm run lint: passes.Repro (not verified in a browser)
\x1b[?1000;1002;1006hto the terminal (button-event tracking, SGR encoding).term.element.remove().Expected before: the data handler receives
\x1b[<0;NaN;NaNm. Expected after: no report.Notes
getMouseReportCoordsalready returnsundefinedthere, and no report is sent.