Sitelet https://web.archive.org/web/20210603191837/https://github.com/flutter/engine/pull/25373
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

Add API to the engine to support attributed text #25373

Merged
merged 5 commits into from Jun 1, 2021

Conversation

@chunhtai
Copy link
Contributor

@chunhtai chunhtai commented Apr 1, 2021 •

Design doc: flutter.dev/go/a11y-text-attributes

framework pr: flutter/flutter#79599

issue flutter/flutter#79318

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.
  • The reviewer has submitted any presubmit flakes in this PR using the engine presubmit flakes form before re-triggering the failure.

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

@chunhtai
Copy link
Contributor Author

@chunhtai chunhtai commented Apr 6, 2021 •

Hi @cbracken and @goderbauer , Since this change touches a wide range of the code, I would like to know if the overall approach looks good to you before i proceed to write the test and finishing up the rest. This PR only has android part implemented.

The overall approach:

I added several classes in the framework, below shows a simply version.

class StringAttribute {
   TextRange range;
   StringAttributeType type;
   Object? args;
}

enum StringAttributeType {
  spellOut,
}

class AttributedString {
  String string;
  List<StringAttribute> attributes;
}

I added a attributedLabel to the semantics node which is AttributedString. Before it is sent to the engine, the StringAttribute will be converted into a map.

<String, Object>{
  'StringAttribute_textStartKey': 0,
  'StringAttribute_textEndKey': 5,
  'StringAttribute_typeKey': 0, // enum value,
  'StringAttribute_argsKey': someArgs, // optional args
}

That map will be converted to c++ std::unordered_map in tonic, and the unordered_map will be converted back to the c++ version of StringAttribute.

struct StringAttribute {
  int32_t start = -1;
  int32_t end = -1;
  StringAttributeType type;
  std::vector<uint8_t> args;
};

enum class StringAttributeType : int32_t {
  kSpellOut,
};

This class then will go through jni and convert into the the java version of the StringAttribute.

  private enum StringAttributeType {
    SPELLOUT,
  }

  private static class StringAttribute {
    StringAttribute() {}

    private int start;
    private int end;
    private StringAttributeType type;
    private ByteBuffer args;
  }

Finally the createAccessibilityNodeInfo will builds a TtsSpan based on the list of StringAttribute

@@ -743,8 +743,11 @@ class SemanticsUpdateBuilder extends NativeFieldWrapperClass2 {
required double thickness,
required Rect rect,
required String label,
List<Map<String, dynamic>>? labelAttributes,

This comment has been minimized.

@goderbauer

goderbauer Apr 8, 2021
Member

Also commented on the framework side: wondering whether each attribute has to be represented as a map or if it could just be a list where we know that index 1 is the start, index 2 is the end, index 3 is the type, etc.

@@ -758,6 +761,7 @@ class SemanticsUpdateBuilder extends NativeFieldWrapperClass2 {
scrollChildren == 0 || scrollChildren == null || (scrollChildren > 0 && childrenInHitTestOrder != null),
'If a node has scrollChildren, it must have childrenInHitTestOrder',
);
print("before converted to c++ labelAttributes $labelAttributes");

This comment has been minimized.

@goderbauer

goderbauer Apr 8, 2021
Member

nit: remove this :)

@@ -1,10 +1,13 @@
// Copyright 2013 The Flutter Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
// found in the LICENSE file.stand

This comment has been minimized.

@goderbauer

goderbauer Apr 8, 2021
Member

nit: revert

@chunhtai chunhtai force-pushed the chunhtai:issues/79318 branch 2 times, most recently from 0684ed3 to 2c41de7 Apr 29, 2021
@chunhtai chunhtai marked this pull request as ready for review May 6, 2021
@chunhtai chunhtai requested a review from dnfield May 6, 2021
ScopedJavaLocalRef<jclass> byte_buffer_clazz(
env, env->FindClass("java/nio/ByteBuffer"));
FML_DCHECK(!byte_buffer_clazz.is_null());
jobjectArray joa =

This comment has been minimized.

@dnfield

dnfield May 7, 2021
Member

nit: joa not a descriptive name. perhaps just array or java_array?

ScopedJavaLocalRef<jobject> item(
env,
env->NewDirectByteBuffer((void*)(vector[i].data()), vector[i].size()));
env->SetObjectArrayElement(joa, i, item.obj());
Comment on lines 156 to 159

This comment has been minimized.

@dnfield

dnfield May 7, 2021
Member

I'm confused about how this works. Doesn't the ScopedJavaLocalRef delete the obj in its dtor?

This comment has been minimized.

