[Embedder API] Add semantic string attributes #44616

loic-sharma · 2023-08-11T00:39:48Z

Flutter's SemanticNodes use StringAttributes to provide additional information on text values for assistive technologies. This exposes the string attributes on the embedder API so that embedders can apply string attributes to their semantics trees.

Addresses flutter/flutter#119970
Part of flutter/flutter#98948

Previous pull request: #44553

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

This refactors how engine semantic updates are mapped to embedder semantic updates. There are no behavioral changes. Part of flutter/flutter#119970, flutter/flutter#98948 Next PR: #44616 ## Background For flutter/flutter#119970, we will need to update the embedder API to add string attributes to semantic nodes' text values. There are multiple kinds of string attributes, and each text value can have multiple string attributes. This requires gnarly mapping code that's best kept out of `embedder.cc`. [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

loic-sharma · 2023-08-11T16:30:26Z

shell/platform/embedder/embedder.h

+// Indicates the assistive technology should announce out the string character
+// by character.
+//
+// See dart:ui's SpellOutStringAttribute.


Mirrors:

engine/lib/ui/semantics.dart

Lines 631 to 639 in 6fb9a09

/// A string attribute that causes the assistive technologies, e.g. VoiceOver,

/// to spell out the string character by character.

///

/// See also:

///

/// * [AttributedString], where the string attributes are used.

/// * [LocaleStringAttribute], which causes the assistive technologies to

/// treat the string in the specific language.

