Implements accessibility bridge in common library

[chunhtai]

Description

design doc
go/accessibility-bridge-detailed-design

Related Issues

flutter/flutter#23601

Tests

I added the following tests:

see files

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

• I read the contributor guide and followed the process outlined there for submitting PRs.
• I signed the CLA.
• I read and followed the C++, Objective-C, Java style guides for the engine.
• I read the tree hygiene wiki page, which explains my responsibilities.
• I updated/added relevant documentation.
• All existing and new tests are passing.
• I am willing to follow-up on review comments in a timely manner.

Reviewer Checklist

• I have submitted any presubmit flakes in this PR using the engine presubmit flakes form before re-triggering the failure.

Breaking Change

Did any tests fail when you ran them? Please read handling breaking changes.

• No, no existing tests failed, so this is not a breaking change.
• Yes, this is a breaking change. If not, delete the remainder of this section.
  • I wrote a design doc: https://flutter.dev/go/template Replace this with a link to your design doc's short link
  • I got input from the developer relations team, specifically from: Replace with the names of who gave advice
  • I wrote a migration guide: https://flutter.dev/go/breaking-changes-template Replace this with a link to a pull request that adds the migration guide to https://flutter.dev/docs/release/breaking-changes

[dnfield]

I've mainly looked at the header files so far, not the implementation so much.

It might help to have a design doc for this laying out what the class heirarchy/relationships will be.

In general:

• Please avoid using raw pointers in C++ code unless absolutely necessary.
• Please clarify the relationship between FlutterAccessibility and the AccessibilityBridge class, and who owns what.

[dnfield]

nit:

Suggested change

	constexpr int khasScrollingAction =
	constexpr int kHasScrollingAction =

(and elsewhere)

[dnfield]

We also might want to define this in the header that defines FlutterSemanticsAction, or perhaps as a method in there with a comment to update it if we add anything to the semantics action enumeration.

[chunhtai]

It will be in the embedder.h, I think we should not add an API there if this is only matter for this use case?

[dnfield]

We could export it from embedder.h and just define it from kScrollableSemanticsActions in semantics_node right?

[chunhtai]

embedder.h redefined FlutterSemanticsActions. I will add a constant in the embedder.h

[dnfield]

nit: formatting here and below. Looks like clang-format might have added line breaks wher eyou don't want them.

not a nit: these docs need to be improved. If this really is the one coming from Flutter to platform, then we should give some examples of what kind of events might get sent and handled here. We should talk about what a targetted event is - e.g. what can be a target, what kind of events there are.

Also, please avoid using raw pointers - I'm assuming this probably wants to be a std::shared_ptr<AccessibilityBridge>, but a weak or unique is fine too if that's what's appropriate.

[dnfield]

Please document some examples, and how these are different from events.

[dnfield]

Why do we need this linkage?

We should come up with a working and as-simple-as-possible ownership model for this.

More specifically: we need to avoid a model where nodes have pointers to other nodes, and to bridges, and bridges also have pointers back to the nodes, and no one knows who owns what or how to properly free memory anymore.

[chunhtai]

as we discuss offline, I will keep this as is

[dnfield]

This seems like we're leaking an abstraction.

Do we want to make this implement AXNode instead?

Do we need to publicly expose this?

And again, we should avoid raw pointer usage.

[chunhtai]

as we discuss offline, I will keep this as is

[dnfield]

Avoid raw pointers.

I'm also still questioning why we'd need a pointer from this object to an a11y bridge, but I might be misunderstanding what this object does.

[chunhtai]

this object wraps AXNode and uses it to implement AXPlatformNodeDelegate API, it needs reference so that it can have access to the parent and its sibling which is required to implement the AXPlatformNodeDelegate API. Alternatively, we can store all these information in this node, but we will have to keep updating them when the structure of AXTree changes.

[dnfield]

Sorry, I see you linked a design doc here, I'll take a look there as well :)

[chunhtai]

Is it possible to export this const int in embedder.h if it uses enum in the same file?
I tried

FLUTTER_EXPORT const int32_t kHasScrollingAction =
    kFlutterSemanticsActionScrollLeft |
    kFlutterSemanticsActionScrollRight |
    kFlutterSemanticsActionScrollUp |
    kFlutterSemanticsActionScrollDown;

but it kept complaining about

duplicate symbol '_kHasScrollingAction' in:
    obj/flutter/shell/platform/embedder/embedder.embedder_engine.o
    obj/flutter/shell/platform/embedder/embedder.platform_view_embedder.o

Do i miss something?

[dnfield]

It can only be defined in one place.

[chunhtai]

yes, I remove the definition in this file and put it in embedder.h only.

[chunhtai]

I keep it as is, unless we want to create a separate memory copy in the embedding, we cannot use smart pointer here

[chunhtai]

the custom action here is the available custom action on a semantics node because the embedding sends the custom action information of a node separately from the node update. Not really sure how I should put in the doc here...

[chunhtai]

There are two reason i did this.

1. the embedding sends update to a semantics node one by one, and the memory of the semantics node is destroy right before it sends the next one. That means I have to create a memory copy of the the FlutterSemanticsNode, which means i have to write a function to assign each attribute one by one anyway.
2. continuing on 1, the FlutterSemanticsNode uses raw pointer to hold data array, which is make it a bit risky to manage the memory allocation. If I create a different struct to use std vector, it will be easier to manage the memory allocation.

[chunhtai]

I kept AccessibilityBridge as raw pointer, I have tried to use shared_ptr/weak_ptr but that forces the owner that create AccessibilityBridge to store it as shared_ptr. I found that quite confusing.

for example, the make embedding can only create AccessibilityBridge using this

std::shared_ptr< AccessibilityBridge> _bridge = std::make_shared<AccessibilityBridge>(delegate);

they cannot do the following

AccessibilityBridge _beidge(delegate);

or

std::unique_ptr< AccessibilityBridge> _bridge = std::make_unique<AccessibilityBridge>(delegate);

[dnfield]

That's actually less confusing because it clarifies up front that the embedding is creating something that it does not exclusively own.

If you want to stack allocate the bridge (and destroy it when the creator is collected), then we should be giving a weak pointer here.

[chunhtai]

@dnfield This is ready for another look

[dnfield]

This shouldn't be a raw pointer.

[chunhtai]

I removed this getter because subclass should figure out their way to access the bridge in the embedding. but it still need to be stored in this class as a raw pointer, I couldn't figure out a way to use smart pointer in this case without forcing the owner of accessiblity bridge to hold it as shared_ptr.

[dnfield]

This shouldn't be a raw pointer.

It also seems like it shouldn't be public. Why would an embedder ever need to access this?

[chunhtai]

the AXNode is owned by the axtree and cannot be a smart pointer here as well. I make it a protected method which should make it less risky

[dnfield]

nit: rename this so the _ is at the end.

[dnfield]

This should have a bug filed.

However, we should be handling this better. Have you tested this code at multiple DPR scales?

Also, we should document who has the responsibility of multiplying byt he DPR - whether it's the bridge or a node delegate.

[chunhtai]

a friendly bump

[dnfield]

LGTM