Aux window does not work in Firefox

[jrieken]

Testing #213597

• linux VM
• FF browser 122.x
• F1 > Report Issue
• 🐛 only a blank window opens, the main window shows the error below in its console

Uncaught (in promise) TypeError: ct.document is undefined
    m dom.ts:1295
    ot dom.ts:1301
    xe dom.ts:1357
    u button.ts:172
    o issue.ts:88
    y issueReporterService.ts:31
    o instantiationService.ts:162
    createInstance instantiationService.ts:128
    openAuxIssueReporter issueFormService.ts:121

[jrieken]

Benjamin Pasero (@bpasero) This might be a general FF issue, moving an editor into a new window also fails

[jrieken]

The same happens with latest FF (126) on macOS

[bpasero]

Wow, this is really bad and I am not sure something we can actually fix with our current assumptions. As it turns out xy instanceof HTMLElement returns false on Firefox when an element is inside the floating window, while it is totally fine in other browsers. For floating windows, the first assertion that breaks is this one:

vscode/src/vs/workbench/browser/parts/editor/editorPart.ts

Line 937 in 38b63e5

assertType(container instanceof HTMLElement);

We have code in place to mitigate this for certain types such as events:

vscode/src/vs/base/browser/dom.ts

Lines 1151 to 1154 in 38b63e5

	export function isMouseEvent(e: unknown): e is MouseEvent {
	// eslint-disable-next-line no-restricted-syntax
	return e instanceof MouseEvent || e instanceof getWindow(e as UIEvent).MouseEvent;
	}

But we never wanted to adopt it for DOM elements because they are actually often created in the main window and only later moved to the floating window.

I wonder why Firefox behaves different here, but its possible that they actually behave more correct than others 🤔

[bpasero]

Just to clarify: maybe its easier to fix for the issue reporter because that one is more scoped to a smaller set of problems. We could start adding those utility methods for the issue reporter and then see whats remaining for the floating windows.

But I think adopting this through our code is quite a thing, and we would need methods for basically each kind of HTML element + lint rules to prevent this from being called...

[justschen]

Benjamin Pasero (@bpasero) I'm assuming it would be something along the lines of:

export function isHTMLElement(e: unknown): e is MouseEvent {
	// eslint-disable-next-line no-restricted-syntax
	return e instanceof HTMLElement || e instanceof getWindow(e as Node).HTMLElement;
}

export function isTextAreaElement(e: unknown): e is HTMLTextAreaElement {
	// eslint-disable-next-line no-restricted-syntax
	return e instanceof HTMLTextAreaElement || e instanceof getWindow(e as Node).HTMLTextAreaElement;
}

....

how would this be used in the context of the issue reporter since I'm just calling this.auxiliaryWindowService.open()....?

[bpasero]

Yeah you are likely using a dom.ts or similar utility function that does an instanceof check, we would need to adopt it gradually through the usages.

[justschen]

gotcha, I see that now. I'l add the ones I am checking in my UI to dom.ts for now and we can see what other actions we need later if it works nicely with the issue reporter. if you'd like help with other non-issue reporter scenarios, let me know as well!

[justschen]

to clarify, would something like this be a cause of this as well?

<HTMLInputElement>this.getElementById('issue-title')

[bpasero]

I think there are actually more issues, I did some testing with most instanceof checks doing it the other way and the issue reporter is not styled:

And floating windows open and then quickly close again :-/