Add API to the engine to support attributed text #25373
Conversation
|
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 |
| @@ -743,8 +743,11 @@ class SemanticsUpdateBuilder extends NativeFieldWrapperClass2 { | |||
| required double thickness, | |||
| required Rect rect, | |||
| required String label, | |||
| List<Map<String, dynamic>>? labelAttributes, | |||
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.
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"); | |||
goderbauer
Apr 8, 2021
Member
nit: remove this :)
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 | |||
goderbauer
Apr 8, 2021
Member
nit: revert
nit: revert
0684ed3
to
2c41de7
| ScopedJavaLocalRef<jclass> byte_buffer_clazz( | ||
| env, env->FindClass("java/nio/ByteBuffer")); | ||
| FML_DCHECK(!byte_buffer_clazz.is_null()); | ||
| jobjectArray joa = |
dnfield
May 7, 2021
Member
nit: joa not a descriptive name. perhaps just array or java_array?
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()); |
dnfield
May 7, 2021
Member
I'm confused about how this works. Doesn't the ScopedJavaLocalRef delete the obj in its dtor?
I'm confused about how this works. Doesn't the ScopedJavaLocalRef delete the obj in its dtor?
chunhtai
May 7, 2021
Author
Contributor
I copied over from the String converting method
I copied over from the String converting method
dnfield
May 7, 2021
Member
Ok. I'm probably just misunderstanding how JNIEnv::DeleteLocalRef works.
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> | |||
dnfield
May 7, 2021
Member
This looks like it doesn't belong here.
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, | |||
dnfield
May 7, 2021
Member
Why are we using std::any? Can we avoid it?
Why are we using std::any? Can we avoid it?
dnfield
May 7, 2021
Member
For example, could we juse use some class(es) instead of vectors of vectors of anything?
For example, could we juse use some class(es) instead of vectors of vectors of anything?
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
Is there a way to serialize a custom dart class? I couldn't figure out a way to do that with the dart api
dnfield
May 7, 2021
chunhtai
May 7, 2021
Author
Contributor
Sounds good
Sounds good
| @@ -415,6 +417,70 @@ struct DartConverter<std::vector<T>> { | |||
| } | |||
| }; | |||
|
|
|||
| /// Json style map. | |||
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.
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.
|
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 { | |||
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.
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.
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.
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.
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.
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; |
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 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.
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.
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.
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?
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?
chunhtai
May 10, 2021
Author
Contributor
I think dart should just provide a immutable list/map/set..
I think dart should just provide a immutable list/map/set..
chunhtai
May 10, 2021
Author
Contributor
ah we can use List.unmodifiable. I think this is probably better?
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, | ||
| } |
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.
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.
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
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}; |
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 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.
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.
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.
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.
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; |
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.
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.
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?
If we do this, each class need to implement their own copyWithRange. do you think that is more preferable?
goderbauer
May 10, 2021
Member
I think that would be more preferable. Having a generic Object? here is odd.
I think that would be more preferable. Having a generic Object? here is odd.
dnfield
May 10, 2021
Member
You could make a protected method to help with it right?
You could make a protected method to help with it right?
|
(just looked at the dart code) |
| class AttributedString { | ||
| /// Creates a attributed string. | ||
| /// | ||
| /// the [TextRange] in the [attributes] must be inside the length of the |
goderbauer
May 10, 2021
Member
the -> The
the -> The
| this.attributes, | ||
| }) : assert((){ | ||
| if (attributes != null) { | ||
| for(final StringAttribute attribute in attributes) { |
goderbauer
May 10, 2021
Member
nit: space after "for"
nit: space after "for"
| attribute.range.start <= string.length && attribute.range.end <= string.length, | ||
| 'The range of $attribute is outside of the string $string' |
goderbauer
May 10, 2021
Member
nit: indentation
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 |
goderbauer
May 10, 2021
Member
Set -> Setting
Set -> Setting
| /// to null causes the assistive technologies to spell out the whole string. | ||
| /// | ||
| /// See also: | ||
| /// * [SpellOutStringAttribute], which is the attribute this factory uses. |
goderbauer
May 10, 2021
Member
blank line before this
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 { |
goderbauer
May 10, 2021
Member
should this be private?
should this be private?
| locale, | ||
| } | ||
|
|
||
| /// An abstract interface for string attribute that affects how assistive |
goderbauer
May 10, 2021
Member
should the class then be marked as "abstract"?
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; |
goderbauer
May 10, 2021
Member
I think that would be more preferable. Having a generic Object? here is odd.
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) { |
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.
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, | |||
goderbauer
May 10, 2021
Member
Why the seperation here (label & labelAttributes instead of attributedLabel)?
Why the seperation here (label & labelAttributes instead of attributedLabel)?
|
@chunhtai I am unable to see the design doc. I'm told to request for permission. |
73446e3
to
25f9455
|
Ready for another look! |
|
Mainly focusing on the Dart/native interfaces right now. |
|
LGTM if LGT @goderbauer |
|
LGTM after doc comments are addressed. |
| @@ -115,18 +120,33 @@ class SemanticsNodeUpdate { | |||
| /// See [ui.SemanticsUpdateBuilder.updateNode]. | |||
| final String label; | |||
|
|
|||
| /// See [ui.SemanticsUpdateBuilder.updateNode]. | |||
goderbauer
May 25, 2021
Member
(here and elsewhere): The API doc on that methods doesn't seem to mention this field.
(here and elsewhere): The API doc on that methods doesn't seem to mention this field.
goderbauer
May 25, 2021
Member
Also: Let's document that the content of these lists must not change.
Also: Let's document that the content of these lists must not change.
9502b6f
into
flutter:master
…)" This reverts commit 9502b6f.
flutter#25373 introdued APIs that return SpannableString, which is a CharSequence subclass that was not previously supported by StandardMessageCodec See flutter/flutter#83751
flutter#25373 introdued APIs that return SpannableString, which is a CharSequence subclass that was not previously supported by StandardMessageCodec See flutter/flutter#83751
flutter#25373 introdued APIs that return SpannableString, which is a CharSequence subclass that was not previously supported by StandardMessageCodec See flutter/flutter#83751
#25373 introdued APIs that return SpannableString, which is a CharSequence subclass that was not previously supported by StandardMessageCodec See flutter/flutter#83751
Design doc: flutter.dev/go/a11y-text-attributes
framework pr: flutter/flutter#79599
issue flutter/flutter#79318
Pre-launch Checklist
writing and running engine tests.
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.