Merge pull request #554 from bitmovin/release/v3.48.0

[RELEASE] v3.48.0

CHANGELOG.md

@@ -4,6 +4,11 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](http://keepachangelog.com/)
 and this project adheres to [Semantic Versioning](http://semver.org/).
 
+## [3.48.0]
+
+### Fixed
+- When more than one UI with spatial navigation is managed by the UI manager, all UIs would handle key events, instead of only the active one.
+
 ## [3.47.0]
 
 ### Changed
@@ -849,6 +854,7 @@ Version 2.0 of the UI framework is built for player 7.1. If absolutely necessary
 ## 1.0.0 (2017-02-03)
 - First release
 
+[3.48.0]: https://github.com/bitmovin/bitmovin-player-ui/compare/v3.47.0...develop
 [3.47.0]: https://github.com/bitmovin/bitmovin-player-ui/compare/v3.46.0...v3.47.0
 [3.46.0]: https://github.com/bitmovin/bitmovin-player-ui/compare/v3.45.0...v3.46.0
 [3.45.0]: https://github.com/bitmovin/bitmovin-player-ui/compare/v3.44.0...v3.45.0

package.json

@@ -1,6 +1,6 @@
 {
   "name": "bitmovin-player-ui",
-  "version": "3.47.0",
+  "version": "3.48.0",
   "description": "Bitmovin Player UI Framework",
   "main": "./dist/js/framework/main.js",
   "types": "./dist/js/framework/main.d.ts",

spec/spatialnavigation/spatialnavigation.spec.ts

@@ -68,13 +68,15 @@ describe('SpatialNavigation', () => {
 
   describe('handleKeyEvent', () => {
       it('should call handle navigation on active group on key event', () => {
+        rootNavigationContainer.show();
         const rootHandleNavigationSpy = jest.spyOn(rootNavigationGroup, 'handleNavigation');
         spatialNavigation['handleKeyEvent'](new KeyboardEvent('keydown', { key:'Up', keyCode: 38 } as any));
 
         expect(rootHandleNavigationSpy).toHaveBeenCalledWith(Direction.UP);
       });
 
     it('should call handle action on active group on key event', () => {
+      rootNavigationContainer.show();
       const rootHandleActionSpy = jest.spyOn(rootNavigationGroup, 'handleAction');
       spatialNavigation['handleKeyEvent'](new KeyboardEvent('keydown', { key:'Escape', keyCode: 27 } as any));
 

spec/uimanager.spec.ts

@@ -1,14 +1,21 @@
-import { InternalUIConfig, PlayerWrapper, UIManager } from '../src/ts/uimanager';
+import {
+  InternalUIConfig,
+  PlayerWrapper,
+  UIManager,
+  UIVariant,
+} from '../src/ts/uimanager';
 import { PlayerAPI } from 'bitmovin-player';
 import { MockHelper, TestingPlayerAPI } from './helper/MockHelper';
 import { MobileV3PlayerEvent } from '../src/ts/mobilev3playerapi';
+import { UIContainer } from '../src/ts/components/uicontainer';
+import { Container } from '../src/ts/components/container';
 
 jest.mock('../src/ts/dom');
 
 // This just simulates a Class that can be wrapped by our PlayerWrapper.
 // To enable this simple class structure we need a lot of any casts in the tests.
 class A {
-  private value: object = undefined;
+  private value?: object = undefined;
 
   get a() {
     return this.value;
@@ -66,6 +73,47 @@ describe('UIManager', () => {
       });
     });
   });
+
+  describe('switchToUiVariant', () => {
+    let firstUi: UIVariant, secondUI: UIVariant, defaultUI: UIVariant;
+    let playerMock: TestingPlayerAPI;
+
+    beforeEach(() => {
+      playerMock = MockHelper.getPlayerMock();
+      firstUi = {
+        ui: new UIContainer({ components: [new Container({})]}),
+        condition: (context) => context.isPlaying,
+      };
+      secondUI = {
+        ui: new UIContainer({ components: [new Container({})]}),
+        condition: (context) => context.isAd,
+      }
+      defaultUI = {
+        ui: new UIContainer({ components: [new Container({})]}),
+      }
+
+    });
+
+    it('should mark invisible UIs as hidden', () => {
+      new UIManager(playerMock, [firstUi, secondUI, defaultUI]);
+
+      expect(firstUi.ui.isHidden()).toBeTruthy()
+      expect(secondUI.ui.isHidden()).toBeTruthy()
+      expect(defaultUI.ui.isHidden()).toBeFalsy()
+    });
+
+    it('should switch to the corresponding ui when a play event is fired', () => {
+      new UIManager(playerMock, [firstUi, secondUI, defaultUI]);
+
+      (playerMock.isPlaying as jest.Mock).mockReturnValue(true);
+      playerMock.eventEmitter.firePlayEvent();
+
+      expect(firstUi.ui.isHidden()).toBeFalsy()
+      expect(secondUI.ui.isHidden()).toBeTruthy()
+      expect(defaultUI.ui.isHidden()).toBeTruthy()
+    });
+  });
+
   describe('mobile v3 handling', () => {
     let playerMock: TestingPlayerAPI;
     beforeEach(() => {

src/ts/spatialnavigation/spatialnavigation.ts

@@ -121,14 +121,19 @@ export class SpatialNavigation {
   private handleKeyEvent = (e: KeyboardEvent): void => {
     const event: Direction | Action | undefined = this.keyMap[getKeyCode(e)];
 
+    const active = this.getActiveNavigationGroup();
+    if (!active || !active.container || active.container.isHidden() || active.container.isDisabled()) {
+      return;
+    }
+
     if (isDirection(event)) {
-      this.getActiveNavigationGroup().handleNavigation(event);
+      active.handleNavigation(event);
 
       e.preventDefault();
       e.stopPropagation();
     }
     if (isAction(event)) {
-      this.getActiveNavigationGroup().handleAction(event);
+      active.handleAction(event);
 
       e.preventDefault();
       e.stopPropagation();

src/ts/uimanager.ts

@@ -384,40 +384,39 @@ export class UIManager {
     let uiVariantIndex = this.uiVariants.indexOf(uiVariant);
 
     const nextUi: InternalUIInstanceManager = this.uiInstanceManagers[uiVariantIndex];
-    let uiVariantChanged = false;
-
     // Determine if the UI variant is changing
-    if (nextUi !== this.currentUi) {
-      uiVariantChanged = true;
+    // Only if the UI variant is changing, we need to do some stuff. Else we just leave everything as-is.
+    if (nextUi === this.currentUi) {
+      return;
       // console.log('switched from ', this.currentUi ? this.currentUi.getUI() : 'none',
       //   ' to ', nextUi ? nextUi.getUI() : 'none');
     }
 
-    // Only if the UI variant is changing, we need to do some stuff. Else we just leave everything as-is.
-    if (uiVariantChanged) {
-      // Hide the currently active UI variant
-      if (this.currentUi) {
-        this.currentUi.getUI().hide();
-      }
-
-      // Assign the new UI variant as current UI
-      this.currentUi = nextUi;
-
-      // When we switch to a different UI instance, there's some additional stuff to manage. If we do not switch
-      // to an instance, we're done here.
-      if (this.currentUi != null) {
-        // Add the UI to the DOM (and configure it) the first time it is selected
-        if (!this.currentUi.isConfigured()) {
-          this.addUi(this.currentUi);
-        }
+    // Hide the currently active UI variant
+    if (this.currentUi) {
+      this.currentUi.getUI().hide();
+    }
 
-        if (onShow) {
-          onShow();
-        }
+    // Assign the new UI variant as current UI
+    this.currentUi = nextUi;
 
-        this.currentUi.getUI().show();
+    // When we switch to a different UI instance, there's some additional stuff to manage. If we do not switch
+    // to an instance, we're done here.
+    if (this.currentUi == null) {
+      return;
+    }
+    // Add the UI to the DOM (and configure it) the first time it is selected
+    if (!this.currentUi.isConfigured()) {
+      this.addUi(this.currentUi);
+      // ensure that the internal state is ready for the upcoming show call
+      if (!this.currentUi.getUI().isHidden()) {
+        this.currentUi.getUI().hide();
       }
     }
+    if (onShow) {
+      onShow();
+    }
+    this.currentUi.getUI().show();
   }
 
   /**
@@ -451,9 +450,12 @@ export class UIManager {
     // Select new UI variant
     // If no variant condition is fulfilled, we switch to *no* UI
     for (let uiVariant of this.uiVariants) {
-      if (uiVariant.condition == null || uiVariant.condition(switchingContext) === true) {
+      const matchesCondition = uiVariant.condition == null || uiVariant.condition(switchingContext) === true;
+      if (nextUiVariant == null && matchesCondition) {
         nextUiVariant = uiVariant;
-        break;
+      } else {
+        // hide all UIs besides the one which should be active
+        uiVariant.ui.hide();
       }
     }