Hide chrome until an app is mounting or mounted to avoid FOC on chrom…

…eless apps (elastic#65036) (elastic#65299)

* hidden chrome until an application is mounted

* emits currentAppId$ before executing mount handler

* address comments

src/core/public/application/application_service.tsx

@@ -26,6 +26,7 @@ import { InjectedMetadataSetup } from '../injected_metadata';
 import { HttpSetup, HttpStart } from '../http';
 import { OverlayStart } from '../overlays';
 import { ContextSetup, IContextContainer } from '../context';
+import { PluginOpaqueId } from '../plugins';
 import { AppRouter } from './ui';
 import { Capabilities, CapabilitiesService } from './capabilities';
 import {
@@ -34,7 +35,6 @@ import {
   AppLeaveHandler,
   AppMount,
   AppMountDeprecated,
-  AppMounter,
   AppNavLinkStatus,
   AppStatus,
   AppUpdatableFields,
@@ -145,6 +145,25 @@ export class ApplicationService {
       this.subscriptions.push(subscription);
     };
 
+    const wrapMount = (plugin: PluginOpaqueId, app: App<any>): AppMount => {
+      let handler: AppMount;
+      if (isAppMountDeprecated(app.mount)) {
+        handler = this.mountContext!.createHandler(plugin, app.mount);
+        if (process.env.NODE_ENV === 'development') {
+          // eslint-disable-next-line no-console
+          console.warn(
+            `App [${app.id}] is using deprecated mount context. Use core.getStartServices() instead.`
+          );
+        }
+      } else {
+        handler = app.mount;
+      }
+      return async params => {
+        this.currentAppId$.next(app.id);
+        return handler(params);
+      };
+    };
+
     return {
       registerMountContext: this.mountContext!.registerContext,
       register: (plugin, app: App<any>) => {
@@ -162,24 +181,6 @@ export class ApplicationService {
           throw new Error('Cannot register an application route that includes HTTP base path');
         }
 
-        let handler: AppMount;
-
-        if (isAppMountDeprecated(app.mount)) {
-          handler = this.mountContext!.createHandler(plugin, app.mount);
-          // eslint-disable-next-line no-console
-          console.warn(
-            `App [${app.id}] is using deprecated mount context. Use core.getStartServices() instead.`
-          );
-        } else {
-          handler = app.mount;
-        }
-
-        const mount: AppMounter = async params => {
-          const unmount = await handler(params);
-          this.currentAppId$.next(app.id);
-          return unmount;
-        };
-
         const { updater$, ...appProps } = app;
         this.apps.set(app.id, {
           ...appProps,
@@ -193,7 +194,7 @@ export class ApplicationService {
         this.mounters.set(app.id, {
           appRoute: app.appRoute!,
           appBasePath: basePath.prepend(app.appRoute!),
-          mount,
+          mount: wrapMount(plugin, app),
           unmountBeforeMounting: false,
         });
       },

src/core/public/application/integration_tests/application_service.test.tsx

@@ -17,6 +17,7 @@
  * under the License.
  */
 
+import { take } from 'rxjs/operators';
 import { createRenderer } from './utils';
 import { createMemoryHistory, MemoryHistory } from 'history';
 import { ApplicationService } from '../application_service';
@@ -56,6 +57,69 @@ describe('ApplicationService', () => {
     service = new ApplicationService();
   });
 
+  describe('navigating to apps', () => {
+    describe('using history.push', () => {
+      it('emits currentAppId$ before mounting the app', async () => {
+        const { register } = service.setup(setupDeps);
+
+        let resolveMount: () => void;
+        const promise = new Promise(resolve => {
+          resolveMount = resolve;
+        });
+
+        register(Symbol(), {
+          id: 'app1',
+          title: 'App1',
+          mount: async ({}: AppMountParameters) => {
+            await promise;
+            return () => undefined;
+          },
+        });
+
+        const { currentAppId$, getComponent } = await service.start(startDeps);
+        update = createRenderer(getComponent());
+
+        await navigate('/app/app1');
+
+        expect(await currentAppId$.pipe(take(1)).toPromise()).toEqual('app1');
+
+        resolveMount!();
+
+        expect(await currentAppId$.pipe(take(1)).toPromise()).toEqual('app1');
+      });
+    });
+
+    describe('using navigateToApp', () => {
+      it('emits currentAppId$ before mounting the app', async () => {
+        const { register } = service.setup(setupDeps);
+
+        let resolveMount: () => void;
+        const promise = new Promise(resolve => {
+          resolveMount = resolve;
+        });
+
+        register(Symbol(), {
+          id: 'app1',
+          title: 'App1',
+          mount: async ({}: AppMountParameters) => {
+            await promise;
+            return () => undefined;
+          },
+        });
+
+        const { navigateToApp, currentAppId$ } = await service.start(startDeps);
+
+        await navigateToApp('app1');
+
+        expect(await currentAppId$.pipe(take(1)).toPromise()).toEqual('app1');
+
+        resolveMount!();
+
+        expect(await currentAppId$.pipe(take(1)).toPromise()).toEqual('app1');
+      });
+    });
+  });
+
   describe('leaving an application that registered an app leave handler', () => {
     it('navigates to the new app if action is default', async () => {
       startDeps.overlays.openConfirm.mockResolvedValue(true);

src/core/public/chrome/chrome_service.test.ts

@@ -29,7 +29,6 @@ import { notificationServiceMock } from '../notifications/notifications_service.
 import { docLinksServiceMock } from '../doc_links/doc_links_service.mock';
 import { ChromeService } from './chrome_service';
 import { App } from '../application';
-import { uiSettingsServiceMock } from '../ui_settings/ui_settings_service.mock';
 
 class FakeApp implements App {
   public title = `${this.id} App`;
@@ -52,7 +51,6 @@ function defaultStartDeps(availableApps?: App[]) {
     http: httpServiceMock.createStartContract(),
     injectedMetadata: injectedMetadataServiceMock.createStartContract(),
     notifications: notificationServiceMock.createStartContract(),
-    uiSettings: uiSettingsServiceMock.createStartContract(),
   };
 
   if (availableApps) {
@@ -163,7 +161,7 @@ describe('start', () => {
   });
 
   describe('visibility', () => {
-    it('updates/emits the visibility', async () => {
+    it('emits false when no application is mounted', async () => {
       const { chrome, service } = await start();
       const promise = chrome
         .getIsVisible$()
@@ -177,33 +175,37 @@ describe('start', () => {
 
       await expect(promise).resolves.toMatchInlineSnapshot(`
         Array [
-          true,
-          true,
           false,
-          true,
+          false,
+          false,
+          false,
         ]
       `);
     });
 
-    it('always emits false if embed query string is preset when set up', async () => {
+    it('emits false until manually overridden when in embed mode', async () => {
       window.history.pushState(undefined, '', '#/home?a=b&embed=true');
+      const startDeps = defaultStartDeps([new FakeApp('alpha')]);
+      const { navigateToApp } = startDeps.application;
+      const { chrome, service } = await start({ startDeps });
 
-      const { chrome, service } = await start();
       const promise = chrome
         .getIsVisible$()
         .pipe(toArray())
         .toPromise();
 
+      await navigateToApp('alpha');
+
       chrome.setIsVisible(true);
       chrome.setIsVisible(false);
-      chrome.setIsVisible(true);
+
       service.stop();
 
       await expect(promise).resolves.toMatchInlineSnapshot(`
         Array [
           false,
           false,
-          false,
+          true,
           false,
         ]
       `);
@@ -228,7 +230,7 @@ describe('start', () => {
 
       await expect(promise).resolves.toMatchInlineSnapshot(`
         Array [
-          true,
+          false,
           true,
           false,
           true,
@@ -245,13 +247,13 @@ describe('start', () => {
         .pipe(toArray())
         .toPromise();
 
-      navigateToApp('alpha');
+      await navigateToApp('alpha');
       chrome.setIsVisible(true);
       service.stop();
 
       await expect(promise).resolves.toMatchInlineSnapshot(`
         Array [
-          true,
+          false,
           false,
           false,
         ]

src/core/public/chrome/chrome_service.tsx

@@ -38,7 +38,6 @@ import { LoadingIndicator, Header } from './ui';
 import { DocLinksStart } from '../doc_links';
 import { ChromeHelpExtensionMenuLink } from './ui/header/header_help_menu';
 import { KIBANA_ASK_ELASTIC_LINK } from './constants';
-import { IUiSettingsClient } from '../ui_settings';
 export { ChromeNavControls, ChromeRecentlyAccessed, ChromeDocTitle };
 
 const IS_LOCKED_KEY = 'core.chrome.isLocked';
@@ -85,14 +84,12 @@ interface StartDeps {
   http: HttpStart;
   injectedMetadata: InjectedMetadataStart;
   notifications: NotificationsStart;
-  uiSettings: IUiSettingsClient;
 }
 
 /** @internal */
 export class ChromeService {
   private isVisible$!: Observable<boolean>;
-  private appHidden$!: Observable<boolean>;
-  private toggleHidden$!: BehaviorSubject<boolean>;
+  private isForceHidden$!: BehaviorSubject<boolean>;
   private readonly stop$ = new ReplaySubject(1);
   private readonly navControls = new NavControlsService();
   private readonly navLinks = new NavLinksService();
@@ -111,13 +108,12 @@ export class ChromeService {
   private initVisibility(application: StartDeps['application']) {
     // Start off the chrome service hidden if "embed" is in the hash query string.
     const isEmbedded = 'embed' in parse(location.hash.slice(1), true).query;
+    this.isForceHidden$ = new BehaviorSubject(isEmbedded);
 
-    this.toggleHidden$ = new BehaviorSubject(isEmbedded);
-    this.appHidden$ = merge(
-      // Default the app being hidden to the same value initial value as the chrome visibility
-      // in case the application service has not emitted an app ID yet, since we want to trigger
-      // combineLatest below regardless of having an application value yet.
-      of(isEmbedded),
+    const appHidden$ = merge(
+      // For the isVisible$ logic, having no mounted app is equivalent to having a hidden app
+      // in the sense that the chrome UI should not be displayed until a non-chromeless app is mounting or mounted
+      of(true),
       application.currentAppId$.pipe(
         flatMap(appId =>
           application.applications$.pipe(
@@ -128,8 +124,8 @@ export class ChromeService {
         )
       )
     );
-    this.isVisible$ = combineLatest([this.appHidden$, this.toggleHidden$]).pipe(
-      map(([appHidden, toggleHidden]) => !(appHidden || toggleHidden)),
+    this.isVisible$ = combineLatest([appHidden$, this.isForceHidden$]).pipe(
+      map(([appHidden, forceHidden]) => !appHidden && !forceHidden),
       takeUntil(this.stop$)
     );
   }
@@ -140,7 +136,6 @@ export class ChromeService {
     http,
     injectedMetadata,
     notifications,
-    uiSettings,
   }: StartDeps): Promise<InternalChromeStart> {
     this.initVisibility(application);
 
@@ -221,7 +216,7 @@ export class ChromeService {
 
       getIsVisible$: () => this.isVisible$,
 
-      setIsVisible: (isVisible: boolean) => this.toggleHidden$.next(!isVisible),
+      setIsVisible: (isVisible: boolean) => this.isForceHidden$.next(!isVisible),
 
       getApplicationClasses$: () =>
         applicationClasses$.pipe(

src/core/public/core_system.ts

@@ -240,7 +240,6 @@ export class CoreSystem {
         http,
         injectedMetadata,
         notifications,
-        uiSettings,
       });
 
       application.registerMountContext(this.coreContext.coreId, 'core', () => ({