Skip to content
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

Re-land Android fullscreen support #27018

Merged
merged 35 commits into from
Jun 29, 2021
Merged

Conversation

Piinks
Copy link
Contributor

@Piinks Piinks commented Jun 28, 2021

This re-lands flutter/flutter#25785. The original PR was breaking tests internally due to a change in SDK support for navigation bar color.

While refactoring the SystemChromeStyle, I noticed that changing the navigation bar color was available at an earlier sdk than we were allowing, so I updated it to match native expectations. (changed from 26 to 21 where earliest available).

This is what broke tests internally on older devices. While it ends up giving the correct behavior for these older versions of Android, it made me realize why we were limiting this in the first place. The customer test has a perfect example:

SystemChrome.setSystemUIOverlayStyle(SystemUiOverlayStyle.dark.copyWith(
        systemNavigationBarColor: Colors.white,
        systemNavigationBarIconBrightness: Brightness.dark,
        statusBarIconBrightness: Brightness.dark));

The ability to change the navigation bar icon colors is only available at sdk 26. So if we were to allow the navigation bar color to be set, you could have white buttons on a white background like in this test. This is not desirable and is probably why it was this way to begin with. I have reverted the change to the way it was previously and documented this.

This is the diff from the original change: https://github.com/flutter/engine/pull/27018/files/ca8e05f563daa8707b25047b48a3c1ddf7ac9242..0f31f8bf4debcdf977d030871dca41ffe0792ee4


From Original PR (#25785)

This adds support for Android fullscreen modes.

Framework side: flutter/flutter#81303

Related Issues:

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on
    writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.
  • The reviewer has submitted any presubmit flakes in this PR using the engine presubmit flakes form before re-triggering the failure.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@Piinks Piinks changed the title WIP Re-land Android fullscreen support Re-land Android fullscreen support Jun 28, 2021
@Piinks Piinks requested review from blasten and gaaclarke June 28, 2021 21:49
@Piinks
Copy link
Contributor Author

Piinks commented Jun 28, 2021

Running through internal testing to make sure this is resolved now. :)

Copy link

@blasten blasten left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@gaaclarke gaaclarke left a comment

Choose a reason for hiding this comment

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

Requested changes are just the aforementioned comments.

@Piinks Piinks requested a review from gaaclarke June 29, 2021 00:27
@Piinks Piinks added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Jun 29, 2021
Copy link
Member

@gaaclarke gaaclarke left a comment

Choose a reason for hiding this comment

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

lgtm

@fluttergithubbot fluttergithubbot merged commit 53ac32f into flutter:master Jun 29, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 29, 2021
zanderso pushed a commit to flutter/flutter that referenced this pull request Jun 29, 2021
* ce489a5 Roll Skia from 0f1ac21185fe to b1590f15a32b (1 revision) (flutter/engine#27000)

* 92e24d8 Record raster end as soon as frame is submitted (flutter/engine#26970)

* 1f35cce Remove presubmit flake reporting instructions from issue template. (flutter/engine#26997)

* f5bc55e Roll Skia from b1590f15a32b to eef5b0e933e3 (2 revisions) (flutter/engine#27001)

* 8f57dfe Roll Fuchsia Mac SDK from z6trYeCMx... to R1ENSI-od... (flutter/engine#27002)

* 360d2d8 Surface frame number identifier through window (flutter/engine#26785)

* ed87f7c Roll Dart SDK from d2025958e351 to eca780278d49 (1 revision) (flutter/engine#27003)

* f558e2a Roll Fuchsia Linux SDK from TqViQQzJo... to 4udsaggtH... (flutter/engine#27006)

* 8ae3ff7 Fix Fuchsia build on Mac (flutter/engine#27007)

* b259ea6 Revert "[Engine] Support for Android Fullscreen Modes (#25785)" (flutter/engine#27014)

* 6e105c9 Roll Skia from eef5b0e933e3 to e5766b808045 (12 revisions) (flutter/engine#27011)

* cfea27e Allow fuchsia_archive to accept a cml file and cmx file (flutter/engine#27012)

* 9ef94a2 [web] Librarify paragraph/text files (flutter/engine#26888)

* cc2c361 Roll Dart SDK from eca780278d49 to bbd701b4ba76 (1 revision) (flutter/engine#27015)

* f6fcab4 Roll CanvasKit to 0.28.1 (flutter/engine#27013)

* db8ed9e Configure contexts to reduce shader variations. (flutter/engine#27016)

* 94876ed Roll Skia from e5766b808045 to 661abd0f8d64 (7 revisions) (flutter/engine#27020)

* 35e297d Update ci.yaml documentation link (flutter/engine#26922)

* 6a87e0b [web] fix actions flags in SemanticsTester (flutter/engine#26992)

* 53ac32f Re-land Android fullscreen support (flutter/engine#27018)

* 1417a82 [web] make analysis options delta of root options (flutter/engine#26991)

* b84ebe5 Roll Skia from 661abd0f8d64 to 1bddd42b9897 (3 revisions) (flutter/engine#27023)

* 4f3d88e [web] Librarify keyboard files (flutter/engine#26917)

* 9d09594 Roll Dart SDK from bbd701b4ba76 to fff3a3747a18 (1 revision) (flutter/engine#27024)
moffatman pushed a commit to moffatman/engine that referenced this pull request Aug 5, 2021
naudzghebre pushed a commit to naudzghebre/engine that referenced this pull request Sep 2, 2021
@flutter-dashboard
Copy link

This pull request was opened against a branch other than main. Since Flutter pull requests should not normally be opened against branches other than main, I have changed the base to main. If this was intended, you may modify the base back to master. See the Release Process for information about how other branches get updated.

Reviewers: Use caution before merging pull requests to branches other than main, unless this is an intentional hotfix/cherrypick.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes platform-android platform-ios waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land.
Projects
None yet
4 participants