Skip to content

Move semantic-related bindings to SemanticsBinding #121289

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

Merged
merged 5 commits into from
Feb 24, 2023
Merged

Move semantic-related bindings to SemanticsBinding #121289

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
62f0804
Move semantic-related bindings to SemanticsBinding
goderbauer Feb 22, 2023
4903181
typo
goderbauer Feb 22, 2023
05c8565
fixes
goderbauer Feb 23, 2023
3d3f0e2
review
goderbauer Feb 23, 2023
6eb6786
review2
goderbauer Feb 23, 2023
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
4 changes: 2 additions & 2 deletions dev/integration_tests/flutter_gallery/test/smoke_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,8 +184,8 @@ void main() {
);

testWidgets('Flutter Gallery app smoke test with semantics', (WidgetTester tester) async {
RendererBinding.instance.setSemanticsEnabled(true);
final SemanticsHandle handle = SemanticsBinding.instance.ensureSemantics();
await smokeGallery(tester);
RendererBinding.instance.setSemanticsEnabled(false);
handle.dispose();
});
}
63 changes: 54 additions & 9 deletions dev/integration_tests/ios_add2app_life_cycle/flutterapp/lib/main.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,19 +3,16 @@
// found in the LICENSE file.

import 'package:collection/collection.dart';
import 'package:flutter/foundation.dart';
import 'package:flutter/material.dart';
import 'package:flutter/rendering.dart';
import 'package:flutter/services.dart';

VoidCallback? originalSemanticsListener;

void main() {
WidgetsFlutterBinding.ensureInitialized();
// Disconnects semantics listener for testing purposes.
originalSemanticsListener = WidgetsBinding.instance.platformDispatcher.onSemanticsEnabledChanged;
RendererBinding.instance.platformDispatcher.onSemanticsEnabledChanged = null;
RendererBinding.instance.setSemanticsEnabled(false);
// If the test passes, LifeCycleSpy will rewire the semantics listener back.
SwitchableSemanticsBinding.ensureInitialized();
assert(!SwitchableSemanticsBinding.instance.semanticsEnabled);

runApp(const LifeCycleSpy());
}

Expand Down Expand Up @@ -68,12 +65,60 @@ class _LifeCycleSpyState extends State<LifeCycleSpy> with WidgetsBindingObserver
Widget build(BuildContext context) {
if (const ListEquality<AppLifecycleState?>().equals(_actualLifeCycleSequence, _expectedLifeCycleSequence)) {
// Rewires the semantics harness if test passes.
RendererBinding.instance.setSemanticsEnabled(true);
RendererBinding.instance.platformDispatcher.onSemanticsEnabledChanged = originalSemanticsListener;
SwitchableSemanticsBinding.instance.semanticsEnabled = true;
}
return const MaterialApp(
title: 'Flutter View',
home: Text('test'),
);
}
}

class SwitchableSemanticsBinding extends WidgetsFlutterBinding {
static SwitchableSemanticsBinding get instance => BindingBase.checkInstance(_instance);
static SwitchableSemanticsBinding? _instance;

static SwitchableSemanticsBinding ensureInitialized() {
if (_instance == null) {
SwitchableSemanticsBinding();
}
return SwitchableSemanticsBinding.instance;
}

VoidCallback? _originalSemanticsListener;

@override
void initInstances() {
super.initInstances();
_instance = this;
_updateHandler();
}

@override
bool get semanticsEnabled => _semanticsEnabled.value;
final ValueNotifier<bool> _semanticsEnabled = ValueNotifier<bool>(false);
set semanticsEnabled(bool value) {
_semanticsEnabled.value = value;
_updateHandler();
}

void _updateHandler() {
if (_semanticsEnabled.value) {
platformDispatcher.onSemanticsEnabledChanged = _originalSemanticsListener;
_originalSemanticsListener = null;
} else {
_originalSemanticsListener = platformDispatcher.onSemanticsEnabledChanged;
platformDispatcher.onSemanticsEnabledChanged = null;
}
}

@override
void addSemanticsEnabledListener(VoidCallback listener) {
_semanticsEnabled.addListener(listener);
}

@override
void removeSemanticsEnabledListener(VoidCallback listener) {
_semanticsEnabled.removeListener(listener);
}
}
40 changes: 15 additions & 25 deletions packages/flutter/lib/src/rendering/binding.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,16 +38,15 @@ mixin RendererBinding on BindingBase, ServicesBinding, SchedulerBinding, Gesture
platformDispatcher
..onMetricsChanged = handleMetricsChanged
..onTextScaleFactorChanged = handleTextScaleFactorChanged
..onPlatformBrightnessChanged = handlePlatformBrightnessChanged
..onSemanticsEnabledChanged = _handleSemanticsEnabledChanged
..onSemanticsAction = _handleSemanticsAction;
..onPlatformBrightnessChanged = handlePlatformBrightnessChanged;
initRenderView();
_handleSemanticsEnabledChanged();
addPersistentFrameCallback(_handlePersistentFrameCallback);
initMouseTracker();
if (kIsWeb) {
addPostFrameCallback(_handleWebFirstFrame);
}
addSemanticsEnabledListener(_handleSemanticsEnabledChanged);
_handleSemanticsEnabledChanged();
}