@chunhtai

chunhtai May 7, 2021
Author Contributor

I copied over from the String converting method

This comment has been minimized.

@dnfield

dnfield May 7, 2021
Member

Ok. I'm probably just misunderstanding how JNIEnv::DeleteLocalRef works.

@@ -5,6 +5,7 @@
#ifndef FLUTTER_LIB_UI_SEMANTICS_SEMANTICS_NODE_H_
#define FLUTTER_LIB_UI_SEMANTICS_SEMANTICS_NODE_H_

#include <any>

This comment has been minimized.

@dnfield

dnfield May 7, 2021
Member

This looks like it doesn't belong here.

@@ -43,10 +46,15 @@ class SemanticsUpdateBuilder
double elevation,
double thickness,
std::string label,
std::string hint,
std::vector<std::vector<std::any>> labelAttributes,

This comment has been minimized.

@dnfield

dnfield May 7, 2021
Member

Why are we using std::any? Can we avoid it?

This comment has been minimized.

@dnfield

dnfield May 7, 2021
Member

For example, could we juse use some class(es) instead of vectors of vectors of anything?

This comment has been minimized.

@chunhtai

chunhtai May 7, 2021
Author Contributor

Is there a way to serialize a custom dart class? I couldn't figure out a way to do that with the dart api

This comment has been minimized.

@dnfield

dnfield May 7, 2021
Member

We do it by hand in a few places - for example

engine/lib/ui/painting.dart

Lines 1104 to 1116 in 9c793f1

// Paint objects are encoded in two buffers:
//
// * _data is binary data in four-byte fields, each of which is either a
// uint32_t or a float. The default value for each field is encoded as
// zero to make initialization trivial. Most values already have a default
// value of zero, but some, such as color, have a non-zero default value.
// To encode or decode these values, XOR the value with the default value.
//
// * _objects is a list of unencodable objects, typically wrappers for native
// objects. The objects are simply stored in the list without any additional
// encoding.
//
// The binary format must match the deserialization code in paint.cc.

This comment has been minimized.

@chunhtai

chunhtai May 7, 2021
Author Contributor

Sounds good

@@ -415,6 +417,70 @@ struct DartConverter<std::vector<T>> {
}
};

/// Json style map.

This comment has been minimized.

@dnfield

dnfield May 7, 2021
Member

I would prefer we avoid this if we can. It's harder to use API like this consistently across multiple platforms/implementations. We'll lose a fair amount of static typechecking here, and it'll be easier to break an implementation unexpectedly.

It's also more expensive/involves a lot more Dart_* api calls than we typically do for argument conversion.

Typically, we use Uint8Lists to pack/unpack structured data between Dart land and native.

@chunhtai chunhtai force-pushed the chunhtai:issues/79318 branch from 46eaa10 to ab91cc1 May 10, 2021
@chunhtai
Copy link
Contributor Author

@chunhtai chunhtai commented May 10, 2021 •

Hi @dnfield, this is ready for another look. The change is in the last commit. This is what i did: I moved the AttritbutedString class from framework to dart:ui, and I wrote a DartConverter to directly convert the dart class to c++ class.

@@ -275,6 +275,123 @@ class SemanticsFlag {
}
}

