From 13eea5c27c57d69e3f0ff14923b8be99da4fea6e Mon Sep 17 00:00:00 2001 From: Victor Sanni Date: Mon, 2 Nov 2026 15:21:02 -0800 Subject: [PATCH] [A11y] Stop sending the brightness that the app reports back to the app The brightness dropdown wrote every change of its state back to the app, including the brightness that the app reported and the reset that ServiceExtensionManager makes when the main isolate exits. The dropdown now calls AccessibilityController.setBrightness, and the controller looks up the brightness state again whenever the main isolate changes. --- .../accessibility_controller.dart | 58 ++++++++----- .../screens/accessibility/overrides_pane.dart | 4 +- .../accessibility_controller_test.dart | 84 +++++++++++++++++-- .../lib/src/mocks/fake_isolate_manager.dart | 6 ++ .../mocks/fake_service_extension_manager.dart | 15 ++++ 5 files changed, 137 insertions(+), 30 deletions(-) diff --git a/packages/devtools_app/lib/src/screens/accessibility/accessibility_controller.dart b/packages/devtools_app/lib/src/screens/accessibility/accessibility_controller.dart index 679bdccd399..f35ab0e690e 100644 --- a/packages/devtools_app/lib/src/screens/accessibility/accessibility_controller.dart +++ b/packages/devtools_app/lib/src/screens/accessibility/accessibility_controller.dart @@ -50,7 +50,6 @@ class AccessibilityController extends DevToolsScreenController } void _initListeners() { - addAutoDisposeListener(brightness, _onBrightnessChanged); addAutoDisposeListener(textScale, _onTextScaleChanged); addAutoDisposeListener(boldText, _onBoldTextChanged); addAutoDisposeListener(screenReader, _onScreenReaderChanged); @@ -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 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 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. } @@ -144,7 +157,10 @@ class AccessibilityController extends DevToolsScreenController final screenId = ScreenMetaData.accessibility.id; // --- Accessibility Overrides State --- - final brightness = ValueNotifier( + + /// The brightness that DevTools last set, or that the app last reported. + ValueListenable get brightness => _brightness; + final _brightness = ValueNotifier( BrightnessOverride.system, ); final textScale = ValueNotifier(1.0); @@ -321,7 +337,7 @@ class AccessibilityController extends DevToolsScreenController @override void dispose() { unawaited(_disposeSemanticsOnApp()); - brightness.dispose(); + _brightness.dispose(); textScale.dispose(); boldText.dispose(); screenReader.dispose(); diff --git a/packages/devtools_app/lib/src/screens/accessibility/overrides_pane.dart b/packages/devtools_app/lib/src/screens/accessibility/overrides_pane.dart index 9e2555ac122..b16c5332155 100644 --- a/packages/devtools_app/lib/src/screens/accessibility/overrides_pane.dart +++ b/packages/devtools_app/lib/src/screens/accessibility/overrides_pane.dart @@ -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'; @@ -117,7 +119,7 @@ class _BrightnessOverride extends StatelessWidget { }).toList(), onChanged: (newValue) { if (newValue != null) { - controller.brightness.value = newValue; + unawaited(controller.setBrightness(newValue)); } }, ); diff --git a/packages/devtools_app/test/screens/accessibility/accessibility_controller_test.dart b/packages/devtools_app/test/screens/accessibility/accessibility_controller_test.dart index 2575cb09fd3..2848fab5a4b 100644 --- a/packages/devtools_app/test/screens/accessibility/accessibility_controller_test.dart +++ b/packages/devtools_app/test/screens/accessibility/accessibility_controller_test.dart @@ -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 { @@ -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.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.delayed(Duration.zero); + await controller.setBrightness(BrightnessOverride.light); final lightState = fakeServiceExtensionManager .getServiceExtensionState(brightnessMode.extension) @@ -133,8 +165,7 @@ void main() { expect(lightState.enabled, isTrue); // Set to system - controller.brightness.value = BrightnessOverride.system; - await Future.delayed(Duration.zero); + await controller.setBrightness(BrightnessOverride.system); final systemState = fakeServiceExtensionManager .getServiceExtensionState(brightnessMode.extension) @@ -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', () { diff --git a/packages/devtools_test/lib/src/mocks/fake_isolate_manager.dart b/packages/devtools_test/lib/src/mocks/fake_isolate_manager.dart index 5e9823fdd07..09c5688a78a 100644 --- a/packages/devtools_test/lib/src/mocks/fake_isolate_manager.dart +++ b/packages/devtools_test/lib/src/mocks/fake_isolate_manager.dart @@ -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> get isolates { return _isolates ??= ValueNotifier([?_selectedIsolate.value]); diff --git a/packages/devtools_test/lib/src/mocks/fake_service_extension_manager.dart b/packages/devtools_test/lib/src/mocks/fake_service_extension_manager.dart index fc47c3adacd..7511d697519 100644 --- a/packages/devtools_test/lib/src/mocks/fake_service_extension_manager.dart +++ b/packages/devtools_test/lib/src/mocks/fake_service_extension_manager.dart @@ -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 setServiceExtensionState(