TransformControls dispose() crashes when domElement is not passed in constructor

[nidal-rr]

Description

If TransformControls is created without domElement parameter, which is marked as optional, and then dispose() method is called, "this.domElement is null" error occurs and crashes the application.

Reproduction steps

1. create TransformControls without the domElement parameter
2. call dispose() method

Quickest way to see this is to change examples/misc_controls_transform.html line

				control = new TransformControls( currentCamera, renderer.domElement );

to

				control = new TransformControls( currentCamera );
				control.dispose();

and reload page, Dev Tools console will show the "this.domElement is null" error inside the dispose() method.

The solution to this is to check for this.domElement in both connect() and disconnect() methods like so:

	connect() {

		if( this.domElement == null ) return;

		this.domElement.addEventListener( 'pointerdown', this._onPointerDown );
		this.domElement.addEventListener( 'pointermove', this._onPointerHover );
		this.domElement.addEventListener( 'pointerup', this._onPointerUp );

		this.domElement.style.touchAction = 'none'; // disable touch scroll

	}

	disconnect() {

		if( this.domElement == null ) return;

		this.domElement.removeEventListener( 'pointerdown', this._onPointerDown );
		this.domElement.removeEventListener( 'pointermove', this._onPointerHover );
		this.domElement.removeEventListener( 'pointermove', this._onPointerMove );
		this.domElement.removeEventListener( 'pointerup', this._onPointerUp );

		this.domElement.style.touchAction = 'auto';

	}

and remove the

if ( domElement !== null ) {

check in constructor which only guards call to connect().

Code

let control = new TransformControls( currentCamera );
control.dispose(); //<-- this crashes with "this.domElement is null"

Live example

• jsfiddle-latest-release WebGLRenderer
• jsfiddle-dev WebGLRenderer
• jsfiddle-latest-release WebGPURenderer
• jsfiddle-dev WebGPURenderer

Screenshots

No response

Version

0.170.0

Device

No response

Browser

No response

OS

No response

[Mugen87]

Calling connect() without a valid domElement defined is actually an error and should not silently fail.

Since calling disconnect() without a previous connect() is also an error situation, I think it's not right to implement the early out. I would rather suggest to throw exceptions in this case. We should not support operations that make no sense.

[Mugen87]

Given the policy that the engines usually does not validate user input, there is not necessarily a need for a change. The existing runtime error should make the error situation already clear.

[nidal-rr]

I got impression that the domElement constructor parameter is optional from the TransformControls.js source code, because it seems marked as such:

constructor( camera, domElement = null ) {

The documentation also says so.
If that is the case, then statements like:

if ( this.domElement == null ) return;

would not constitute validation of user input, but implementation of a code logic that handles optional parameter, IMO.

Just my 2 cents, I might be wrong, haven't dwelled into the threejs source code much, if at all.

[Mugen87]

Closing. We can reconsider the issue when if mode devs complain about the current behavior.