[macOS] Make the default background color black. #36906

a-wallen · 2022-10-20T22:16:54Z

The background color of a macOS application defaults to the system's window default. This patch makes the default color black. More discussion is required to engineer an API that is suitable for clients that need to override this default behavior.

Fixes flutter/flutter#113975

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.

a-wallen · 2022-10-20T22:23:24Z

I considered setting the background color in FlutterSurfaceManager's _containingLayer, but it wouldn't have been testable, nor easily overridable.

engine/shell/platform/darwin/macos/framework/Source/FlutterSurfaceManager.mm

Lines 35 to 47 in f24ea1a

    
           - (instancetype)initWithLayer:(CALayer*)containingLayer contentTransform:(CATransform3D)transform { 
        
             self = [super init]; 
        
             if (self) { 
        
               _containingLayer = containingLayer; 
        
               _contentTransform = transform; 
        
               _contentLayer = [[CALayer alloc] init]; 
        
               [_containingLayer addSublayer:_contentLayer]; 
        
               _ioSurfaces[0] = [[FlutterIOSurfaceHolder alloc] init]; 
        
               _ioSurfaces[1] = [[FlutterIOSurfaceHolder alloc] init]; 
        
             } 
        
             return self; 
        
           }

@knopp do you have any suggestions for an API that could easily allow a user to override the behavior introduced in this patch? I think it would either have to be in the instantiation of the FlutterEngine or FlutterViewController.

a-wallen · 2022-10-21T03:31:09Z

shell/platform/darwin/macos/framework/Source/fixtures/flutter_desktop_test.dart

+
+@pragma('vm:entry-point')
+void backgroundTest() {
+  PlatformDispatcher.instance.views.first.render(SceneBuilder().build());


@cbracken we decided the test should look like

Picture _createSimplePicture() { Paint paint = Paint(); paint.color = Color(0xFFFF3A00); PictureRecorder baseRecorder = PictureRecorder(); Canvas canvas = Canvas(baseRecorder); canvas.drawRect(Rect.fromLTRB(0.0, 0.0, 1000.0, 1000.0), paint); return baseRecorder.endRecording(); } void main() async { window.render(SceneBuilder().build()); // 1. should look black final SceneBuilder builder = SceneBuilder(); builder.addPicture(Offset(1.0, 1.0), _createSimplePicture()); builder.pushOffset(1.0, 2.0); builder.pop(); // offset window.render(builder.build()); // 2. should have a red square window.render(SceneBuilder().build()); // 3. should be black again. }

but flutter/flutter#113785 is preventing step 3 from producing the correct output. Before we can add a test like the one above, flutter/flutter#113785 has to be fixed.

shell/platform/darwin/macos/framework/Source/FlutterView.h

knopp · 2022-10-24T19:58:26Z

shell/platform/darwin/macos/framework/Source/FlutterView.mm

+  return _backgroundColor ? _backgroundColor : [NSColor blackColor];
+}
+
+- (void)paintContainingLayerBackgroundColor {


It seems bit strange to call method paintContainingLayerBackgroundColor if it just sets a property on layer.

It also forces layer creation even if the view doesn't really need it (in case user doesn't want background color).

Actually, I think the view has layer created anyway (I'm not entirely sure what the hierarchy is right now). So maybe just override / forwardsetBackgroundColor on FlutterView to the layer.

The _containingLayer is provided to the FlutterSurfaceManager during initialization. Could you explain why forwarding setBackgroundColor to the Surface Manager would be better than at the FlutterView level?

engine/shell/platform/darwin/macos/framework/Source/FlutterSurfaceManager.mm

Line 26 in ecf2f85

CALayer* _containingLayer; // provided (parent layer)

Also, if setBackgroundColor propagates down to FlutterSurfaceManager, I think that we're going to have to provide overrides going all the way up to FlutterViewController since the FlutterView or FlutterSurfaceManager isn't exported as a part of FlutterMacOS. But please correct me if I'm not seeing something here.

I meant setting the background color on self.layer (where self is the FlutterView.) But not just do it in constructor, do it every time user calls [FlutterView setBackgroundColor]. Not doing it in FlutterSurfaceManager.

i.e.

-(void)setBackgroundColor:(NSColor*)color { self.layer.backgroundColor = color; }

And the in contructor ensure wantsLayer=YES and set layer color to black.

Yes. I think that will be fine. If user overrides the color in same CA transaction where the view is added to window the color will not be painted.

Sounds good.

Also, do you have any idea about how we can expose setBackgroundColor to a Flutter project? IIRC, FlutterView is not available via FlutterMacOS.

Good point. Maybe the setter needs to be on FlutterViewController then? At least for the time being. I'm not sure what the longer term plan with multi-view support is. That might result in exposing FlutterView in public api. cc @goderbauer

@knopp thanks for the help. This PR should have the changes requested if I understood you correctly.

I'm holding off on adding adding setBackgroundColor to FlutterViewController while awaiting @goderbauer's response. If we can expose FlutterView as a public api, it won't make sense to add it to the FlutterViewController.

I spoke with @goderbauer offline and it looks like we don't have concrete plans for exposing FlutterView in the near future.

To land this, we will have to propagate setBackgroundColor to the FlutterViewController.

knopp · 2022-10-24T20:01:19Z

shell/platform/darwin/macos/framework/Source/FlutterView.mm

@@ -84,6 +86,17 @@ - (void)reshaped {
                            }];
 }

+- (NSColor*)backgroundColor {


Instead of overriding getter wouldn't it be better to set blackColor as default value in constructor? That way user could override the color and possibly prevent additional layer creation (i.e. when background color is nil).

Also while I don't think the additional layer brings much overhead, in the end it is still another layer to composite. I'm probably missing bigger picture, but why can't the background just be painted by framework?

why can't the background just be painted by framework?

AFAIK, if the framework painted the background color, all platforms would have the background color as black. And, it wouldn't be an issue for macOS since this behavior is not present on Windows. I assume that the black background color is delegated to the platform for a reason, but I don't know why.

On windows is pretty just an artifact of Flutter currently not using direct composition and having redirection bitmap enabled on HWND (see WS_EX_NOREDIRECTIONBITMAP). This by itself is not ideal for peformance. I think if we ever move to direct composition (or whatever the current non deprecated way of compositing is in windows) the background color will be transparent as well.

Transparent makes sense to me, but I think that we have tests that will be affected if we do make the default background transparent.

AFAIK, if the framework painted the background color, all platforms would have the background color as black.

That is the intent. For consistency across platforms, the default background colour for Flutter apps is expected to be black [1]. As @knopp mentions, on some platforms we lucked out and got a black background by default due to our implementation, on others, we need to do work to make it happen. Pushing a default background colour layer is one way we could solve this.

If we went that route, there are a couple considerations:

As @knopp suggests, we really should allow this to be overridden by the user. If the user elects to set a transparent background colour, if the native view has a black background though, the user will still get black. So if we went this route, I think we'd want to make sure that the default native view has a transparent background so we pick up whatever is underneath it. In this case we're saying we need to set the native view background colour to transparent though, which is really no less work than setting it black, so not too sure adding the layer really wins us much.

This will add a layer to the layer tree. If that's done in a way that's detectable by the user it's possible it breaks some unit tests. AFAIK there's no way for user code to extract the layer tree from Scene or SceneBuilder so potentially it could be done in there (or on the C++ engine side of the flow). Note that this shouldn't be done on the framework side of things since this needs to work for pure dart:ui code too.

[1] Almost no one actually notices this because almost everyone has a widget tree where some widget deep in the tree covers the whole view and renders using some theme.

Pushing a default background colour layer is one way we could solve this. ... This will add a layer to the layer tree. If that's done in a way that's detectable by the user it's possible it breaks some unit tests.

This may actually be a positive:

We'd be able to test that the background color is black using dart:ui, golden tests, or Flutter driver's screenshot feature.

I'd be curious to see which unit tests are broken and why, this seems fishy to be affected by this

(1) applies regardless of how this is implemented, though not 100% how we'd test it using dart:ui unless we have an API to sample pixel colour.

If we support setting the bg colour we'd probably want a (separate?) test that layers in a non-flutter view that renders some colour like red, then layers a flutter view on top and verifies that if the user sets a transparent background, the red is visible through the flutter view.

Agreed on (2); since AFAIK there's no easy way to poke at the underlying layer-tree from the framework, I'd expect any breakage to be suspicious, but I've definitely seen weirder things break tests in the past.

though not 100% how we'd test it using dart:ui unless we have an API to sample pixel colour

I believe you can create an Image and then verify the pixels. See this or this.

I believe you can create an Image and then verify the pixels. See this or this.

Do these methods capture the whole screen or just the FlutterView?

From my understanding they both capture the scene's layer tree, which doesn't include the whole screen.

shell/platform/darwin/macos/framework/Source/FlutterView.mm

a-wallen · 2022-10-26T22:37:56Z

shell/platform/darwin/macos/framework/Source/FlutterViewController.mm

@@ -159,6 +159,8 @@ void Reset() {
 */
 @interface FlutterViewWrapper : NSView

+- (void)setBackgroundColor:(NSColor*)color;


This isn't pretty, but the FlutterViewController's content view is a FlutterViewWrapper and not a FlutterView. AFAIK, this is the only way to route the user's intent to the FlutterView.

cc @chunhtai, do you think there's a better way to do this?

The FlutterViewController should have direct access to FlutterView?

The FlutterViewController's view is set to a wrapped FlutterView

engine/shell/platform/darwin/macos/framework/Source/FlutterViewController.mm

Lines 379 to 380 in e39c4df

FlutterViewWrapper* wrapperView = [[FlutterViewWrapper alloc] initWithFlutterView:flutterView];

self.view = wrapperView;

but the FlutterViewWrapper's FlutterView is private.

engine/shell/platform/darwin/macos/framework/Source/FlutterViewController.mm

Lines 255 to 257 in e39c4df

@implementation FlutterViewWrapper {

FlutterView* _flutterView;

}

I tried calling setBackgroundColor on self.view (FlutterView) from FlutterViewController by casting self.view to a FlutterView. Oddly enough there was no runtime error, but the call took no effect until I routed the call through FlutterViewWrapper.

I meant the FlutterViewController should be able to access flutter view directly

engine/shell/platform/darwin/macos/framework/Source/FlutterViewController_Internal.h

Line 14 in 7d5df81

@property(nonatomic, readonly, nullable) FlutterView* flutterView;

For some reason, every time I reference this flutterView and call setBackgroundColor on it, the background color is never set. The current patch is the only way that I've found how to do it...

It's probably PEBKAC

a-wallen · 2022-10-26T22:48:12Z

shell/platform/darwin/macos/framework/Headers/FlutterViewController.h

+ *     // If the `FlutterView`'s color is not set via `FlutterViewController.setBackgroundColor`
+ *     // it's default will be black.
+ *     self.backgroundColor = NSColor.clear
+ *     flutterViewController.setBackgroundColor(NSColor.white)


@knopp please let me know if this API is satisfactory for your use case.

Example for fully transparent macOS window.

import Cocoa import FlutterMacOS class MainFlutterWindow: NSWindow { override func awakeFromNib() { let flutterViewController = FlutterViewController.init() self.backgroundColor = NSColor.clear flutterViewController.setBackgroundColor(NSColor.clear) let windowFrame = self.frame self.contentViewController = flutterViewController self.setFrame(windowFrame, display: true) RegisterGeneratedPlugins(registry: flutterViewController) super.awakeFromNib() } }

knopp

Just some nits.

shell/platform/darwin/macos/framework/Source/FlutterView.mm

shell/platform/darwin/macos/framework/Headers/FlutterViewController.h

chunhtai · 2022-10-27T23:17:56Z

shell/platform/darwin/macos/framework/Source/FlutterViewController.mm

@@ -415,6 +421,11 @@ - (void)setMouseTrackingMode:(FlutterMouseTrackingMode)mode {
  [self configureTrackingArea];
 }

+- (void)setBackgroundColor:(NSColor*)color {
+  _backgroundColor = color;


If you are going to cache it, you will also need to apply the background color during the loadView.

auto-submit · 2022-10-28T16:00:48Z

auto label is removed for flutter/engine, pr: 36906, due to - This commit is not mergeable and has conflicts. Please rebase your PR and fix all the conflicts.

chunhtai

LGTM, is there a plan to provide such API from the flutter side?

a-wallen · 2022-10-28T16:40:51Z

LGTM, is there a plan to provide such API from the flutter side?

Not that I'm aware of.

…ngine#36906)

## Description This PR fixes Ctrl+Tab shortcut on macOS. From https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/EventOverview/HandlingKeyEvents/HandlingKeyEvents.html#//apple_ref/doc/uid/10000060i-CH7-SW11, some key events, such as Ctrl+Tab, are notified by calling `performKeyEvent`: instead of `keyDown:`. The text input plugin already implement this method, this is why the shortcut works only when there is a TextField (TextInputPlugin is then activated), see 'Step 6' of flutter/flutter#122084 description. ## Implementation I had to implement `performKeyEvent:` on `FlutterViewWrapper` because when adding it to `FlutterView` it was not called (see this similar comment related to setBackgroundColor: #36906 (comment)). I also duplicated some code from FlutterTextInputPlugin.mm (the `KeyEquivalentMarker` definition) because `FlutterViewWrapper.performKeyEvent:` takes precedence over `FlutterTextInputPlugin.performKeyEvent:` so setting the marker is important to not break `FlutterTextInputPlugin.handleKeyEvent:`. ## Related Issue Fixes flutter/flutter#122084 ## Tests Adds 1 test.

flutter-dashboard bot added the platform-macos label Oct 20, 2022

a-wallen requested review from cbracken, knopp and Hixie and removed request for cbracken October 20, 2022 22:17

a-wallen commented Oct 21, 2022

View reviewed changes

knopp suggested changes Oct 24, 2022

View reviewed changes

a-wallen force-pushed the macos_black_background branch from 64ab675 to 76069ee Compare October 24, 2022 21:44

cbracken mentioned this pull request Oct 26, 2022

Background color on desktop is not black flutter/flutter#76082

Closed

a-wallen commented Oct 26, 2022

View reviewed changes

a-wallen force-pushed the macos_black_background branch 2 times, most recently from cc26e43 to 06baef9 Compare October 26, 2022 22:47

a-wallen commented Oct 26, 2022

View reviewed changes

knopp approved these changes Oct 27, 2022

View reviewed changes

shell/platform/darwin/macos/framework/Source/FlutterView.mm Show resolved Hide resolved

shell/platform/darwin/macos/framework/Headers/FlutterViewController.h Outdated Show resolved Hide resolved

cbracken approved these changes Oct 27, 2022

View reviewed changes

chunhtai reviewed Oct 27, 2022

View reviewed changes

a-wallen added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 28, 2022

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Oct 28, 2022

a-wallen force-pushed the macos_black_background branch from 79a0859 to 62dcd0c Compare October 28, 2022 16:11

chunhtai approved these changes Oct 28, 2022

View reviewed changes

a-wallen added 6 commits October 28, 2022 08:06

EXPERIMENT

831744b

Create failing unit test for black background on macos

18f13a0

Add black background default to FlutterView root layer

d8f3f65

Expose in FlutterView

4aaad35

Route setting background of FlutterView through FlutterViewController

0e94bfe

Add test to ensure background is overridable

25ddb12

a-wallen added 4 commits October 28, 2022 08:10

Remove SetWantsLayer from setBackgroundColor

c2f7828

Make backgroundColor a property on FVC

4b143f2

Set background when viewDidLoad

18ecb7f

Fix ios accident

4b459f6

a-wallen force-pushed the macos_black_background branch from 036061c to 4b459f6 Compare October 28, 2022 18:11

a-wallen added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 28, 2022

auto-submit bot merged commit da5fc92 into flutter:main Oct 28, 2022

engine-flutter-autoroll mentioned this pull request Oct 28, 2022

Roll Flutter Engine from d90e5b7372a3 to 774cb34d3ff5 (27 revisions) flutter/flutter#114258

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 28, 2022

da5fc92a3 [macOS] Make the default background color black. (flutter/e…

13dedc8

…ngine#36906)

zanderso mentioned this pull request Nov 14, 2022

New flake in FlutterEngineTest.BackgroundIsBlack flutter/flutter#115245

Closed

schwa423 pushed a commit to schwa423/engine that referenced this pull request Nov 16, 2022

[macOS] Make the default background color black. (flutter#36906)

808ad56

bleroux mentioned this pull request Jan 25, 2023

Flutter 3.7.0 renders a black background behind my app on macOS. flutter/flutter#119132

Closed

bleroux mentioned this pull request Mar 28, 2023

[macOS] Fix Ctrl+Tab is broken #40706

Merged

	FlutterViewWrapper* wrapperView = [[FlutterViewWrapper alloc] initWithFlutterView:flutterView];
	self.view = wrapperView;

	@implementation FlutterViewWrapper {
	FlutterView* _flutterView;
	}

[macOS] Make the default background color black. #36906

[macOS] Make the default background color black. #36906

Uh oh!

Conversation

a-wallen commented Oct 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Fixes flutter/flutter#113975

Pre-launch Checklist

Uh oh!

a-wallen commented Oct 20, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

knopp Oct 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

a-wallen Oct 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

a-wallen Oct 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

a-wallen Oct 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

knopp Oct 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbracken Oct 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbracken Oct 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

loic-sharma Oct 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

a-wallen Oct 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

a-wallen commented Oct 20, 2022 •

edited

Loading

knopp Oct 24, 2022 •

edited

Loading

a-wallen Oct 24, 2022 •

edited

Loading

a-wallen Oct 24, 2022 •

edited

Loading

a-wallen Oct 24, 2022 •

edited

Loading

knopp Oct 24, 2022 •

edited

Loading

cbracken Oct 26, 2022 •

edited

Loading

cbracken Oct 26, 2022 •

edited

Loading

loic-sharma Oct 27, 2022 •

edited

Loading

a-wallen Oct 26, 2022 •

edited

Loading

a-wallen Oct 26, 2022 •

edited

Loading