From 943269c018356476908876de0a86bcd14dfb3388 Mon Sep 17 00:00:00 2001 From: Daniel DeMicco Date: Thu, 1 Feb 2018 11:54:35 -0800 Subject: [PATCH] Fix: use getBoundingClientRect in settings menu to prevent scrollbar when zoomed (#622) --- src/lib/viewers/media/Settings.js | 8 ++- .../viewers/media/__tests__/Settings-test.js | 55 +++++++++++++------ 2 files changed, 44 insertions(+), 19 deletions(-) diff --git a/src/lib/viewers/media/Settings.js b/src/lib/viewers/media/Settings.js index 3d3294530..5953d36af 100644 --- a/src/lib/viewers/media/Settings.js +++ b/src/lib/viewers/media/Settings.js @@ -295,13 +295,17 @@ class Settings extends EventEmitter { * @return {void} */ setMenuContainerDimensions(menu) { - const paddedHeight = menu.offsetHeight + SETTINGS_MENU_PADDING; + // Use getBoundingClientRect because when browsers are zoomed, the height/width + // Can be a fractional value, which gets stripped off with offsetHeight/offsetWidth + const { width, height } = menu.getBoundingClientRect(); + + const paddedHeight = height + 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`; + this.settingsEl.style.width = `${width + scrollPadding}px`; } /** diff --git a/src/lib/viewers/media/__tests__/Settings-test.js b/src/lib/viewers/media/__tests__/Settings-test.js index ff0ca8670..878f1c3bd 100644 --- a/src/lib/viewers/media/__tests__/Settings-test.js +++ b/src/lib/viewers/media/__tests__/Settings-test.js @@ -61,7 +61,10 @@ describe('lib/viewers/media/Settings', () => { describe('increaseSpeed()', () => { it('should increase speed one step', () => { sandbox.stub(settings, 'chooseOption'); - sandbox.stub(settings.cache, 'get').withArgs('media-speed').returns('1.25'); + sandbox + .stub(settings.cache, 'get') + .withArgs('media-speed') + .returns('1.25'); settings.increaseSpeed(); @@ -70,7 +73,10 @@ describe('lib/viewers/media/Settings', () => { it('should not increase speed after max', () => { sandbox.stub(settings, 'chooseOption'); - sandbox.stub(settings.cache, 'get').withArgs('media-speed').returns('2.0'); + sandbox + .stub(settings.cache, 'get') + .withArgs('media-speed') + .returns('2.0'); settings.increaseSpeed(); @@ -81,7 +87,10 @@ describe('lib/viewers/media/Settings', () => { describe('decreaseSpeed()', () => { it('should decrease speed one step', () => { sandbox.stub(settings, 'chooseOption'); - sandbox.stub(settings.cache, 'get').withArgs('media-speed').returns('1.5'); + sandbox + .stub(settings.cache, 'get') + .withArgs('media-speed') + .returns('1.5'); settings.decreaseSpeed(); @@ -93,7 +102,10 @@ describe('lib/viewers/media/Settings', () => { expect(speedOptions.length).to.be.above(0); sandbox.stub(settings, 'chooseOption'); - sandbox.stub(settings.cache, 'get').withArgs('media-speed').returns(speedOptions[0]); + sandbox + .stub(settings.cache, 'get') + .withArgs('media-speed') + .returns(speedOptions[0]); settings.decreaseSpeed(); @@ -112,8 +124,10 @@ describe('lib/viewers/media/Settings', () => { 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 + getBoundingClientRect: () => ({ + width: 0, + height: 500 // 210 is max height of settings menu + }) }; settings.setMenuContainerDimensions(menuEl); @@ -121,14 +135,18 @@ describe('lib/viewers/media/Settings', () => { }); it('should grow the height of the settingsEl to that of the sub-menu, with padding', () => { + const MENU_PADDING = 18; + const MENU_HEIGHT = 20; const menuEl = { - offsetWidth: 0, - offsetHeight: 18 + getBoundingClientRect: () => ({ + width: 0, + height: MENU_HEIGHT + }) }; settings.setMenuContainerDimensions(menuEl); - // Adds 18px to the offsetHeight of sum of child element's heights - expect(settings.settingsEl.style.height).to.equal('36px'); + // Adds 18px (padding) to the offsetHeight of sum of child element's heights + expect(settings.settingsEl.style.height).to.equal(`${MENU_HEIGHT + MENU_PADDING}px`); }); }); @@ -150,20 +168,18 @@ describe('lib/viewers/media/Settings', () => { sandbox.stub(settings, 'chooseOption'); const quality = 'sd'; const speed = '2.0'; - const autoplay = 'Enabled' + const autoplay = 'Enabled'; const getStub = sandbox.stub(settings.cache, 'get'); getStub.withArgs('media-quality').returns(quality); getStub.withArgs('media-speed').returns(speed); getStub.withArgs('media-autoplay').returns(autoplay); - settings.init(); expect(settings.chooseOption).to.be.calledWith('quality', quality); expect(settings.chooseOption).to.be.calledWith('speed', speed); expect(settings.chooseOption).to.be.calledWith('autoplay', autoplay); - }); }); @@ -736,7 +752,6 @@ describe('lib/viewers/media/Settings', () => { sandbox.stub(settings, 'handleSubtitleSelection'); sandbox.stub(settings.cache, 'set'); - settings.chooseOption('speed', 0.5, false); expect(settings.cache.set).to.not.be.called; @@ -853,7 +868,10 @@ describe('lib/viewers/media/Settings', () => { it('Should toggle on subtitles if they were on in the most recently viewed subtitled video', () => { sandbox.stub(settings, 'chooseOption'); sandbox.stub(settings, 'areSubtitlesOn').returns(false); - sandbox.stub(settings.cache, 'get').withArgs('media-subtitles').returns('2'); + sandbox + .stub(settings.cache, 'get') + .withArgs('media-subtitles') + .returns('2'); settings.loadSubtitles(['English', 'Russian', 'Spanish']); @@ -863,7 +881,10 @@ describe('lib/viewers/media/Settings', () => { it('Should not toggle on subtitles if they were off in the most recently viewed subtitled video', () => { sandbox.stub(settings, 'chooseOption'); sandbox.stub(settings, 'areSubtitlesOn').returns(false); - sandbox.stub(settings.cache, 'get').withArgs('media-subtitles').returns('-1'); + sandbox + .stub(settings.cache, 'get') + .withArgs('media-subtitles') + .returns('-1'); settings.loadSubtitles(['English', 'Russian', 'Spanish']); @@ -1107,7 +1128,7 @@ describe('lib/viewers/media/Settings', () => { sandbox.stub(settings.cache, 'get').returns('hd'); sandbox.stub(settings, 'chooseOption'); const CLASS_SETTINGS_QUALITY_MENU = 'bp-media-settings-menu-quality'; - const qualitySubMenu = settings.containerEl.querySelector(`.${CLASS_SETTINGS_QUALITY_MENU}`) + const qualitySubMenu = settings.containerEl.querySelector(`.${CLASS_SETTINGS_QUALITY_MENU}`); settings.enableHD();