Skip to content

[A11y] Stop sending the brightness that the app reports back to the app #10071

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
victorsanni wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

[A11y] Stop sending the brightness that the app reports back to the app #10071

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 37 additions & 21 deletions packages/devtools_app/lib/src/screens/accessibility/accessibility_controller.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@ class AccessibilityController extends DevToolsScreenController
}

void _initListeners() {
addAutoDisposeListener(brightness, _onBrightnessChanged);
addAutoDisposeListener(textScale, _onTextScaleChanged);
addAutoDisposeListener(boldText, _onBoldTextChanged);
addAutoDisposeListener(screenReader, _onScreenReaderChanged);
Expand Down Expand Up @@ -94,35 +93,49 @@ class AccessibilityController extends DevToolsScreenController
}

void _initServiceExtensionStates() {
final state = serviceConnection.serviceManager.serviceExtensionManager
.getServiceExtensionState(extensions.brightnessMode.extension);
final serviceManager = serviceConnection.serviceManager;
late ValueListenable<ServiceExtensionState> state;

void updateFromDeviceState(ServiceExtensionState state) {
final newBrightness = !state.enabled || state.value == null
void updateFromDeviceState() {
final newBrightness = !state.value.enabled || state.value.value == null
? BrightnessOverride.system
: BrightnessOverride.values.firstWhere(
(b) => b.value == state.value,
(b) => b.value == state.value.value,
orElse: () => BrightnessOverride.system,
);
brightness.value = newBrightness;
_brightness.value = newBrightness;
}

updateFromDeviceState(state.value);
addAutoDisposeListener(state, () => updateFromDeviceState(state.value));
}
// ServiceExtensionManager stops updating the state that it returned once
// the main isolate exits, such as during a hot restart, so look the state
// up again whenever the main isolate changes.
void listenToDeviceState() {
cancelListener(updateFromDeviceState);
state = serviceManager.serviceExtensionManager.getServiceExtensionState(
extensions.brightnessMode.extension,
);
addAutoDisposeListener(state, updateFromDeviceState);
updateFromDeviceState();
}

void _onBrightnessChanged() {
final value = brightness.value;
unawaited(
serviceConnection.serviceManager.serviceExtensionManager
.setServiceExtensionState(
extensions.brightnessMode.extension,
enabled: value != BrightnessOverride.system,
value: value.value,
),
listenToDeviceState();
addAutoDisposeListener(
serviceManager.isolateManager.mainIsolate,
listenToDeviceState,
);
}

/// Overrides the brightness of the app with [value], or stops overriding it
/// if [value] is [BrightnessOverride.system].
Future<void> setBrightness(BrightnessOverride value) {
return serviceConnection.serviceManager.serviceExtensionManager
.setServiceExtensionState(
extensions.brightnessMode.extension,
enabled: value != BrightnessOverride.system,
value: value.value,
);
}

void _onTextScaleChanged() {
// TODO(hannah-hyj): Implement VM service extension call for text scale override.
}
Expand All @@ -144,7 +157,10 @@ class AccessibilityController extends DevToolsScreenController
final screenId = ScreenMetaData.accessibility.id;

// --- Accessibility Overrides State ---
final brightness = ValueNotifier<BrightnessOverride>(

/// The brightness that DevTools last set, or that the app last reported.
ValueListenable<BrightnessOverride> get brightness => _brightness;
final _brightness = ValueNotifier<BrightnessOverride>(
BrightnessOverride.system,
);
final textScale = ValueNotifier<double>(1.0);
Expand Down Expand Up @@ -321,7 +337,7 @@ class AccessibilityController extends DevToolsScreenController
@override
void dispose() {
unawaited(_disposeSemanticsOnApp());
brightness.dispose();
_brightness.dispose();
textScale.dispose();
boldText.dispose();
screenReader.dispose();
Expand Down
4 changes: 3 additions & 1 deletion packages/devtools_app/lib/src/screens/accessibility/overrides_pane.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file or at https://developers.google.com/open-source/licenses/bsd.

import 'dart:async';

import 'package:devtools_app_shared/ui.dart';
import 'package:material_ui/material_ui.dart';

Expand Down Expand Up @@ -117,7 +119,7 @@ class _BrightnessOverride extends StatelessWidget {
}).toList(),
onChanged: (newValue) {
if (newValue != null) {
controller.brightness.value = newValue;
unawaited(controller.setBrightness(newValue));
}
},
);
Expand Down
84 changes: 76 additions & 8 deletions packages/devtools_app/test/screens/accessibility/accessibility_controller_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,41 @@ void main() {
},
);

test(
'does not send the brightness that the app reports back to the app',
() async {
final extensionManager =
serviceConnection.serviceManager.serviceExtensionManager
as FakeServiceExtensionManager;
final brightnessExtension = brightnessMode.extension;
extensionManager.extensionValueOnDevice[brightnessExtension] =
'Brightness.dark';
await extensionManager.fakeAddServiceExtension(brightnessExtension);
await extensionManager.fakeFrame();
// Forget the brightness on the device, so that a call to set it shows.
extensionManager.extensionValueOnDevice.remove(brightnessExtension);

// The page opens after DevTools read the brightness of the app.
final pageController = AccessibilityController()..init();
addTearDown(pageController.dispose);
expect(pageController.brightness.value, BrightnessOverride.dark);
// Let any call that the page makes in response finish.
await pumpEventQueue();

extensionManager.fakeServiceExtensionStateChanged(
brightnessExtension,
'Brightness.light',
);
expect(pageController.brightness.value, BrightnessOverride.light);

await pumpEventQueue();
expect(
extensionManager.extensionValueOnDevice,
isNot(contains(brightnessExtension)),
);
},
);

test(
'setting controller brightness updates service extension state',
() async {
Expand All @@ -111,20 +146,17 @@ void main() {
expect(controller.brightness.value, BrightnessOverride.system);

// Set to dark mode
controller.brightness.value = BrightnessOverride.dark;

// Wait for async operations to complete
await Future<void>.delayed(Duration.zero);
await controller.setBrightness(BrightnessOverride.dark);

final darkState = fakeServiceExtensionManager
.getServiceExtensionState(brightnessMode.extension)
.value;
expect(darkState.value, equals('Brightness.dark'));
expect(darkState.enabled, isTrue);
expect(controller.brightness.value, BrightnessOverride.dark);

// Set to light mode
controller.brightness.value = BrightnessOverride.light;
await Future<void>.delayed(Duration.zero);
await controller.setBrightness(BrightnessOverride.light);

final lightState = fakeServiceExtensionManager
.getServiceExtensionState(brightnessMode.extension)
Expand All @@ -133,8 +165,7 @@ void main() {
expect(lightState.enabled, isTrue);

// Set to system
controller.brightness.value = BrightnessOverride.system;
await Future<void>.delayed(Duration.zero);
await controller.setBrightness(BrightnessOverride.system);

final systemState = fakeServiceExtensionManager
.getServiceExtensionState(brightnessMode.extension)
Expand All @@ -144,6 +175,43 @@ void main() {
},
);

test('restores the brightness override after a hot restart', () async {
final extensionManager =
serviceConnection.serviceManager.serviceExtensionManager
as FakeServiceExtensionManager;
final isolateManager =
serviceConnection.serviceManager.isolateManager as FakeIsolateManager;
final brightnessExtension = brightnessMode.extension;
await extensionManager.fakeAddServiceExtension(brightnessExtension);
await extensionManager.fakeFrame();
await controller.setBrightness(BrightnessOverride.dark);
// Let any call that the page makes in response finish.
await pumpEventQueue();

// A hot restart replaces the main isolate, and the new one starts
// without the override.
extensionManager.fakeMainIsolateClosed();
isolateManager.setMainIsolate(null);
extensionManager.extensionValueOnDevice.remove(brightnessExtension);
isolateManager.setMainIsolate(
IsolateRef.parse({'id': 'new_main_isolate_id'}),
);
await extensionManager.fakeAddServiceExtension(brightnessExtension);
await extensionManager.fakeFrame();
expect(
extensionManager.extensionValueOnDevice[brightnessExtension],
'Brightness.dark',
);
expect(controller.brightness.value, BrightnessOverride.dark);

// The page follows the brightness of the new main isolate.
extensionManager.fakeServiceExtensionStateChanged(
brightnessExtension,
'Brightness.light',
);
expect(controller.brightness.value, BrightnessOverride.light);
});

test(
'SemanticsNodeModel properties, parseRect, rectDisplay, and shallowCopy',
() {
Expand Down
6 changes: 6 additions & 0 deletions packages/devtools_test/lib/src/mocks/fake_isolate_manager.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,12 @@ base class FakeIsolateManager extends Fake with TestIsolateManager {
_isolateCreatedController.add(isolate);
}

/// Changes the main isolate, as [IsolateManager] does during a hot restart.
@visibleForTesting
void setMainIsolate(IsolateRef? isolate) {
_mainIsolate.value = isolate;
}

@override
ValueNotifier<List<IsolateRef>> get isolates {
return _isolates ??= ValueNotifier([?_selectedIsolate.value]);
Expand Down
15 changes: 15 additions & 0 deletions packages/devtools_test/lib/src/mocks/fake_service_extension_manager.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,21 @@ base class FakeServiceExtensionManager extends Fake
}
}

/// Hook for tests to simulate the main isolate exiting, as it does during a
/// hot restart.
///
/// Like [ServiceExtensionManager], this resets the listenables that
/// [hasServiceExtension] and [getServiceExtensionState] returned, and then
/// stops updating them.
void fakeMainIsolateClosed() {
vmServiceClosed();
_serviceExtensionAvailable.clear();
for (final state in _serviceExtensionStateController.values) {
state.value = ServiceExtensionState(enabled: false, value: null);
}
_serviceExtensionStateController.clear();
}

/// Sets the state for a service extension and makes the call to the VMService.
@override
Future<void> setServiceExtensionState(
Expand Down
Loading