update accessibility mode on window reload

[meganrogge]

fixes #167583

[deepak1556]

Blocked by the chromium fix but works in OSS

Is this referring to #167960 (comment) ? That would be only for macOS.

[meganrogge]

Oh ok great then this should be good to review and merge

[bpasero]

Why does this have to be a IPC message sent from app.ts and why can this not be one of the configuration options that we pass into a window here:

vscode/src/vs/platform/windows/electron-main/windowsMainService.ts

Line 1329 in 1c85aea

const configuration: INativeWindowConfiguration = {

[meganrogge]

open is called in places where app refers to CodeApplication and thus does not have an isAccessibilitySupportEnabled function to call

I'm not familiar with this code - would you recommend I save the value of that before this

vscode/src/vs/code/electron-main/app.ts

Line 870 in 71cd323

const app = this;

happens and pass that in to all of the open calls?

[bpasero]

@meganrogge windowImpl is still in electron-main and has access to Electron app module, see for example:

vscode/src/vs/platform/windows/electron-main/windowImpl.ts

Line 485 in b928944

app.focus({ steal: true });

Same goes for windowsMainService if we want to put this into the config.

Bottom line: instead of sending an IPC message to the window, I would send this information to the window as part of the config when opening.

[meganrogge]

it turns out, we're already passing that in as an option, but not updating the accessibility service on reload

vscode/src/vs/platform/windows/electron-main/windowsMainService.ts

Line 1380 in a3dd458

accessibilitySupport: app.accessibilitySupportEnabled,

[bpasero]

@meganrogge there is a way to update the configuration explicitly when doing a window reload, see here:

vscode/src/vs/platform/windows/electron-main/windowImpl.ts

Line 1043 in 422b581

async reload(cli?: NativeParsedArgs): Promise<void> {

For example we update some properties here:

vscode/src/vs/platform/windows/electron-main/windowImpl.ts

Lines 1069 to 1070 in 422b581

	configuration.policiesData = this.policyService.serialize(); // set policies data again
	configuration.continueOn = this.environmentMainService.continueOn;

I would think we can set accessibility mode too.

[meganrogge]

@bpasero Thanks for the code pointers and continued discussion.

Based on logging, the values on environmentService.window do not get updated on reload and I don't think that's possible

It seems that consumers of INativeWorkbenchEnvironmentService end up using the storage service for this case instead

For example:

vscode/src/vs/workbench/services/themes/electron-sandbox/nativeHostColorSchemeService.ts

Lines 28 to 59 in 60bbd42

	constructor(
	@INativeHostService private readonly nativeHostService: INativeHostService,
	@INativeWorkbenchEnvironmentService environmentService: INativeWorkbenchEnvironmentService,
	@IStorageService private storageService: IStorageService
	) {
	super();
	// register listener with the OS
	this._register(this.nativeHostService.onDidChangeColorScheme(scheme => this.update(scheme)));
	const initial = this.getStoredValue() ?? environmentService.window.colorScheme;
	this.dark = initial.dark;
	this.highContrast = initial.highContrast;
	// fetch the actual value from the OS
	this.nativeHostService.getOSColorScheme().then(scheme => this.update(scheme));
	}
	private getStoredValue(): IColorScheme | undefined {
	const stored = this.storageService.get(NativeHostColorSchemeService.STORAGE_KEY, StorageScope.APPLICATION);
	if (stored) {
	try {
	const scheme = JSON.parse(stored);
	if (isObject(scheme) && isBoolean(scheme.highContrast) && isBoolean(scheme.dark)) {
	return scheme as IColorScheme;
	}
	} catch (e) {
	// ignore
	}
	}
	return undefined;
	}

When I open a window, the property is known. When I changed the theme to light and reloaded, logging the value of the theme shows that it was not correct (thus the necessity of the storage service)

[bpasero]

@meganrogge theming might do something very special for other reasons, do not take it as an example. The values of the environment service will be updated based on the main side configuration from the reload call, can you try it?

[meganrogge]

I tried that and the value was undefined - perhaps because of a timing issue?

[meganrogge]

I noticed continueOn also uses the storage service and policiesData is used in initServices - which we can't do for the accessibilityService

[bpasero]

@meganrogge a simple example works for me, here are my changes:

In windowImpl.ts.reload():

configuration.verbose = true;

In constructor of window.ts:

console.log(this.environmentService.verbose);

On first start I get false and on reload true.

[meganrogge]

It does appear to be working for me now

[meganrogge]

Oh I had my old changes still, hopefully it will work when I remove them as well

Think I see what's wrong though

[bpasero]

You do not need the changes in argv.ts if you add the property to INativeWindowConfiguration:

vscode/src/vs/platform/window/common/window.ts

Line 276 in d1f42f4

export interface INativeWindowConfiguration extends IWindowConfiguration, NativeParsedArgs, ISandboxConfiguration {

[bpasero]

We still need to somewhere read the accessibilitySupport value in the workbench, no?

[meganrogge]

@bpasero it was already on INativeWindowConfiguration

[meganrogge]

that already happens here

vscode/src/vs/workbench/services/accessibility/electron-sandbox/accessibilityService.ts

Line 44 in 84b9f93

this.setAccessibilitySupport(environmentService.window.accessibilitySupport ? AccessibilitySupport.Enabled : AccessibilitySupport.Disabled);

[bpasero]

Cool!

[meganrogge]

Thanks for the help @bpasero 👍🏼