-
Notifications
You must be signed in to change notification settings - Fork 6k
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
Introduce SemanticsFlag.hasSelectedState to the engine API #55780
Conversation
a91a9da
to
975e1d0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you'll also need to update flutter/shell/platform/android/io/flutter/view/AccessibilityBridge.java
around line 2161. Search for IS_EXPANDED(1 << 27)
.
If tools/api_check/test/apicheck_test.dart
doesn't blow up, we should fix that. Hopefully it will!
@cbracken Yep, will update everything. This is still WIP. Sorry, I should've made it a draft. |
…7017) This test relies on the enumeration of all engine flags, and it will fail when flutter/engine#55780 adds `SemanticsFlag.hasSelectedState`. So temporarily skip the test to allow the new flag to be added. I will later unskip the test and change it to check for the new flag as well. A step towards #66673
ea68634
to
484e08e
Compare
484e08e
to
f2e6754
Compare
…tter#157017) This test relies on the enumeration of all engine flags, and it will fail when flutter/engine#55780 adds `SemanticsFlag.hasSelectedState`. So temporarily skip the test to allow the new flag to be added. I will later unskip the test and change it to check for the new flag as well. A step towards flutter#66673
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me
// `flutter_test/controller.dart#SemanticsController._importantFlags` | ||
// must be updated as well. | ||
static const int _kHasSelectedStateIndex = 1 << 28; | ||
// READ THIS: if you add a flag here, you MUST update the following: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for updating the comment here!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 -- much nicer.
/// Whether the widget corresponding to this node is currently selected or not | ||
/// is determined by the [isSelected] flag. | ||
/// | ||
/// When this flag is not set, the corresponding widget cannot be selected by |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, if hasSelectedState is a noop in mobile, should we add comments this flag value is only used in web?
Or do you mean in mobile engine we also check the hasSelectedState flag value before setting hasSelectedState?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you foresee an issue with mandating that the framework always provides hasSelectedState
for selectable widgets? I'm thinking it would be simpler if all platforms followed the same rules. SemanticsProperties.selected is already nullable, and the framework only sets SemanticsFlag.isSelected
if it is not null here and here. This means that the framework can always supply the hasSelectedState
flag for all selectable widgets on all platforms. But maybe I'm missing something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I don't see an issue with framework always providehasSelectedState
for selectable widgets.
I just wanted to confirm the flag is also used in mobile engines because you previously said it will be mostly noop on mobile.
I just to make sure
like this sample in the original issue (flutter/flutter#66673)
Semantics(
selected: true,
button: true,
child: Center(
child: Text(
'Text',
),
),
)
before the changes, it will be displayed as a selected widget on mobile, but non-selectable widget on web.
After the changes (this PR and future related PR), it will be a non-selectable widget on both mobile and web because now you need to always specify the hasSelectedState flag even for mobile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Talked with yegor offline and this is resolved.
cases like
Semantics(
selected: true,
button: true,
child: Center(
child: Text(
'Text',
),
),
)
should automatically set hasSelectedState
to true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…157130) flutter/engine@bf8530e...09598f6 2024-10-17 [email protected] Introduce SemanticsFlag.hasSelectedState to the engine API (flutter/engine#55780) 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] 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://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Introduce
SemanticsFlag.hasSelectedState
to the engine API. At this point the flag is not backed by any engine functionality. See also: flutter/flutter#66673 (comment)A step towards flutter/flutter#66673