Repository navigation
[a11y] Semanctis flag refactor step 1: engine part - #167421
Conversation
github-actions
Bot
added
platform-android
hannah-hyj
changed the title
b7b8485 to
9558f31
Compare
1fb5399 to
8a0d539
Compare
chunhtai
left a comment
chunhtai
left a comment
There was a problem hiding this comment.
LGTM, just some comments
| String toString() => 'SemanticsFlag.$name'; | ||
| } | ||
|
|
||
| /// Semantics flags. |
There was a problem hiding this comment.
this doc string is not useful.
| }); | ||
|
|
||
| /// SemanticsFlags with everything default to false. | ||
| static const SemanticsFlags none = SemanticsFlags(); |
There was a problem hiding this comment.
| static const SemanticsFlags none = SemanticsFlags(); | |
| static const SemanticsFlags kNone = SemanticsFlags(); |
There was a problem hiding this comment.
We use none for SemanticsRole, SemanticsInputType, and SemanticsValidationResult. Why deviate here?
There was a problem hiding this comment.
this is a constant object we usually prefix with k See(SemanticsConfiguration._kEmptyConfig).
SemanticsInputType and SemanticsValidationResult.none is an actual enum
| } | ||
|
|
||
| /// Semantics flags. | ||
| class SemanticsFlags { |
There was a problem hiding this comment.
@immutable is an annotation from package:meta. We cannot use it in the engine.
There was a problem hiding this comment.
ah i forgot this is in web engine
|
|
||
| /// Semantics flags. | ||
| class SemanticsFlags { | ||
| /// All flags default to `false` unless specified. |
There was a problem hiding this comment.
nit: the first paragraph of a constructor dartdoc should say what it does, such as "Creates a set of semantics flags that describe various states of a widget". The nuances of arguments usually goes into a separate paragraph.
| this.isRequired = false, | ||
| }); | ||
|
|
||
| /// SemanticsFlags with everything default to false. |
There was a problem hiding this comment.
nit: consider rephrasing, such as "The set of semantics flags with every flag set to false" (the word "default" implies a starting value that you can change later, but SemanticsFlags.none guaranteed that every flag is set to false and it cannot be changed)
| /// {@macro dart.ui.semantics.isRequired} | ||
| final bool isRequired; | ||
|
|
||
| /// Merges the flags from this object with [other]. |
There was a problem hiding this comment.
nit: It's unclear what "merges" implies. Consider expanding, e.g. "Combines two sets of flags, such that if a flag it set to true in any of the two sets, the resulting set contains that flag set to true."
| String toString() => 'SemanticsFlag.$name'; | ||
| } | ||
|
|
||
| /// Semantics flags. |
There was a problem hiding this comment.
We don't include the dartdocs in the web copies of the dart:ui API (this way they don't get stale).
This comment applies here and below.
| } | ||
| } | ||
|
|
||
| int64_t flagsToInt64(flutter::SemanticsFlags flags) { |
There was a problem hiding this comment.
Since is doesn't make sense for flags to have a sign, should we use an unsigned type, e.g. uint64_t?
There was a problem hiding this comment.
we will read it on java side using buffer.getLong(); and it seems there's no unsigned long in java. So I think it's ok to just use int64 here
| }; | ||
| } | ||
|
|
||
| FlutterSemanticsFlag SemanticsFlagtoInt(const SemanticsFlags& flags) { |
There was a problem hiding this comment.
The spelling/camel-casing seems off. Should this be SemanticsFlagsToInt?
|
|
||
| int32_t id = 0; | ||
| int32_t flags = 0; | ||
| SemanticsFlags flags{}; |
There was a problem hiding this comment.
Since you've initialized the default values of everything in SemanticsFlag struct you can just write this as SemanticsFlags flags
Update semantics.dart lint Update semantics.dart lint Update apicheck_test.dart fix tests SemanticsFlags updates SemanticsFlags updates lint Update semantics_update_builder.cc 1
05dff38 to
75c8fe8
Compare
hannah-hyj
added
the
autosubmit
4 participants
issue: #166101
step 1: Add struct SemanticsFlags in semantics_node.h and class SemanticsFlags in dart:ui, it's still a bitmask in embedder and framework.
TODO:
Other parts will be in other following PRs
Embedder
add a struct FlutterSemanticsFlags
add FlutterSemanticsFlags* to FlutterSemanticsNode2
web engine
use the new class
SemanticsUpdateBuilder.updateNode
pass a list of bools instead of bitmask
Framework
use the SemanticsFlags class instead of bitmask
flutter_tester
use the SemanticsFlags class instead of bitmask
apicheck_test.dart
Add this test back
Replace this paragraph with a description of what this PR is changing or adding, and why. Consider including before/after screenshots.
List which issues are fixed by this PR. You must list at least one issue. An issue is not required if the PR fixes something trivial like a typo.
If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.