base class SpellOutStringAttribute extends StringAttribute {

loic-sharma · 2023-08-11T16:30:47Z

shell/platform/embedder/embedder.h

+// Indicates the assistive technology should announce the string using the
+// specified locale.
+//
+// See dart:ui's LocaleStringAttribute.


Mirrors:

engine/lib/ui/semantics.dart

Lines 662 to 670 in 6fb9a09

/// A string attribute that causes the assistive technologies, e.g. VoiceOver,

/// to treat string as a certain language.

///

/// See also:

///

/// * [AttributedString], where the string attributes are used.

/// * [SpellOutStringAttribute], which causes the assistive technologies to

/// spell out the string character by character when announcing the string.

base class LocaleStringAttribute extends StringAttribute {

shell/platform/embedder/embedder.h

chunhtai

LGTM

yaakovschectman

Looks good now that the first PR is in

shell/platform/embedder/embedder.h

chinmaygarde · 2023-08-14T23:07:56Z

shell/platform/embedder/embedder_semantics_update.h

@@ -58,13 +58,23 @@ class EmbedderSemanticsUpdate2 {
  std::vector<FlutterSemanticsCustomAction2> actions_;
  std::vector<FlutterSemanticsCustomAction2*> action_pointers_;

+  std::vector<std::unique_ptr<std::vector<FlutterStringAttribute*>>>


I am no longer super familiar with this bit of the codebase. But can you conform that the underlying nodes array is never modified after the vector of pointers is constructed? This will invalidate the old pointers.

But can you conform that the underlying nodes array is never modified after the vector of pointers is constructed?

Yup, confirmed. Pointers to T are captured only if:

T is owned by a std::unique_ptr<T>, or...

T is owned by a std::vector<T> that is fully constructed, or...

T is owned by a std::vector<std::unique_ptr<T>>. The std::unique_ptr makes the pointer to T stable even if the vector's data is reallocated.

I mirrored the approach taken for the array of layers:

engine/shell/platform/embedder/embedder_layers.h

Lines 40 to 45 in f1f912c

std::vector<std::unique_ptr<FlutterPlatformView>> platform_views_referenced_;

std::vector<std::unique_ptr<FlutterPlatformViewMutation>>

mutations_referenced_;

std::vector<std::unique_ptr<std::vector<const FlutterPlatformViewMutation*>>>

mutations_arrays_referenced_;

std::vector<FlutterLayer> presented_layers_;

cbracken

Not done reviewing yet -- still thinking about the CreateStringAttributes method.

shell/platform/embedder/embedder.h

shell/platform/embedder/embedder_semantics_update.cc

cbracken

lgtm - just a few comments/suggestions.

cbracken · 2023-08-17T22:55:38Z

shell/platform/embedder/embedder_semantics_update.h

+      node_string_attributes_;
+  std::vector<std::unique_ptr<FlutterStringAttribute>> string_attributes_;
+  std::vector<std::unique_ptr<FlutterLocaleStringAttribute>> locale_attributes_;
+  std::unique_ptr<FlutterSpellOutStringAttribute> spell_out_attribute_;


I think it's worth adding comments to these for the benefit of future us.

Basically something to the effect that these are buffers for holding temporary embedder-specific objects we create from their internal representation and pass back to the embedder in the semantics update callback in order to manage their lifetimes such that the objects are valid for the duration of the callback, and are automatically cleaned up when the update itself is dealloc'ed after the callback finishes.

Added this comment to both the struct declaration and these field declarations.

cbracken · 2023-08-17T23:15:00Z

shell/platform/embedder/embedder_semantics_update.cc

+    switch (attribute->type) {
+      case StringAttributeType::kLocale: {
+        std::shared_ptr<flutter::LocaleStringAttribute> locale_attribute =
+            std::static_pointer_cast<flutter::LocaleStringAttribute>(attribute);


Alright - looking at string_attribute.h:15-16 and string_attribute.h:49-51, I see why where the shared_ptrs are coming from. I haven't looked into that code do see why they're necessary, but I'll take it on faith that they are :)

cbracken · 2023-08-17T23:19:56Z

shell/platform/embedder/embedder_semantics_update.cc

+        auto embedder_locale = std::make_unique<FlutterLocaleStringAttribute>();
+        embedder_locale->struct_size = sizeof(FlutterLocaleStringAttribute);
+        embedder_locale->locale = locale_attribute->locale.c_str();
+        locale_attributes_.push_back(std::move(embedder_locale));


Can we write this as:

locale_attributes_.emplace_back({fields, in, declaration, order});

That said, I'd have to look it up; struct initializer support without an explicit constructor may only have been added in C++20. I feel like I may have had the same discussion with knopp recently.

Similar below.

Sadly, our compiler doesn't compile this without an explicit constructor on the struct :(

shell/platform/embedder/embedder_semantics_update.h

shell/platform/embedder/embedder_semantics_update.cc

…ne#44616)

…132914) flutter/engine@f4bffdc...2dcfb6c 2023-08-20 [email protected] Roll Skia from e9cf3f1740eb to 4f6b9d08b6d1 (2 revisions) (flutter/engine#44868) 2023-08-20 [email protected] Revert "Implementing TextScaler for nonlinear text scaling" (flutter/engine#44882) 2023-08-18 [email protected] [Embedder API] Add semantic string attributes (flutter/engine#44616) 2023-08-18 [email protected] Implementing TextScaler for nonlinear text scaling (flutter/engine#42062) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

This refactors how engine semantic updates are mapped to embedder semantic updates. There are no behavioral changes. Part of flutter/flutter#119970, flutter/flutter#98948 Next PR: flutter#44616 ## Background For flutter/flutter#119970, we will need to update the embedder API to add string attributes to semantic nodes' text values. There are multiple kinds of string attributes, and each text value can have multiple string attributes. This requires gnarly mapping code that's best kept out of `embedder.cc`. [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

Flutter's `SemanticNode`s use [`StringAttribute`](https://api.flutter.dev/flutter/dart-ui/StringAttribute-class.html)s to provide additional information on text values for assistive technologies. This exposes the string attributes on the embedder API so that embedders can apply string attributes to their semantics trees. Addresses flutter/flutter#119970 Part of flutter/flutter#98948 Previous pull request: flutter#44553 [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

loic-sharma force-pushed the embedder_string_attributes branch from a8229be to 80c9626 Compare August 11, 2023 00:44

loic-sharma changed the title ~~[Embedder] Refactor how semantic updates are mapped~~ [Embedder API] Add semantic string attributes Aug 11, 2023

loic-sharma requested review from chunhtai and yaakovschectman August 11, 2023 00:48

loic-sharma force-pushed the embedder_string_attributes branch 2 times, most recently from f9e176a to 824339e Compare August 11, 2023 00:55

loic-sharma mentioned this pull request Aug 11, 2023

[Embedder] Refactor how semantic updates are mapped #44553

Merged

8 tasks

loic-sharma force-pushed the embedder_string_attributes branch from f67d58b to 82743fe Compare August 11, 2023 16:25

loic-sharma marked this pull request as ready for review August 11, 2023 16:26

loic-sharma requested review from cbracken and chinmaygarde as code owners August 11, 2023 16:26

loic-sharma commented Aug 11, 2023

View reviewed changes

shell/platform/embedder/embedder.h Show resolved Hide resolved

chunhtai approved these changes Aug 11, 2023

View reviewed changes

yaakovschectman approved these changes Aug 11, 2023

View reviewed changes

[Embedder API] Add semantic string attributes

dc9dc1a

loic-sharma force-pushed the embedder_string_attributes branch from f69621a to dc9dc1a Compare August 14, 2023 19:08

chinmaygarde reviewed Aug 14, 2023

View reviewed changes

Use const pointers

9e3f989

cbracken reviewed Aug 17, 2023

View reviewed changes

Add comments

12db213

cbracken approved these changes Aug 17, 2023

View reviewed changes

Feedback

e61bc18

loic-sharma added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 18, 2023

auto-submit bot merged commit 7ba90e6 into flutter:main Aug 18, 2023

loic-sharma deleted the embedder_string_attributes branch August 18, 2023 22:57

loic-sharma mentioned this pull request Aug 18, 2023

Embedding API does not support string attributes. flutter/flutter#119970

Closed

engine-flutter-autoroll mentioned this pull request Aug 20, 2023

Roll Flutter Engine from f4bffdcf8536 to 2dcfb6cb7e49 (4 revisions) flutter/flutter#132914

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 20, 2023

7ba90e695 [Embedder API] Add semantic string attributes (flutter/engi…

9bc36d0

…ne#44616)

leiflinse-trivector mentioned this pull request Sep 6, 2023

Allow setting assistive technology locale via Semantics widget flutter/flutter#124889

Closed

	/// A string attribute that causes the assistive technologies, e.g. VoiceOver,
	/// to spell out the string character by character.
	///
	/// See also:
	///
	/// * [AttributedString], where the string attributes are used.
	/// * [LocaleStringAttribute], which causes the assistive technologies to
	/// treat the string in the specific language.
	base class SpellOutStringAttribute extends StringAttribute {

	/// A string attribute that causes the assistive technologies, e.g. VoiceOver,
	/// to treat string as a certain language.
	///
	/// See also:
	///
	/// * [AttributedString], where the string attributes are used.
	/// * [SpellOutStringAttribute], which causes the assistive technologies to
	/// spell out the string character by character when announcing the string.
	base class LocaleStringAttribute extends StringAttribute {

	std::vector<std::unique_ptr<FlutterPlatformView>> platform_views_referenced_;
	std::vector<std::unique_ptr<FlutterPlatformViewMutation>>
	mutations_referenced_;
	std::vector<std::unique_ptr<std::vector<const FlutterPlatformViewMutation*>>>
	mutations_arrays_referenced_;
	std::vector<FlutterLayer> presented_layers_;

[Embedder API] Add semantic string attributes #44616

[Embedder API] Add semantic string attributes #44616

Uh oh!

Conversation

loic-sharma commented Aug 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

loic-sharma Aug 11, 2023

Choose a reason for hiding this comment

Uh oh!

loic-sharma Aug 11, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chunhtai left a comment

Choose a reason for hiding this comment

Uh oh!

yaakovschectman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chinmaygarde Aug 14, 2023

Choose a reason for hiding this comment

Uh oh!

loic-sharma Aug 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbracken left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cbracken left a comment

Choose a reason for hiding this comment

Uh oh!

cbracken Aug 17, 2023

Choose a reason for hiding this comment

Uh oh!

loic-sharma Aug 18, 2023

Choose a reason for hiding this comment

Uh oh!

cbracken Aug 17, 2023

Choose a reason for hiding this comment

Uh oh!

cbracken Aug 17, 2023

Choose a reason for hiding this comment

Uh oh!

loic-sharma Aug 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

loic-sharma commented Aug 11, 2023 •

edited

Loading

loic-sharma Aug 14, 2023 •

edited

Loading

loic-sharma Aug 18, 2023 •

edited

Loading