Skip to content
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.

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

Move semantic-related bindings to SemanticsBinding #121289

Merged
merged 5 commits into from Feb 24, 2023

Conversation

goderbauer
Copy link
Member

@goderbauer goderbauer commented Feb 22, 2023

Part of #121573.

Moves all the semantics related stuff out of the other bindings into SemanticsBinding where it belongs (see TODO fixed by this change). We added support for generating semantics information before we saw the need for a dedicated SemanticsBinding. That's why semantics related stuff was found in all the other bindings. This change cleans that up.

Furthermore, it also makes the SemanticsBinding the source of truth of whether semantics information should be generated for the entire app. For this, it introduces a new semanticsEnabled getter (with the option to register listeners for when that value changes) and a new ensureSemantics method, which clients can use to turn on semantics globally. Previously, this was all handled by the PipelineOwner. However, in the multi view world we will have multiple PipelineOwners (one to power each view), so a single source of truth to synchronize the semantics of all these PipelineOwners is needed.

@flutter-dashboard flutter-dashboard bot added a: accessibility Accessibility, e.g. VoiceOver or TalkBack. (aka a11y) a: tests "flutter test", flutter_test, or one of our tests framework flutter/packages/flutter repository. See also f: labels. labels Feb 22, 2023
));
}

/// Called whenever the platform requests an action to be performed on a
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Called whenever the platform requests an action to be performed on a
/// Called whenever the platform requests an action to be performed on a

}

void _handleSemanticsAction(int id, ui.SemanticsAction action, ByteData? args) {
performSemanticsAction(SemanticsActionEvent(
Copy link
Contributor

Choose a reason for hiding this comment

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

This indirection doesn't seem to add too much value.. maybe just leave _handleSemanticsAction public and let RenderingBinding to handle raw args directly?

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 do believe it is the responsibility of the semantics layer to decode args as it is the one that picks the messageCodec. If we were to decide that we need to change the message codec used, we should do the change in one place here instead of fixing up client code elsewhere.

Furthermore, the SemanticsActionEvent gives us a little more flexibility in the future to extend the information associated with a SemanticsEvent without breaking the method signature. For example, we may want to add the view ID from where the event was originating in the future.

/// Called when the platform accessibility features change.
///
/// See [dart:ui.PlatformDispatcher.onAccessibilityFeaturesChanged].
@protected
void handleAccessibilityFeaturesChanged() {
_accessibilityFeatures = platformDispatcher.accessibilityFeatures;
// Nothing to do by default.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just mark this mustCallSuper?

Copy link
Member Author

Choose a reason for hiding this comment

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

My thinking was that this setup guarantees that accessibilityFeatures is called before handleAccessibilityFeaturesChanged is invoked on subclasses. With mustCallSuper you could accidentally call super at the end of your method and operate on outdated data.

Copy link
Member Author

Choose a reason for hiding this comment

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

But we don't do it this way for other handlers on the bindings, so I will change this to your suggestion as well.

/// Clients can close their [SemanticsHandle] by calling
/// [SemanticsHandle.dispose]. Once all outstanding [SemanticsHandle] objects
/// are closed, semantics information are no longer collected.
SemanticsHandle ensureSemantics() {
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the use case to expose this API?

Copy link
Member Author

Choose a reason for hiding this comment

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

Rephrasing from the PR description:

Clients can use ensureSemantics to turn on semantics globally for the entire app. Previously, this was all handled internally by the PipelineOwner. However, in the multi view world we will have multiple PipelineOwners (one to power each view), so a single source of truth to turn on semantics for of all these PipelineOwners across the entire app is needed.

/// 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

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

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

Copy link
Contributor

@chunhtai chunhtai left a comment

Choose a reason for hiding this comment

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

LGTM

/// 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.

sounds good to me

@goderbauer goderbauer added the autosubmit Merge PR when tree becomes green via auto submit App label Feb 24, 2023
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Feb 24, 2023
@auto-submit
Copy link
Contributor

auto-submit bot commented Feb 24, 2023

auto label is removed for flutter/flutter, pr: 121289, due to - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label.

@goderbauer goderbauer added the autosubmit Merge PR when tree becomes green via auto submit App label Feb 24, 2023
@auto-submit auto-submit bot merged commit b1b7284 into flutter:master Feb 24, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 24, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 25, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 25, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 26, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 26, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 26, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 27, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 27, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 27, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 27, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 27, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 27, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 27, 2023
@flutter-dashboard flutter-dashboard bot added the team Infra upgrades, team productivity, code health, technical debt. See also team: labels. label Feb 28, 2023
@goderbauer goderbauer deleted the semanticsMultiVIew branch March 3, 2023 18:20
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 10, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a: accessibility Accessibility, e.g. VoiceOver or TalkBack. (aka a11y) a: tests "flutter test", flutter_test, or one of our tests autosubmit Merge PR when tree becomes green via auto submit App framework flutter/packages/flutter repository. See also f: labels. team Infra upgrades, team productivity, code health, technical debt. See also team: labels.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants