Skip to content

MenuItemButton and SubmenuButton should be used in a menu or menu bar #1443

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

Merged
merged 2 commits into from
Mar 27, 2025

Conversation

QuncCccccc
Copy link
Contributor

This is to unblock PR: flutter/flutter#165596 which has test failures in customer testing.

MenuItemButton and SubmenuButton widgets are wired up with SemanticsRole.menuItem and they require their parent to be either SemanticsRole.menu or SemanticsRole.menuBar.

@chrisbobbe chrisbobbe force-pushed the fix_menu_item_button_semantics branch from 1731386 to 71d8478 Compare March 27, 2025 01:10
@chrisbobbe
Copy link
Collaborator

Thanks! I've just pushed a revision with a small whitespace change and a new commit message; @gnprice PTAL. :)

@chrisbobbe chrisbobbe added the integration review Added by maintainers when PR may be ready for integration label Mar 27, 2025
@chrisbobbe chrisbobbe requested a review from gnprice March 27, 2025 01:11
chrisbobbe and others added 2 commits March 26, 2025 18:50

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Flutter PR
  flutter/flutter#165596
(wiring up MenuAnchor-related widgets to SemanticsRole) breaks this
test; fix it by adding the needed Semantics wrappers.

Co-authored-by: Chris Bobbe <[email protected]>
@chrisbobbe chrisbobbe force-pushed the fix_menu_item_button_semantics branch from 71d8478 to e0dff1e Compare March 27, 2025 01:52
@chrisbobbe
Copy link
Collaborator

chrisbobbe commented Mar 27, 2025

(Added a commit bumping our Flutter-version floor because Greg noticed some failing tests with a Flutter version in the older range; also sent #1444 which I noticed was needed.)

@gnprice
Copy link
Member

gnprice commented Mar 27, 2025

Thanks! Looks good; merging.

@gnprice gnprice merged commit e0dff1e into zulip:main Mar 27, 2025
1 check passed
@gnprice
Copy link
Member

gnprice commented Mar 27, 2025

FTR the test failures I saw looked like this, when running the Flutter commit that was in pubspec.yaml before the upgrade in this PR (flutter/flutter@a780f85, from last week):

00:38 +1022 ~42 -104: /home/greg/z/flutterz/test/widgets/emoji_reaction_test.dart: EmojiPicker handle view paddings with bottom view padding
══╡ EXCEPTION CAUGHT BY SCHEDULER LIBRARY ╞═════════════════════════════════════════════════════════
The following assertion was thrown during a scheduler callback:
Missing checks for role SemanticsRole.menu

When the exception was thrown, this was the stack:
#0      SemanticsNode._addToUpdate.<anonymous closure> (package:flutter/src/semantics/semantics.dart:3288:9)
#1      SemanticsNode._addToUpdate (package:flutter/src/semantics/semantics.dart:3291:6)
#2      SemanticsOwner.sendSemanticsUpdate (package:flutter/src/semantics/semantics.dart:4049:14)
#3      PipelineOwner.flushSemantics (package:flutter/src/rendering/object.dart:1474:24)
#4      PipelineOwner.flushSemantics (package:flutter/src/rendering/object.dart:1476:15)
#5      AutomatedTestWidgetsFlutterBinding.drawFrame (package:flutter_test/src/binding.dart:1508:35)
#6      RendererBinding._handlePersistentFrameCallback (package:flutter/src/rendering/binding.dart:495:5)
#7      SchedulerBinding._invokeFrameCallback (package:flutter/src/scheduler/binding.dart:1438:15)
#8      SchedulerBinding.handleDrawFrame (package:flutter/src/scheduler/binding.dart:1351:9)
#9      AutomatedTestWidgetsFlutterBinding.pump.<anonymous closure> (package:flutter_test/src/binding.dart:1340:9)
#12     TestAsyncUtils.guard (package:flutter_test/src/test_async_utils.dart:74:41)
#13     AutomatedTestWidgetsFlutterBinding.pump (package:flutter_test/src/binding.dart:1329:27)
#14     WidgetTester.pump.<anonymous closure> (package:flutter_test/src/widget_tester.dart:653:53)
#17     TestAsyncUtils.guard (package:flutter_test/src/test_async_utils.dart:74:41)
#18     WidgetTester.pump (package:flutter_test/src/widget_tester.dart:653:27)
#19     main.<anonymous closure>.setupEmojiPicker (file:///home/greg/z/flutterz/test/widgets/emoji_reaction_test.dart:338:20)
<asynchronous suspension>
#20     main.<anonymous closure>.<anonymous closure>.prepare (file:///home/greg/z/flutterz/test/widgets/emoji_reaction_test.dart:496:9)
<asynchronous suspension>
#21     main.<anonymous closure>.<anonymous closure>.<anonymous closure> (file:///home/greg/z/flutterz/test/widgets/emoji_reaction_test.dart:533:9)
<asynchronous suspension>
#22     testWidgets.<anonymous closure>.<anonymous closure> (package:flutter_test/src/widget_tester.dart:193:15)
<asynchronous suspension>
#23     TestWidgetsFlutterBinding._runTestBody (package:flutter_test/src/binding.dart:1064:5)
<asynchronous suspension>
<asynchronous suspension>
(elided 5 frames from dart:async and package:stack_trace)
════════════════════════════════════════════════════════════════════════════════════════════════════
00:38 +1022 ~42 -105: /home/greg/z/flutterz/test/widgets/emoji_reaction_test.dart: EmojiPicker handle view paddings with bottom view padding [E]
  Test failed. See exception logs above.
  The test description was: with bottom view padding

I'm guessing the relevant change we needed to get by upgrading was flutter/flutter@32b34ff.

@QuncCccccc
Copy link
Contributor Author

Ah yes, the menu-related SemanticsRoles are added in flutter/flutter@32b34ff.

@QuncCccccc
Copy link
Contributor Author

Just a question, do you know when the change will be synced with flutter/flutter? No rush at all, I'm just curious😆

@gnprice
Copy link
Member

gnprice commented Mar 27, 2025

Yeah, you can do that by making a quick PR to flutter/tests (which I can stamp-review), and then updating the flutter/tests pin as part of your flutter/flutter PR.

It looks like the best docs for this process are here:
https://github.com/flutter/tests?tab=readme-ov-file#if-a-test-is-broken

with a recent change to the process found here:
flutter/flutter#162041
flutter/flutter#162048
flutter/tests#445

@QuncCccccc
Copy link
Contributor Author

Thank you! Just created a PR for flutter/tests: flutter/tests#450

QuncCccccc added a commit to flutter/tests that referenced this pull request Mar 28, 2025
This is to fix the customer testing in
flutter/flutter#165596
by picking up zulip/zulip-flutter#1443

## Pre-launch Checklist

- [x] I read the [Contributor Guide] and followed the process outlined
there for submitting PRs.
- [x] I read the [Tree Hygiene] wiki page, which explains my
responsibilities.
- [x] I read the [Flutter Style Guide] _recently_, and have followed its
advice.
- [x] I signed the [CLA].
- [x] I listed at least one issue that this PR fixes in the description
above.
- [x] I updated/added relevant documentation (doc comments with `///`).
- [x] I added new tests to check the change I am making, or this PR is
[test-exempt].
- [x] All existing and new tests are passing.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integration review Added by maintainers when PR may be ready for integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants