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

[web] change status bar color based on SystemUiOverlayStyle #40599

Merged
merged 12 commits into from Apr 20, 2023

Conversation

maRci002
Copy link
Contributor

@maRci002 maRci002 commented Mar 24, 2023

Closes flutter/flutter#123365

In my example code I'm using SystemUiOverlayStyle.dark which has null statusBarColor by default (which can be changed via Change status bar color button) in this case we do not override browser's default status bar color.

Old behaviour New behaviour
web_output.mp4
web_new_output.mp4

In case of PWA the when statusBarColor is null it will use theme_color property from manifest.json (I don't know from where does flutter generate manifest.json).

The default status bar color for PWA is: "theme_color": "#0175C2",
https://github.com/flutter/flutter/blob/f4caee6efbc0b0094f3cee9e31a7486e3d030819/examples/api/web/manifest.json#L7

Old PWA behaviour New PWA behaviour
pwa_old_output.mp4
pwa_new_output.mp4

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.

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

@flutter-dashboard flutter-dashboard bot added the platform-web Code specifically for the web engine label Mar 24, 2023
Copy link
Member

@ditman ditman left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @maRci002!

I only have a couple of nitpicks about this. My biggest concern is that this might break some behavior unexpectedly, and this may need to warn users on where to find the functionality.

lib/web_ui/lib/src/engine/platform_dispatcher.dart Outdated Show resolved Hide resolved
lib/web_ui/lib/src/engine/platform_dispatcher.dart Outdated Show resolved Hide resolved
lib/web_ui/lib/src/engine/util.dart Outdated Show resolved Hide resolved
lib/web_ui/test/ui/system_ui_overlay_style_test.dart Outdated Show resolved Hide resolved
@maRci002 maRci002 requested a review from ditman April 11, 2023 14:24
@ditman
Copy link
Member

ditman commented Apr 11, 2023

Do I need any build before using fest test command?

@maRci002 now that you mention this, you probably do, but I'm not sure that our wasm build works in windows yet, I normally use a (g)Linux machine for development.

If you don't mind, I'm going to check out your branch and maybe push a fix to get it mergeable again.

@ditman
Copy link
Member

ditman commented Apr 12, 2023

I did a small change, the error in tests was causing because the wrong platform_dispatcher_test.dart was moved into the new directory (there's TWO tests named that in the repo, one inside canvaskit, the other inside engine). My apologies for the confusion, tests here should pass now!

I'll update the branch to the latest changes from main to see how it goes.

@maRci002
Copy link
Contributor Author

If you don't mind, I'm going to check out your branch and maybe push a fix to get it mergeable again.

Thanks for the modifications.

Once this PR is merged I'm going to update Stackoverflow answers, here are some of them:

It seems everyone is creating <meta name="theme-color" content="#000000"> a meta element as a workaround and it worked even if the engine created it's own when SystemChrome.setApplicationSwitcherDescription called, this means we don't break anything.

@ditman
Copy link
Member

ditman commented Apr 14, 2023

@maRci002 I gave this another thought, and I don't think we should tell people to "disregard this warning if...", so I made this PR backwards compatible.

The old API will continue to work until we fix the offending widgets in the framework. THEN we will be able to nag users that are using the platform message directly (or through some 3p widget) so they can fix it before it stops working altogether.

(I've also moved a random "ui" test that was testing the title to the "platform_dispatcher" directory that was created for this PR, and I think it makes more sense than before)

I hope you don't mind this change. Please, let me know if your use case is still working as intended, so I can land this!

(PS: the stack overflow updates are still good, so people know what's coming, but they should be done carefully because this is going to take a while to land in master, which it seems is what most people are using!)

@maRci002
Copy link
Contributor Author

(PS: the stack overflow updates are still good, so people know what's coming, but they should be done carefully because this is going to take a while to land in master, which it seems is what most people are using!)

I was going to mention that there are currently two ways to set the color of the browser's status and address bars: by setting the primaryColor property of the MaterialApp theme, or by setting the color property of MaterialApp. Another solution available on the master channel is to use SystemUiOverlayStyle. I will update the answer and specify the exact version that includes this update once it is available on the dev and stable channels.

The old API will continue to work until we fix the offending widgets in the framework.

Since the old API did not specify its behavior on the web, the behavior exhibited by the old API on the web was considered a bug

https://api.flutter.dev/flutter/widgets/Title/color.html

A color that the window manager should use to identify this app. Must be an opaque color (i.e. color.alpha must be 255 (0xFF)), and must not be null.

https://api.flutter.dev/flutter/widgets/WidgetsApp/color.html

The primary color to use for the application in the operating system interface.

For example, on Android this is the color used for the application in the application switcher.

https://api.flutter.dev/flutter/services/ApplicationSwitcherDescription-class.html

Specifies a description of the application that is pertinent to the embedder's application switcher (also known as "recent tasks") user interface.

Used by SystemChrome.setApplicationSwitcherDescription.

https://api.flutter.dev/flutter/services/SystemChrome/setApplicationSwitcherDescription.html

Specifies the description of the current state of the application as it pertains to the application switcher (also known as "recent tasks").

Any part of the description that is unsupported on the current platform will be ignored.

https://github.com/flutter/flutter/blob/f72efea43c3013323d1b95cff571f3c1caa37583/packages/flutter/lib/src/material/app.dart#L967-L974

used on old Android OSes to color the app bar in Android's switcher UI.

I am very pleased with these changes. The RenderView._updateSystemChrome function calls SystemChrome.setSystemUIOverlayStyle, which triggers a microtask to update the system UI overlay style so it is always superior to the SystemChrome.setApplicationSwitcherDescription which is triggered by Title widget.

@ditman
Copy link
Member

ditman commented Apr 18, 2023

Let's land this version then, and then we can experiment to remove the color changing support from the Title widget, maybe even remove it first from the flutter/flutter widget itself!

Copy link
Member

@ditman ditman left a comment

Choose a reason for hiding this comment

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

LGTM! Let's go with this new platform method first!

@ditman ditman added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 18, 2023
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Apr 19, 2023
@auto-submit
Copy link
Contributor

auto-submit bot commented Apr 19, 2023

auto label is removed for flutter/engine, pr: 40599, due to This PR has not met approval requirements for merging. You have project association NONE and need 1 more review(s) in order to merge this PR.

  • Merge guidelines: You need at least one approved review if you are already a MEMBER or two member reviews if you are not a MEMBER before re-applying the autosubmit label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

@auto-submit
Copy link
Contributor

auto-submit bot commented Apr 19, 2023

auto label is removed for flutter/engine, pr: 40599, due to Validations Fail.

@mdebbar mdebbar added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 20, 2023
@auto-submit auto-submit bot merged commit 1a25b7a into flutter:main Apr 20, 2023
33 checks passed
zanderso pushed a commit to flutter/flutter that referenced this pull request Apr 21, 2023
…125271)

flutter/engine@2db85cb...122c3b3

2023-04-21 737941+loic-sharma@users.noreply.github.com [Windows] Don't
block raster thread until v-blank (flutter/engine#41231)
2023-04-21 chillers@google.com Manual roll skia to d5b4acfb4
(flutter/engine#41378)
2023-04-21 magder@google.com Run mac unopt arm builds with arm toolchain
(flutter/engine#41353)
2023-04-20 zanderso@users.noreply.github.com Revert "re-land "Migrate
mac_host_engine to engine v2 builds." (#41233)"" (flutter/engine#41380)
2023-04-20 skia-flutter-autoroll@skia.org Roll Dart SDK from
df05e451b79a to 50b96abe9f6f (1 revision) (flutter/engine#41379)
2023-04-20 109111084+yaakovschectman@users.noreply.github.com Move
ownership of `AccessibilityBridgeWindows` to `FlutterWindowsView`
(flutter/engine#41308)
2023-04-20 skia-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from
AoPEjX8Xfq1v0h4kx... to PqBDstaESE_l77k1e... (flutter/engine#41373)
2023-04-20 godofredoc@google.com Revert "Upload windows arm artifacts to
production bucket." (flutter/engine#41372)
2023-04-20 godofredoc@google.com re-land "Migrate mac_host_engine to
engine v2 builds." (#41233)" (flutter/engine#41323)
2023-04-20 godofredoc@google.com Upload windows arm artifacts to
production bucket. (flutter/engine#41324)
2023-04-20 jason-simmons@users.noreply.github.com [Impeller] Change the
default color format for the GLES backend to RGBA (flutter/engine#41342)
2023-04-20 maRci002@users.noreply.github.com [web] change status bar
color based on SystemUiOverlayStyle (flutter/engine#40599)
2023-04-20 skia-flutter-autoroll@skia.org Roll Fuchsia Mac SDK from
OcPCdaE17MAihaCrD... to 4OrPF9lzqCKGwBLRh... (flutter/engine#41367)
2023-04-20 skia-flutter-autoroll@skia.org Roll Skia from fc09f9b2fb27 to
f4609aa2eaba (1 revision) (flutter/engine#41366)
2023-04-20 skia-flutter-autoroll@skia.org Roll Dart SDK from
7d165bd0bb5e to df05e451b79a (2 revisions) (flutter/engine#41365)
2023-04-20 skia-flutter-autoroll@skia.org Roll Skia from 80c38970791e to
fc09f9b2fb27 (1 revision) (flutter/engine#41362)
2023-04-20 skia-flutter-autoroll@skia.org Roll Skia from c50081c62219 to
80c38970791e (2 revisions) (flutter/engine#41360)
2023-04-20 skia-flutter-autoroll@skia.org Roll Skia from c21e7df194c3 to
c50081c62219 (11 revisions) (flutter/engine#41358)
2023-04-20 skia-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from
Tun7i4VLz6ncx8JJJ... to AoPEjX8Xfq1v0h4kx... (flutter/engine#41357)
2023-04-20 skia-flutter-autoroll@skia.org Roll Dart SDK from
88a3b66b50d6 to 7d165bd0bb5e (1 revision) (flutter/engine#41356)
2023-04-20 jason-simmons@users.noreply.github.com Manual Skia roll from
ad90b6bd4760 to c21e7df194c3 (flutter/engine#41341)
2023-04-20 jonahwilliams@google.com [impeller] convert src over to src
for solid color (flutter/engine#41351)

Also rolling transitive DEPS:
  fuchsia/sdk/core/linux-amd64 from Tun7i4VLz6nc to PqBDstaESE_l
  fuchsia/sdk/core/mac-amd64 from OcPCdaE17MAi to 4OrPF9lzqCKG

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 chinmaygarde@google.com,rmistry@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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App platform-web Code specifically for the web engine
Projects
None yet
4 participants