/// The current [RendererBinding], if one has been created.
Expand Down Expand Up @@ -308,8 +307,6 @@ mixin RendererBinding on BindingBase, ServicesBinding, SchedulerBinding, Gesture
);
}

SemanticsHandle? _semanticsHandle;

/// Creates a [MouseTracker] which manages state about currently connected
/// mice, for hover notification.
///
Expand All @@ -333,33 +330,20 @@ mixin RendererBinding on BindingBase, ServicesBinding, SchedulerBinding, Gesture
super.dispatchEvent(event, hitTestResult);
}

void _handleSemanticsEnabledChanged() {
setSemanticsEnabled(platformDispatcher.semanticsEnabled);
}
SemanticsHandle? _semanticsHandle;

/// Whether the render tree associated with this binding should produce a tree
/// of [SemanticsNode] objects.
void setSemanticsEnabled(bool enabled) {
if (enabled) {
void _handleSemanticsEnabledChanged() {
if (semanticsEnabled) {
_semanticsHandle ??= _pipelineOwner.ensureSemantics();
} else {
_semanticsHandle?.dispose();
_semanticsHandle = null;
}
}

void _handleWebFirstFrame(Duration _) {
assert(kIsWeb);
const MethodChannel methodChannel = MethodChannel('flutter/service_worker');
methodChannel.invokeMethod<void>('first-frame');
}

void _handleSemanticsAction(int id, SemanticsAction action, ByteData? args) {
_pipelineOwner.semanticsOwner?.performAction(
id,
action,
args != null ? const StandardMessageCodec().decodeMessage(args) : null,
);
@override
void performSemanticsAction(SemanticsActionEvent action) {
_pipelineOwner.semanticsOwner?.performAction(action.nodeId, action.type, action.arguments);
}

void _handleSemanticsOwnerCreated() {
Expand All @@ -374,6 +358,12 @@ mixin RendererBinding on BindingBase, ServicesBinding, SchedulerBinding, Gesture
renderView.clearSemantics();
}

void _handleWebFirstFrame(Duration _) {
assert(kIsWeb);
const MethodChannel methodChannel = MethodChannel('flutter/service_worker');
methodChannel.invokeMethod<void>('first-frame');
}

void _handlePersistentFrameCallback(Duration timeStamp) {
drawFrame();
_scheduleMouseTrackerUpdate();
Expand Down
37 changes: 10 additions & 27 deletions packages/flutter/lib/src/rendering/object.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -808,24 +808,8 @@ typedef RenderObjectVisitor = void Function(RenderObject child);
/// Used by [RenderObject.invokeLayoutCallback].
typedef LayoutCallback<T extends Constraints> = void Function(T constraints);

/// A reference to the semantics tree.
///
/// The framework maintains the semantics tree (used for accessibility and
/// indexing) only when there is at least one client holding an open
/// [SemanticsHandle].
///
/// The framework notifies the client that it has updated the semantics tree by
/// calling the [listener] callback. When the client no longer needs the
/// semantics tree, the client can call [dispose] on the [SemanticsHandle],
/// which stops these callbacks and closes the [SemanticsHandle]. When all the
/// outstanding [SemanticsHandle] objects are closed, the framework stops
/// updating the semantics tree.
///
/// To obtain a [SemanticsHandle], call [PipelineOwner.ensureSemantics] on the
/// [PipelineOwner] for the render tree from which you wish to read semantics.
/// You can obtain the [PipelineOwner] using the [RenderObject.owner] property.
class SemanticsHandle {
SemanticsHandle._(PipelineOwner owner, this.listener)
class _LocalSemanticsHandle implements SemanticsHandle {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is weird that we reuse the same interface for handles from two different object. Do you know why we need to add an ensureSemantics to the SemanticsBinding?
or PipelineOwner can do the logic in the construction and put dispose logic to one callback and reuse the SemanticsHandle class directly?

Copy link
Member Author

@goderbauer goderbauer Feb 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know why we need to add an ensureSemantics to the SemanticsBinding?

See answer to previous question.

or PipelineOwner can do the logic in the construction and put dispose logic to one callback and reuse the SemanticsHandle class directly?

Can you elaborate what you mean by this?

This is mostly here for backwards compatibility. PipelineOwner had an ensureSemantics method that is getting some good use, I didn't want to remove it just yet. But like I said in the other comment, we also need something that manages the state of semantics on/off globally outside of individual PipelineOwners.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is weird that we reuse the same interface for handles from two different object.

The objects have the same purpose, though. They are both handles to denote your interest in semantics. One on a global level, the other on a per-PipelineOwner level.

Copy link
Contributor

@chunhtai chunhtai Feb 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for explanation, yes this make sense.

What i meant to reuse the SemanticsHandle class is to do something like this

  SemanticsHandle ensureSemantics({ VoidCallback? listener }) {
    _outstandingSemanticsHandles += 1;
    if (_outstandingSemanticsHandles == 1) {
      assert(_semanticsOwner == null);
      assert(onSemanticsUpdate != null, 'Attempted to open a semantics handle without an onSemanticsUpdate callback.');
      _semanticsOwner = SemanticsOwner(onSemanticsUpdate: onSemanticsUpdate!);
      onSemanticsOwnerCreated?.call();
    }
    if (listener != null) {
      _owner.semanticsOwner!.addListener(listener!);
    }
    return SemanticsHandle(() {
    if (listener != null) {
      _owner.semanticsOwner!.removeListenr(listener!);
    }
   _didDisposeSemanticsHandle();
});
  }

but you will have to expose the constructor.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more question though, will we have use case that some views enabled semantics but not others? I don't think there is platform that can turn on accessibility for a certain window only.

If that is the case, maybe we should deprecate the pipelineOwner.ensureSemantics, or redirect the call to SemanticsBinding?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. I don't think I want to make it easy for people to instantiate SemanticsHandle because doing so is more or less meaningless. A handle only has meaning if it was obtained from one of the ensureSemantics methods.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more question though, will we have use case that some views enabled semantics but not others? I don't think there is platform that can turn on accessibility for a certain window only.

I can't think of any, really.

If that is the case, maybe we should deprecate the pipelineOwner.ensureSemantics, or redirect the call to SemanticsBinding?

I am considering this for a future change. The PipelineOwner will get a bigger refactoring soon to make it fit for multi view.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds good to me

_LocalSemanticsHandle._(PipelineOwner owner, this.listener)
: _owner = owner {
if (listener != null) {
_owner.semanticsOwner!.addListener(listener!);
Expand All @@ -837,13 +821,7 @@ class SemanticsHandle {
/// The callback that will be notified when the semantics tree updates.
final VoidCallback? listener;

/// Closes the semantics handle and stops calling [listener] when the
/// semantics updates.
///
/// When all the outstanding [SemanticsHandle] objects for a given
/// [PipelineOwner] are closed, the [PipelineOwner] will stop updating the
/// semantics tree.
@mustCallSuper
@override
void dispose() {
if (listener != null) {
_owner.semanticsOwner!.removeListener(listener!);
Expand Down Expand Up @@ -1171,7 +1149,12 @@ class PipelineOwner {
int _outstandingSemanticsHandles = 0;

/// Opens a [SemanticsHandle] and calls [listener] whenever the semantics tree
/// updates.
/// generated from the render tree owned by this [PipelineOwner] updates.
///
/// Calling this method only ensures that this particular [PipelineOwner] will
/// generate a semantics tree. Consider calling
/// [SemanticsBinding.ensureSemantics] instead to turn on semantics globally
/// for the entire app.
///
/// The [PipelineOwner] updates the semantics tree only when there are clients
/// that wish to use the semantics tree. These clients express their interest
Expand All @@ -1190,7 +1173,7 @@ class PipelineOwner {
_semanticsOwner = SemanticsOwner(onSemanticsUpdate: onSemanticsUpdate!);
onSemanticsOwnerCreated?.call();
}
return SemanticsHandle._(this, listener);
return _LocalSemanticsHandle._(this, listener);
}

void _didDisposeSemanticsHandle() {
Expand Down
Loading