From cdb627e1b3377150c10f90ca0bc584d4635cffa0 Mon Sep 17 00:00:00 2001
From: Justin Holdstock <justinholdstock91@gmail.com>
Date: Tue, 23 Jan 2018 16:00:58 -0800
Subject: [PATCH] Fix settings menu title cut off by scrollbar (#599)

COXP-4461
---
 src/lib/viewers/media/Settings.js             | 16 ++++++----
 .../viewers/media/__tests__/Settings-test.js  | 31 +++++++++++++++++++
 2 files changed, 41 insertions(+), 6 deletions(-)

diff --git a/src/lib/viewers/media/Settings.js b/src/lib/viewers/media/Settings.js
index 99878287e..3d3294530 100644
--- a/src/lib/viewers/media/Settings.js
+++ b/src/lib/viewers/media/Settings.js
@@ -22,6 +22,9 @@ const MEDIA_SPEEDS = ['0.5', '1.0', '1.25', '1.5', '2.0'];
 const MEDIA_QUALITY_SD = 'sd';
 const MEDIA_QUALITY_HD = 'hd';
 const MEDIA_QUALITY_AUTO = 'auto';
+const SETTINGS_MENU_PADDING = 18;
+const SETTINGS_MENU_PADDING_SCROLL = 32;
+const SETTINGS_MENU_MAX_HEIGHT = 210;
 
 const SETTINGS_TEMPLATE = `<div class="bp-media-settings">
     <div class="bp-media-settings-menu-main bp-media-settings-menu" role="menu">
@@ -292,12 +295,13 @@ class Settings extends EventEmitter {
      * @return {void}
      */
     setMenuContainerDimensions(menu) {
-        // NOTE: need to explicitly set the dimensions in order to get css transitions. width=auto doesn't work with css transitions
-        this.settingsEl.style.width = `${menu.offsetWidth + 18}px`;
-        // height = n * $item-height + 2 * $padding (see Settings.scss) + 2 * border (see Settings.scss)
-        // where n is the number of displayed items in the menu
-        const sumHeight = [].reduce.call(menu.children, (sum, child) => sum + child.offsetHeight, 0);
-        this.settingsEl.style.height = `${sumHeight + 18}px`;
+        const paddedHeight = menu.offsetHeight + SETTINGS_MENU_PADDING;
+        this.settingsEl.style.height = `${paddedHeight}px`;
+
+        // If the menu grows tall enough to require scrolling, take into account scroll bar width.
+        const scrollPadding =
+            paddedHeight >= SETTINGS_MENU_MAX_HEIGHT ? SETTINGS_MENU_PADDING_SCROLL : SETTINGS_MENU_PADDING;
+        this.settingsEl.style.width = `${menu.offsetWidth + scrollPadding}px`;
     }
 
     /**
diff --git a/src/lib/viewers/media/__tests__/Settings-test.js b/src/lib/viewers/media/__tests__/Settings-test.js
index b1ab9c6e8..ff0ca8670 100644
--- a/src/lib/viewers/media/__tests__/Settings-test.js
+++ b/src/lib/viewers/media/__tests__/Settings-test.js
@@ -101,6 +101,37 @@ describe('lib/viewers/media/Settings', () => {
         });
     });
 
+    describe('setMenuContainerDimensions', () => {
+        it('should add padding to settingsEl based on menu contents and additional padding', () => {
+            const menuEl = document.createElement('div');
+            menuEl.appendChild(document.createElement('span'));
+            settings.setMenuContainerDimensions(menuEl);
+
+            expect(settings.settingsEl.style.width).to.equal('18px');
+        });
+
+        it('should add extra padding to settingsEl based on menu contents that require scroll bar', () => {
+            const menuEl = {
+                offsetWidth: 0,
+                offsetHeight: 500 // 210 is max height of settings menu
+            };
+            settings.setMenuContainerDimensions(menuEl);
+
+            expect(settings.settingsEl.style.width).to.equal('32px');
+        });
+
+        it('should grow the height of the settingsEl to that of the sub-menu, with padding', () => {
+            const menuEl = {
+                offsetWidth: 0,
+                offsetHeight: 18
+            };
+            settings.setMenuContainerDimensions(menuEl);
+
+            // Adds 18px to the offsetHeight of sum of child element's heights
+            expect(settings.settingsEl.style.height).to.equal('36px');
+        });
+    });
+
     describe('destroy()', () => {
         it('should remove event listeners on settings element and document', () => {
             sandbox.stub(settings.settingsEl, 'removeEventListener');