Fix Status Bar Icon Brightness #162297
Fix Status Bar Icon Brightness #162297
Conversation
github-actions
bot
added
framework
jesswrd
changed the title
| expect(pressed, isTrue); | ||
| }); | ||
|
|
||
| testWidgets('The status bar icons are dark by default', (WidgetTester tester) async { |
There was a problem hiding this comment.
Should this test be "the status bar icons are the opposite brightness of the system?
(related to me not following how ThemeMode works in relation to brightness)
There was a problem hiding this comment.
In Material app, I don't use the brightness of the system at all to determine the brightness of the icon. This is because the default app's brightness (from flutter create or flutter create -e) does not change when the system is in dark mode or light mode.
|
|
||
| final bool useDarkTheme = | ||
| mode == ThemeMode.dark || | ||
| (mode == ThemeMode.system && platformBrightness == ui.Brightness.dark); |
There was a problem hiding this comment.
I am trying to square this code with the review description. I think the default ThemeMode is system. Which means that material should also have the ternary logic about brightness.
That said I trust your evaluation so where am I misunderstanding the code?
There was a problem hiding this comment.
In Material, the system's brightness does not reflect the brightness of the Flutter default app. This means when I turn the phone to dark mode, my app does not use a dark theme—it stays the same. I realize this code block has logic for using the dark theme. It's not triggering when I set the system to dark mode.
If I add ternary logic (similar to my CupertinoApp changes), I see the same bug from the issue where you cannot see the status bar icons. This bug does not appear in CupertinoApp (with ternary logic).
Also, I updated my description above. Instead of default I just put "made to be." I meant to the user, it looks like the default.
There was a problem hiding this comment.
Also that added line is there by accident from a previous commit. Deleting that now.
There was a problem hiding this comment.
Does this mean that the bug is that material does not respect the phones brightness by default and that it should. That is what the documentation I linked seems to imply.
@Piinks who on your team might know the history of brightness and its default relationship with system brightness?
There was a problem hiding this comment.
I am less familiar with ThemeMode, cc @QuncCccccc for material and @MitchellGoodwin for cupertino. They may have some thoughts.
jesswrd
force-pushed
the
i160305-statusbar-light-bug
branch
from
36cd41b to
3541a70
Compare
|
|
||
| final bool useDarkTheme = | ||
| mode == ThemeMode.dark || | ||
| (mode == ThemeMode.system && platformBrightness == ui.Brightness.dark); |
There was a problem hiding this comment.
I am less familiar with ThemeMode, cc @QuncCccccc for material and @MitchellGoodwin for cupertino. They may have some thoughts.
jesswrd
force-pushed
the
i160305-statusbar-light-bug
branch
from
6dcee54 to
6367516
Compare
MitchellGoodwin
left a comment
MitchellGoodwin
left a comment
There was a problem hiding this comment.
LGTM! Looks like the analyzer is unhappy about the sample code in system_chrome.dart.
|
To fix the analyzer complaint, we can add on top of the file (below imports). |
There was a problem hiding this comment.
I strongly oppose this PR because the changes have completely broken existing configuration methods. For example, the following code no longer works as intended:
void main() { WidgetsFlutterBinding.ensureInitialized(); SystemChrome.setSystemUIOverlayStyle( SystemUiOverlayStyle( statusBarIconBrightness: WidgetsBinding.instance!.platformDispatcher.platformBrightness == Brightness.light ? Brightness.dark : Brightness.light, ), ); runApp(const MainApp()); }
This modification has caused regression in functionality that was previously working correctly.
There was a problem hiding this comment.
Please direct comments towards the open issue around this behavior. Merged PRs are difficult to track discussion on.
6 participants
On Android devices, the statusBar is now by default transparent starting android API 35 here (due to automatic opt-in to edge to edge mode here), which is making the statusBar icons difficult to see. We decided to make the change in Cupertino/Material layer as opposed to the embedder layer becasue we cannot make assumptions on the defaults when we don't know what kind of Widgets the Flutter dev will be using.
In MaterialApp, the
statusBarIconBrightnessis made to be dark because the system's brightness (dark mode or light mode) does not affect the brightness of the default app. This means when I turn on dark mode on my phone, the app does not use a dark theme—it uses the same light theme.In CupertinoApp, the
statusBarIconBrightnessis made to be opposite of the system's brightness (dark mode or light mode) because the system does affect the brightness of the default app. The system and the default app have the same brightness. This means when I turn on dark mode on my phone, the app also uses a dark theme.Fixes #160305
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.