Forwards iOS dark mode trait to the Flutter framework (#34441).

[matthew-carroll]

Forwards iOS dark mode trait to the Flutter framework (#34441).

This change forwards iOS's dark mode setting to the Flutter framework from the appropriate ViewController methods:
https://developer.apple.com/documentation/appkit/supporting_dark_mode_in_your_interface?language=objc

MaterialApp will respect this setting on iOS. I don't believe CupertinoApp is setup yet to respect brightness.

This change does not include official color support for iOS's dark mode, nor does this PR resolve the issue of rendering correctly in the app switcher.

[matthew-carroll]

@xster @dnfield please let me know if you can think of a reasonable way to test this.

[dnfield]

For tests, take a look at https://github.com/flutter/engine/blob/master/shell/platform/darwin/ios/framework/Source/FlutterEngineTest.mm

You could mock out the engine for the VC, mock out the settingschannel for the mock engine, and verify that calling the methods you're changing here resulted in a message on the settings channel.

[matthew-carroll]

@dnfield can we do any better in terms of test value? Seems like that kind of test fakes most of the important machinery to validate one of the simplest and least likely to change details, a channel message....seems very low value from a regression-prevention standpoint.

[dnfield]

My thought is that it would test that a channel message is sent on these events, which is all the new behavior this PR introduces.

An end to end test of that would be even better, but harder to create IMO. You could put another integration test in ios_add2app but not sure how you would modify the system setting in a test.

[justinmc]

LGTM. I can't think of a better way to test than what's been suggested. Not sure about changing system settings in an e2e test.

[LongCatIsLooong]

When the user changes the system appearance, the system automatically asks each window and view to redraw itself. During this process, the system calls several well-known methods for both macOS and iOS, listed in the following table, to update your content. The system updates the trait environment before calling these methods, so if you make all of your appearance-sensitive changes in them, your app updates itself correctly.

It sounds like reporting trait collection changes in one of the methods in the list should suffice? I wonder if we can reduce it down to viewDidLoad and traitCollectionDidChange.

EDIT:
@matthew-carroll
traitCollectionDidChange(_:) should be called every single time the surrounding UITraitCollection changes, per its name and documentation. It also gets called if I change color vibrancy while the app is in background and then foreground the app.

According to this article, traitCollectionDidChange(_:) will no longer pick up the initial traitCollection value on iOS 13 so I guess it's safer to manually pick up the initial value in viewDidLoad or a similar method.

[dnfield]

In terms of preventing regressions, it would prevent someone from inadvertently removing one of the places where we need to invoke this channel.

Part of the idea here is that channel message passing should also be well tested (but shouldn't be the responsibility of this PR), and the framework's handling of such a message should also be well tested (but also isn't the job of this PR). It's also not our responsibility to test whether iOS calls the methods it documents that it will call in the linked documentation.

As I said, an integration test would also be good but is probably pretty challenging to get quite right in this case - I think at the least we'd end up mocking out the system preference change by just directly messaging the ViewController.

[matthew-carroll]

@dnfield @justinmc I'll go ahead and try to write up that test as described.

@LongCatIsLooong I'm open to removing excessive calls. Can you demonstrate that we don't need all of those methods? It may be the case that they are often called in succession, but the question is, are there ever moments where only 1 of them is called? If so, then we would need that call overridden. Unfortunately, I don't know nearly enough about ViewController lifecycles. CC @dnfield if you happen to know about this.

[matthew-carroll]

@dnfield do we have any docs for how to run those tests locally? If not, would you mind providing instructions here?

[dnfield]

@gaaclarke should know, but I think you just run the shell scripts in that folder....

[matthew-carroll]

@dnfield I'm not seeing a shell script in that directory, or its parent directory. Can you point me to the script you're thinking of?

[dnfield]

https://github.com/flutter/engine/blob/master/testing/ios/IosUnitTests/build_and_run_tests.sh

[cbracken]

@matthew-carroll do you plan to continue with this PR?

[matthew-carroll]

Yep. I need to figure out how to write unit tests for iOS first.

[matthew-carroll]

I finally got around to adding tests for this.

@dnfield can you take another look?

[dnfield]

LGTM. Thanks for the test. Fyi @LongCatIsLooong

[chinmaygarde]

This patch causes LUCI failures on the iOS builder. https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket.appspot.com/8905650479855166128/+/steps/build_ios_debug_sim/0/stdout