fix(FEC-8163): UI doesn't remove its listeners when components unmount (

#234) Add bindMethod util. Bind any listener context in the constructor.
kaltura · May 6, 2018 · 13f9078 · 13f9078
1 parent 90c30d6
commit 13f9078
Show file tree

Hide file tree

Showing 9 changed files with 173 additions and 52 deletions.
diff --git a/src/components/fullscreen/fullscreen.js b/src/components/fullscreen/fullscreen.js
@@ -8,6 +8,7 @@ import {actions} from '../../reducers/shell';
 import {actions as fullscreenActions} from '../../reducers/fullscreen';
 import BaseComponent from '../base';
 import {default as Icon, IconType} from '../icon';
+import {bindMethod} from '../../utils/bind-method';
 
 /**
  * mapping state to props
@@ -31,6 +32,7 @@ const mapStateToProps = state => ({
    */
 class FullscreenControl extends BaseComponent {
   _targetDiv: ?HTMLElement;
+  fullscreenChangeHandler: Function;
 
   /**
    * Creates an instance of FullscreenControl.
@@ -39,6 +41,7 @@ class FullscreenControl extends BaseComponent {
    */
   constructor(obj: Object) {
     super({name: 'Fullscreen', player: obj.player});
+    this.fullscreenChangeHandler = bindMethod(this, this.fullscreenChangeHandler);
   }
 
   /**
@@ -52,21 +55,34 @@ class FullscreenControl extends BaseComponent {
   }
 
   /**
-   * after component mounted, set up event listerners to window fullscreen state change
+   * after component mounted, set up event listeners to window fullscreen state change
    *
    * @returns {void}
    * @memberof FullscreenControl
    */
-  componentDidMount() {
-    document.addEventListener('webkitfullscreenchange', () => this.fullscreenChangeHandler());
-    document.addEventListener('mozfullscreenchange', () => this.fullscreenChangeHandler());
-    document.addEventListener('fullscreenchange', () => this.fullscreenChangeHandler());
-    document.addEventListener('MSFullscreenChange', () => this.fullscreenChangeHandler());
+  componentDidMount(): void {
+    document.addEventListener('webkitfullscreenchange', this.fullscreenChangeHandler);
+    document.addEventListener('mozfullscreenchange', this.fullscreenChangeHandler);
+    document.addEventListener('fullscreenchange', this.fullscreenChangeHandler);
+    document.addEventListener('MSFullscreenChange', this.fullscreenChangeHandler);
     this.player.addEventListener(this.player.Event.REQUESTED_ENTER_FULLSCREEN, () => this.enterFullscreen());
     this.player.addEventListener(this.player.Event.REQUESTED_EXIT_FULLSCREEN, () => this.exitFullscreen());
     this.handleIosFullscreen();
   }
 
+  /**
+   * before component unmounted, remove event listeners
+   *
+   * @returns {void}
+   * @memberof FullscreenControl
+   */
+  componentWillUnmount(): void {
+    document.removeEventListener('webkitfullscreenchange', this.fullscreenChangeHandler);
+    document.removeEventListener('mozfullscreenchange', this.fullscreenChangeHandler);
+    document.removeEventListener('fullscreenchange', this.fullscreenChangeHandler);
+    document.removeEventListener('MSFullscreenChange', this.fullscreenChangeHandler);
+  }
+
   /**
    * Handle iOS full screen changes
    *
@@ -81,12 +97,8 @@ class FullscreenControl extends BaseComponent {
        */
       const attachIosFullscreenListeners = () => {
         this.player.removeEventListener(this.player.Event.SOURCE_SELECTED, attachIosFullscreenListeners);
-        this.player.getVideoElement().addEventListener('webkitbeginfullscreen', () => {
-          this.fullscreenEnterHandler();
-        });
-        this.player.getVideoElement().addEventListener('webkitendfullscreen', () => {
-          this.fullscreenExitHandler();
-        });
+        this.player.getVideoElement().addEventListener('webkitbeginfullscreen', () => this.fullscreenEnterHandler());
+        this.player.getVideoElement().addEventListener('webkitendfullscreen', () => this.fullscreenExitHandler());
       };
       this.player.addEventListener(this.player.Event.SOURCE_SELECTED, attachIosFullscreenListeners);
     }
@@ -99,7 +111,7 @@ class FullscreenControl extends BaseComponent {
    * @memberof FullscreenControl
    */
   fullscreenChangeHandler(): void {
-    let isFullscreen = typeof document.fullscreenElement !== 'undefined' && Boolean(document.fullscreenElement) ||
+    const isFullscreen = typeof document.fullscreenElement !== 'undefined' && Boolean(document.fullscreenElement) ||
       typeof document.webkitFullscreenElement !== 'undefined' && Boolean(document.webkitFullscreenElement) ||
       typeof document.mozFullScreenElement !== 'undefined' && Boolean(document.mozFullScreenElement) ||
       typeof document.msFullscreenElement !== 'undefined' && Boolean(document.msFullscreenElement);

diff --git a/src/components/language/language.js b/src/components/language/language.js
@@ -12,6 +12,7 @@ import {default as Icon, IconType} from '../icon';
 import {CVAAOverlay} from '../cvaa-overlay';
 import Portal from 'preact-portal';
 import {KeyMap} from "../../utils/key-map";
+import {bindMethod} from '../../utils/bind-method';
 
 /**
  * mapping state to props
@@ -35,6 +36,7 @@ const mapStateToProps = state => ({
    */
 class LanguageControl extends BaseComponent {
   state: Object;
+  handleClickOutside: Function;
   _controlLanguageElement: any;
   _portal: any;
 
@@ -45,6 +47,7 @@ class LanguageControl extends BaseComponent {
    */
   constructor(obj: Object) {
     super({name: 'LanguageControl', player: obj.player});
+    this.handleClickOutside = bindMethod(this, this.handleClickOutside);
   }
 
   /**
@@ -64,7 +67,7 @@ class LanguageControl extends BaseComponent {
    * @memberof LanguageControl
    */
   componentDidMount() {
-    document.addEventListener('click', this.handleClickOutside.bind(this), true);
+    document.addEventListener('click', this.handleClickOutside, true);
   }
 
   /**
@@ -74,7 +77,7 @@ class LanguageControl extends BaseComponent {
    * @memberof LanguageControl
    */
   componentWillUnmount() {
-    document.removeEventListener('click', this.handleClickOutside.bind(this), true);
+    document.removeEventListener('click', this.handleClickOutside, true);
   }
 
   /**

diff --git a/src/components/menu/menu.js b/src/components/menu/menu.js
@@ -3,7 +3,8 @@ import style from '../../styles/style.scss';
 import {h, Component} from 'preact';
 import {default as Icon, IconType} from '../icon';
 import {connect} from 'preact-redux';
-import {KeyMap} from "../../utils/key-map";
+import {KeyMap} from '../../utils/key-map';
+import {bindMethod} from '../../utils/bind-method';
 
 /**
  * mapping state to props
@@ -28,8 +29,20 @@ const mapStateToProps = state => ({
    * @extends {Component}
    */
 class Menu extends Component {
-  _menuElement: any;
   state: Object;
+  handleClickOutside: Function;
+  _menuElement: any;
+
+  /**
+   * Creates an instance of Menu.
+   *
+   * @constructor
+   * @memberof Menu
+   */
+  constructor() {
+    super();
+    this.handleClickOutside = bindMethod(this, this.handleClickOutside);
+  }
 
   /**
    * before component mounted, set initial state of the menu position
@@ -46,7 +59,7 @@ class Menu extends Component {
    * @memberof Menu
    */
   componentDidMount() {
-    document.addEventListener('click', this.handleClickOutside.bind(this), true);
+    document.addEventListener('click', this.handleClickOutside, true);
     if (!this.props.isMobile) {
       this.setState({position: this.getPosition()});
     }
@@ -59,7 +72,7 @@ class Menu extends Component {
    * @memberof Menu
    */
   componentWillUnmount() {
-    document.removeEventListener('click', this.handleClickOutside.bind(this));
+    document.removeEventListener('click', this.handleClickOutside);
   }
 
   /**

diff --git a/src/components/seekbar/seekbar.js b/src/components/seekbar/seekbar.js
@@ -7,6 +7,7 @@ import {connect} from "preact-redux";
 import {bindActions} from "../../utils/bind-actions";
 import {actions} from "../../reducers/shell";
 import {KEYBOARD_DEFAULT_SEEK_JUMP} from '../keyboard/keyboard';
+import {bindMethod} from '../../utils/bind-method';
 
 /**
  * mapping state to props
@@ -27,18 +28,28 @@ const mapStateToProps = state => ({
    */
 class SeekBarControl extends Component {
   state: Object;
+  onPlayerMouseUp: Function;
   _seekBarElement: HTMLElement;
   _framePreviewElement: HTMLElement;
   _timeBubbleElement: HTMLElement;
   _movex: number;
 
+  /**
+   * Creates an instance of SeekBarControl.
+   * @memberof SeekBarControl
+   */
+  constructor() {
+    super();
+    this.onPlayerMouseUp = bindMethod(this, this.onPlayerMouseUp);
+  }
+
   /**
    * before component mounted, set initial state
    *
    * @returns {void}
    * @memberof SeekBarControl
    */
-  componentWillMount() {
+  componentWillMount(): void {
     this.setState({virtualTime: 0});
   }
 
@@ -48,14 +59,20 @@ class SeekBarControl extends Component {
    * @returns {void}
    * @memberof SeekBarControl
    */
-  componentDidMount() {
-    document.addEventListener('mouseup', (e: Event) => {
-      this.onPlayerMouseUp(e);
-    });
+  componentDidMount(): void {
+    document.addEventListener('mouseup', this.onPlayerMouseUp);
+    document.addEventListener('mousemove', this.onPlayerMouseUp);
+  }
 
-    document.addEventListener('mousemove', (e: Event) => {
-      this.onPlayerMouseMove(e);
-    });
+  /**
+   * before component unmounted, remove event listeners
+   *
+   * @returns {void}
+   * @memberof SeekBarControl
+   */
+  componentWillUnmount(): void {
+    document.removeEventListener('mouseup', this.onPlayerMouseUp);
+    document.removeEventListener('mousemove', this.onPlayerMouseUp);
   }
 
   /**

diff --git a/src/components/settings/settings.js b/src/components/settings/settings.js
@@ -9,6 +9,7 @@ import BaseComponent from '../base';
 import {SmartContainer} from '../smart-container';
 import {SmartContainerItem} from '../smart-container/smart-container-item';
 import {default as Icon, IconType} from '../icon';
+import {bindMethod} from '../../utils/bind-method';
 
 /**
  * mapping state to props
@@ -31,6 +32,7 @@ const mapStateToProps = state => ({
    */
 class SettingsControl extends BaseComponent {
   state: Object;
+  handleClickOutside: Function;
   _controlSettingsElement: any;
 
   /**
@@ -40,6 +42,7 @@ class SettingsControl extends BaseComponent {
    */
   constructor(obj: Object) {
     super({name: 'Settings', player: obj.player});
+    this.handleClickOutside = bindMethod(this, this.handleClickOutside);
   }
 
   /**
@@ -59,7 +62,7 @@ class SettingsControl extends BaseComponent {
    * @memberof SettingsControl
    */
   componentDidMount() {
-    document.addEventListener('click', this.handleClickOutside.bind(this), true);
+    document.addEventListener('click', this.handleClickOutside, true);
   }
 
   /**
@@ -69,7 +72,7 @@ class SettingsControl extends BaseComponent {
    * @memberof SettingsControl
    */
   componentWillUnmount() {
-    document.removeEventListener('click', this.handleClickOutside.bind(this));
+    document.removeEventListener('click', this.handleClickOutside, true);
   }
 
   /**

diff --git a/src/components/shell/shell.js b/src/components/shell/shell.js
@@ -5,7 +5,8 @@ import BaseComponent from '../base';
 import {connect} from 'preact-redux';
 import {bindActions} from '../../utils/bind-actions';
 import {actions} from '../../reducers/shell';
-import {KeyMap} from "../../utils/key-map";
+import {KeyMap} from '../../utils/key-map';
+import {bindMethod} from '../../utils/bind-method';
 
 /**
  * mapping state to props
@@ -52,6 +53,7 @@ class Shell extends BaseComponent {
   hoverTimeout: ?number;
   _fallbackToMutedAutoPlayMode: boolean;
   _environmentClasses: Array<string>;
+  _onWindowResize: Function;
 
   /**
    * Creates an instance of Shell.
@@ -60,6 +62,7 @@ class Shell extends BaseComponent {
    */
   constructor(obj: Object) {
     super({name: 'Shell', player: obj.player});
+    this._onWindowResize = bindMethod(this, this._onWindowResize);
     this._fallbackToMutedAutoPlayMode = false;
     this.player.addEventListener(this.player.Event.FALLBACK_TO_MUTED_AUTOPLAY, () => {
       this._fallbackToMutedAutoPlayMode = true
@@ -172,22 +175,34 @@ class Shell extends BaseComponent {
    */
   componentDidMount() {
     this.props.updateIsMobile(!!this.player.env.device.type || this.props.forceTouchUI);
-    if (document.body) {
-      this.props.updateDocumentWidth(document.body.clientWidth);
-    }
+    this._onWindowResize();
+    window.addEventListener('resize', this._onWindowResize);
+  }
+
+  /**
+   * window resize handler
+   *
+   * @returns {void}
+   * @memberof Shell
+   */
+  _onWindowResize(): void {
     const playerContainer = document.getElementById(this.props.targetId);
     if (playerContainer) {
       this.props.updatePlayerClientRect(playerContainer.getBoundingClientRect());
     }
-    window.addEventListener('resize', () => {
-      const playerContainer = document.getElementById(this.props.targetId);
-      if (playerContainer) {
-        this.props.updatePlayerClientRect(playerContainer.getBoundingClientRect());
-      }
-      if (document.body) {
-        this.props.updateDocumentWidth(document.body.clientWidth);
-      }
-    });
+    if (document.body) {
+      this.props.updateDocumentWidth(document.body.clientWidth);
+    }
+  }
+
+  /**
+   * before component mounted, remove event listeners
+   *
+   * @returns {void}
+   * @memberof Shell
+   */
+  componentWillUnmount(): void {
+    window.removeEventListener('resize', this._onWindowResize);
   }
 
   /**