Skip to content

Wrap PopupMenu with SafeArea to respect status bar#64678

Merged
fluttergithubbot merged 10 commits into
flutter:masterfrom
JuYeong0413:popup-menu
Oct 7, 2020
Merged

Wrap PopupMenu with SafeArea to respect status bar#64678
fluttergithubbot merged 10 commits into
flutter:masterfrom
JuYeong0413:popup-menu

Conversation

@JuYeong0413

@JuYeong0413 JuYeong0413 commented Aug 27, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

Description

Continuous work of #55489

Currently, PopupMenu does not respect the system status bar and bottom notch.

Wrapping PopupMenu with SafeArea would solve this problem.

Related Issues

Fixes #19954

Tests

I added the following tests:

  1. PopupMenu in AppBar does not overlap with the status bar
  2. Vertically long PopupMenu does not overlap with the status bar and bottom notch

However, new tests are failing.
I have created MediaQuery.data for mimicking status bar and bottom notch, but the results are same before&after wrapping PopupMenu with SafeArea.
Executing the same widgets on the emulator works as expected, so I think there's some problem with MediaQueryData or getting y offset of PopupMenu in my test code. (BTW, using MediaQueryData.viewInsets and MediaQueryData.viewPadding both give me the same result when representing the status bar... I think MediaQueryData.viewInsets may be correct.)

cc/ @Piinks

Checklist

Before you create this PR, confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I signed the CLA.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.
  • The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read Handling breaking changes.

@flutter-dashboard flutter-dashboard Bot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Aug 27, 2020
@HansMuller HansMuller requested a review from Piinks August 27, 2020 23:55
@Piinks Piinks added a: layout SystemChrome and Framework's Layout Issues a: quality A truly polished experience labels Sep 2, 2020
@Piinks

Piinks commented Sep 2, 2020

Copy link
Copy Markdown
Contributor

Hey @JuYeong0413 Can you share some sample code that reproduces the behavior in your images above? I have been looking at these tests and I suspect they may be failing because it is not quite working as expected. Thanks!

@JuYeong0413

JuYeong0413 commented Sep 3, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Ah I've put the reproducible code gist link in the design doc, but found out the link was broken. Sorry.
Here are the sample codes:

@Piinks

Piinks commented Sep 22, 2020

Copy link
Copy Markdown
Contributor

Sorry for the delay @JuYeong0413 I've been away. This is on my list to review today. :)

@JuYeong0413

JuYeong0413 commented Sep 23, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

It's okay, there's no rush. :) I know the Flutter team has a lot of works to do. Take your time.

@Piinks

Piinks commented Sep 24, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

I cannot sort out why the mocked-up media queries in your tests are not working as expected. Let me see if I can recruit a second opinion.

@Piinks

Piinks commented Sep 25, 2020

Copy link
Copy Markdown
Contributor

Hey @JuYeong0413 I forgot that the Popup menu is in a different overlay entry, so if you use MaterialApp.builder and place your media query there it should work. I found a test that you can use as an example to update your test:

flutter/packages/flutter/test/material/dialog_test.dart

Line 1551 in 3e8fb33

testWidgets('showDialog safe area', (WidgetTester tester) async {

Let me know how this works out for you!

@JuYeong0413

JuYeong0413 commented Oct 1, 2020

Copy link
Copy Markdown
Contributor Author

Thank you very much for your help @Piinks, using MaterialApp.builder worked! 😄
I've updated the tests. Please take a look when you have time.

Piinks
Piinks approved these changes Oct 7, 2020
View reviewed changes

@Piinks Piinks left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Flutter_LGTM
Thank you for the contribution! And for your patience in figuring out the tests! 🎉

@fluttergithubbot fluttergithubbot merged commit 2e419ff into flutter:master Oct 7, 2020
jonahwilliams pushed a commit that referenced this pull request Oct 7, 2020
Revert "Wrap PopupMenu with SafeArea to respect status bar (#64678)"
f047dcd 
This reverts commit 2e419ff.
@jonahwilliams jonahwilliams mentioned this pull request Oct 7, 2020
Merged
jonahwilliams pushed a commit that referenced this pull request Oct 7, 2020
Revert "Wrap PopupMenu with SafeArea to respect status bar (#64678)" (#…
5e4c86d 
…67566)

This reverts commit 2e419ff.
@Piinks Piinks mentioned this pull request Oct 7, 2020
Closed
13 tasks
Piinks added a commit that referenced this pull request Oct 7, 2020
@Piinks
Revert "Revert "Wrap PopupMenu with SafeArea to respect status bar (#…
516ab1a 
…64678)" (#67566)"

This reverts commit 5e4c86d.
@Piinks Piinks mentioned this pull request Oct 7, 2020
Merged
1 task
@JuYeong0413 JuYeong0413 deleted the popup-menu branch October 7, 2020 23:14
@ipcjs ipcjs mentioned this pull request Oct 27, 2020
Closed
@xu-baolin xu-baolin mentioned this pull request Nov 10, 2020
Merged
This was referenced Mar 11, 2021
Closed
Merged
@xu-baolin xu-baolin mentioned this pull request May 20, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Piinks Piinks Piinks approved these changes

Assignees

No one assigned

Labels

a: layout SystemChrome and Framework's Layout Issues a: quality A truly polished experience f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Tall popup menus go under the system status bar

4 participants

@JuYeong0413 @Piinks @googlebot @fluttergithubbot