class AttributedString {

This comment has been minimized.

@chunhtai

chunhtai May 10, 2021 •
Author Contributor

I wonder if there is a way to avoid this duplication?
This class has to be in dart:ui so that i can write the class specific dartconverter. If we put it in framework side, we will need to go back to the List<Object?> thing with the std::any thing I did before.

This comment has been minimized.

@dnfield

dnfield May 10, 2021
Member

Yes, we end up duplicating classes from dart:ui into the web implementation.

The good part about doing it here is we can think a bit more about how it impacts web, if at all. It may make sense to throw an unimplemented error until this is implemented, but it'd be worth checking with @ferhatb or @yjbanov about that.

This comment has been minimized.

@yjbanov

yjbanov May 14, 2021
Contributor

If it's simply copy&paste, then it's fine. That's how things work as of right now. There are ideas to automate it using some script, but we can directly share code because dart2js doesn't understand the native keyword and native wrapper classes. Since the Flutter team is the only non-Dart SDK user of the dart:* namespace it is unlikely to be fixed.

New API should not throw exceptions. Otherwise, someone could start using this API in one of the standard widgets, such as ElevatedButton and break all of Flutter Web. Instead, we should implement it, or provide a graceful fallback/polyfill.

final String string;

/// The attributes the [string] carries.
final List<StringAttribute>? attributes;

This comment has been minimized.

@dnfield

dnfield May 10, 2021
Member

This probably shouldn't be nullable.

It seems strange to have this and have the named constructors and have ==/hashCode overrides. Someone could easily push to this right? In that case, the hash codewould be unstable.

This comment has been minimized.

@dnfield

dnfield May 10, 2021
Member

The more I think about this, the more I think we need to document this is not modifable after construction. It might also be good to not even expose this publicly, or to expose it only for debugging purposes, so that it doesn't accidentally get modified and then not reflected in the tree (since modifying it would have no effect on the tree).

It's also confusing because it seems like some attributes would be mutually exclusive, and we don't have any validation logic here asserting that you avoid adding mututally exclusive ones.

That should let us keep the ==/hashCode if we really need it.

This comment has been minimized.

@chunhtai

chunhtai May 10, 2021
Author Contributor

I think I can make this nonnullable, but I have to expose it for framework side to do string concatenation.
Is there a way to only expose this for framework internal?

This comment has been minimized.

@chunhtai

chunhtai May 10, 2021
Author Contributor

I think dart should just provide a immutable list/map/set..

This comment has been minimized.

@chunhtai

chunhtai May 10, 2021
Author Contributor

ah we can use List.unmodifiable. I think this is probably better?

enum StringAttributeType {
/// The string should be spell out character by character.
///
/// The additional argument of the string attribute is null.
spellOut,
/// The string should be treated as a specific language.
///
/// The additional argument of the string attribute is a string language tag.
locale,
}
Comment on lines 742 to 752

This comment has been minimized.

@dnfield

dnfield May 10, 2021
Member

Why do we need this public? Shouldn't we just have subclasses of StringAttribute instead of an enum type field in it?

This seems like it might be better implemented as private constants that correspond to the C++ impl.

This comment has been minimized.

@chunhtai

chunhtai May 10, 2021 •
Author Contributor

In the framework side, we need to concatenate the AttributedString https://github.com/flutter/flutter/pull/79599/files#r629679123

If we make subclasses of StringAttribute, each class will need duplicate copyWithRange if they add new field into the subclasses. I feel that is unnecessary and they should just use the Object? args to save their additional arguments. That is why I decided to create a bunch of named constructor.

I think we can keep the StringAttributeType private though, i will change it

auto dart_attributed_string_string_string =
Dart_NewStringFromCString(flutter::kAttributedStringStringFieldName);
auto dart_attributed_string_attributes_string =
Dart_NewStringFromCString(flutter::kAttributedStringAttributesFieldName);

// Gets the string from the dart_attributed_string.
const char* string = NULL;
auto dart_attributed_string_string = Dart_GetField(
dart_attributed_string, dart_attributed_string_string_string);
Dart_StringToCString(dart_attributed_string_string, &string);

// Gets the attributes from the dart_attributed_string.
auto dart_attributed_string_attributes = Dart_GetField(
dart_attributed_string, dart_attributed_string_attributes_string);
flutter::StringAttributes attributes =
DartConverter<flutter::StringAttributes>::FromDart(
dart_attributed_string_attributes);
return {.string = string, .attributes = attributes};
Comment on lines 87 to 104

This comment has been minimized.

@dnfield

dnfield May 10, 2021
Member

This will be slow, especially if it's something we have to do often, which AFAICT we will.

We may want to consider implementing the attributed strings as NativeFieldWrapper2 classes, which is probably fine since all the variables are final - although I'm not clear on the implications of adding or removing attributes (do we expect that strings really can have multiple attributes, and users will be changing those attributes? Is there some point where we don't allow it anymore? See comments above in the dart impl). Alternatively, we should just shove all this information into a TypedData array and unpack it on the C++ side.

If this object is created once and not changed, we should probably do NativeFieldWrapper2. If it needs to be/can be updated more frequently, we should come up with a more efficient serialization scheme.

This comment has been minimized.

@chunhtai

chunhtai May 10, 2021
Author Contributor

I don't have much knowledge around the performance implication, but I will trust your judgement and make it a NativeFieldWrapper2.

It is possible to have multiple attributes, for example you may have two different text ranges that need to be spell out or a string contains multiple language and needs to be annotated separately.

This comment has been minimized.

@chunhtai

chunhtai May 10, 2021
Author Contributor

I have two concerns about converting this to NativeFieldWrapper2. The first one is the web implementation will be completely different from the mobile. This may make it hard to maintain. The other i am not sure how inheritance would work for NativeFieldWrapper2 since the StringAttribute will have subclasses.

I think I will just use TypeData array for now.

///
/// Depends on what [type] this attribute is, it may or may not carry an
/// additional arguement. For which more information, see the [StringAttributeType].
final Object? args;

This comment has been minimized.

@dnfield

dnfield May 10, 2021
Member

If we rearrange this into separate classes for the concrete attribute types, this can just go away and the concrete class can be responsible for implementing it.

This comment has been minimized.

@chunhtai

chunhtai May 10, 2021
Author Contributor

If we do this, each class need to implement their own copyWithRange. do you think that is more preferable?

This comment has been minimized.

@goderbauer

goderbauer May 10, 2021
Member

I think that would be more preferable. Having a generic Object? here is odd.

This comment has been minimized.

@dnfield

dnfield May 10, 2021
Member

You could make a protected method to help with it right?

Copy link
Member

@goderbauer goderbauer left a comment

(just looked at the dart code)

class AttributedString {
/// Creates a attributed string.
///
/// the [TextRange] in the [attributes] must be inside the length of the

This comment has been minimized.

@goderbauer

goderbauer May 10, 2021
Member

the -> The

this.attributes,
}) : assert((){
if (attributes != null) {
for(final StringAttribute attribute in attributes) {

This comment has been minimized.

@goderbauer

goderbauer May 10, 2021
Member

nit: space after "for"

attribute.range.start <= string.length && attribute.range.end <= string.length,
'The range of $attribute is outside of the string $string'
Comment on lines 657 to 658

This comment has been minimized.

@goderbauer

goderbauer May 10, 2021
Member

nit: indentation


/// Creates a string that needs to be spelled out in assistive technologies.
///
/// The [range] specifies the text range that should be spelled out. Set it

This comment has been minimized.

@goderbauer

goderbauer May 10, 2021
Member

Set -> Setting

/// to null causes the assistive technologies to spell out the whole string.
///
/// See also:
/// * [SpellOutStringAttribute], which is the attribute this factory uses.

This comment has been minimized.

@goderbauer

goderbauer May 10, 2021
Member

blank line before this

// * engine/src/flutter/shell/platform/android/io/flutter/view/AccessibilityBridge.java

/// The enum that is used in [StringAttribute] class.
enum StringAttributeType {

This comment has been minimized.

@goderbauer

goderbauer May 10, 2021
Member

should this be private?

locale,
}

/// An abstract interface for string attribute that affects how assistive

This comment has been minimized.

@goderbauer

goderbauer May 10, 2021
Member

should the class then be marked as "abstract"?

///
/// Depends on what [type] this attribute is, it may or may not carry an
/// additional arguement. For which more information, see the [StringAttributeType].
final Object? args;

This comment has been minimized.

@goderbauer

goderbauer May 10, 2021
Member

I think that would be more preferable. Having a generic Object? here is odd.

final Object? args;

/// Create a new copy of this string attribute with the given range.
StringAttribute copyWithRange(TextRange range) {

This comment has been minimized.

@goderbauer

goderbauer May 10, 2021
Member

nit: could just be named "copy" with an optional range argument. If non is provided, the current range is used. I think that's the copy pattern we sue elsewhere.

@@ -57,10 +57,15 @@ class SemanticsNodeUpdate {
required this.scrollExtentMin,
required this.rect,
required this.label,
required this.hint,
this.labelAttributes,

This comment has been minimized.

@goderbauer

goderbauer May 10, 2021
Member

Why the seperation here (label & labelAttributes instead of attributedLabel)?

@ferhatb ferhatb requested a review from yjbanov May 10, 2021
@chunhtai chunhtai force-pushed the chunhtai:issues/79318 branch from 1686cbd to e12ac2a May 12, 2021
@yjbanov
Copy link
Contributor

@yjbanov yjbanov commented May 14, 2021 •

@chunhtai I am unable to see the design doc. I'm told to request for permission.

@chunhtai chunhtai force-pushed the chunhtai:issues/79318 branch 2 times, most recently from 73446e3 to 25f9455 May 14, 2021
@chunhtai chunhtai requested review from goderbauer and dnfield May 17, 2021
@chunhtai
Copy link
Contributor Author

@chunhtai chunhtai commented May 17, 2021

Ready for another look!

Copy link
Member

@dnfield dnfield left a comment

Mainly focusing on the Dart/native interfaces right now.

lib/ui/semantics.dart Outdated Show resolved Hide resolved
lib/ui/semantics.dart Outdated Show resolved Hide resolved
lib/ui/semantics.dart Outdated Show resolved Hide resolved
lib/ui/semantics.dart Outdated Show resolved Hide resolved
lib/ui/semantics.dart Show resolved Hide resolved
lib/ui/semantics/attributed_string.cc Outdated Show resolved Hide resolved
lib/ui/semantics/attributed_string.cc Outdated Show resolved Hide resolved
lib/ui/semantics/attributed_string_unittests.cc Outdated Show resolved Hide resolved
lib/ui/semantics/string_attribute.cc Outdated Show resolved Hide resolved
@chunhtai chunhtai force-pushed the chunhtai:issues/79318 branch from 0ea2e15 to 9698c01 May 18, 2021
@chunhtai chunhtai requested a review from dnfield May 18, 2021
@chunhtai chunhtai requested review from goderbauer and dnfield May 23, 2021
Copy link
Member

@dnfield dnfield left a comment

LGTM if LGT @goderbauer

Copy link
Member

@goderbauer goderbauer left a comment

LGTM after doc comments are addressed.

@@ -115,18 +120,33 @@ class SemanticsNodeUpdate {
/// See [ui.SemanticsUpdateBuilder.updateNode].
final String label;

/// See [ui.SemanticsUpdateBuilder.updateNode].

This comment has been minimized.

@goderbauer

goderbauer May 25, 2021
Member

(here and elsewhere): The API doc on that methods doesn't seem to mention this field.

This comment has been minimized.

@goderbauer

goderbauer May 25, 2021
Member

Also: Let's document that the content of these lists must not change.

@chunhtai chunhtai force-pushed the chunhtai:issues/79318 branch from 87073c7 to 56c9cd5 May 26, 2021
@chunhtai chunhtai force-pushed the chunhtai:issues/79318 branch from d5bb8ca to 1538707 May 26, 2021
@chunhtai chunhtai force-pushed the chunhtai:issues/79318 branch from 1538707 to 56af58c Jun 1, 2021
@fluttergithubbot fluttergithubbot merged commit 9502b6f into flutter:master Jun 1, 2021
25 checks passed
25 checks passed
@flutter-dashboard
Linux Android AOT Engine
Details
@flutter-dashboard
Linux Android Debug Engine
Details
@flutter-dashboard
Linux Android Scenarios
Details
@flutter-dashboard
Linux Arm Host Engine
Details
@flutter-dashboard
Linux Framework Smoke Tests
Details
@flutter-dashboard
Linux Fuchsia
Details
@flutter-dashboard
Linux Fuchsia FEMU
Details
@flutter-dashboard
Linux Host Engine
Details
@flutter-dashboard
Linux Web Engine
Details
@flutter-dashboard
Linux Web Framework tests
Details
@flutter-dashboard
Mac Android AOT Engine
Details
@flutter-dashboard
Mac Android Debug Engine
Details
@flutter-dashboard
Mac Host Engine
Details
@flutter-dashboard
Mac Web Engine
Details
@flutter-dashboard
Mac iOS Engine
Details
@wip
WIP Ready for review
Details
@flutter-dashboard
Windows Android AOT Engine
Details
@flutter-dashboard
Windows Host Engine
Details
@flutter-dashboard
Windows UWP Engine
Details
@flutter-dashboard
Windows Web Engine
Details
@cirrus-ci
build_and_test_linux_unopt_debug Task Summary
Details
@flutter-dashboard
ci.yaml validation
Details
@google-cla
cla/google All necessary CLAs are signed
@cirrus-ci
licenses_check Task Summary
Details
@flutter-dashboard
luci-engine
Details
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 1, 2021
iskakaushik added a commit to iskakaushik/engine that referenced this pull request Jun 1, 2021
jason-simmons added a commit to jason-simmons/flutter_engine that referenced this pull request Jun 2, 2021
flutter#25373 introdued APIs that
return SpannableString, which is a CharSequence subclass that was
not previously supported by StandardMessageCodec

See flutter/flutter#83751
jason-simmons added a commit to jason-simmons/flutter_engine that referenced this pull request Jun 2, 2021
flutter#25373 introdued APIs that
return SpannableString, which is a CharSequence subclass that was
not previously supported by StandardMessageCodec

See flutter/flutter#83751
jason-simmons added a commit to jason-simmons/flutter_engine that referenced this pull request Jun 2, 2021
flutter#25373 introdued APIs that
return SpannableString, which is a CharSequence subclass that was
not previously supported by StandardMessageCodec

See flutter/flutter#83751
jason-simmons added a commit that referenced this pull request Jun 2, 2021
#25373 introdued APIs that
return SpannableString, which is a CharSequence subclass that was
not previously supported by StandardMessageCodec

See flutter/flutter#83751
chunhtai added a commit to chunhtai/engine that referenced this pull request Jun 2, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment