Introduce SemanticsFlag.hasSelectedState to the engine API #55780
Conversation
a91a9da to
975e1d0
Compare
There was a problem hiding this comment.
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
| // `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.
Thank you for updating the comment here!
| /// 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.
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.
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.
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.
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
flutter/engine@bf8530e...09598f6 2024-10-17 yjbanov@google.com 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 jonahwilliams@google.com,zra@google.com 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
…ngine#55780) Introduce `SemanticsFlag.hasSelectedState` to the engine API. At this point the flag is not backed by any engine functionality. See also: flutter#66673 (comment) |⚠️ WARNING | |:----------------------------| | Submit _AFTER_ flutter#157017 and flutter#157061 land in the framework | A step towards flutter#66673
Introduce
SemanticsFlag.hasSelectedStateto 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