From 3f97896dd5b312ef5add153375bc288c20113964 Mon Sep 17 00:00:00 2001 From: Conrad Chan Date: Mon, 5 Nov 2018 14:06:13 -0800 Subject: [PATCH 1/6] Fix: Add tabindex to preview container --- src/lib/Fullscreen.js | 10 ++++++++ src/lib/__tests__/Fullscreen-test.js | 34 +++++++++++++++++++++++++++- src/lib/shell.html | 2 +- 3 files changed, 44 insertions(+), 2 deletions(-) diff --git a/src/lib/Fullscreen.js b/src/lib/Fullscreen.js index 7a4d01b35..04bbac83a 100644 --- a/src/lib/Fullscreen.js +++ b/src/lib/Fullscreen.js @@ -99,6 +99,16 @@ class Fullscreen extends EventEmitter { if (this.isSupported()) { if (this.isFullscreen()) { + // Focus on the fullscreen element so keyboard + // events are triggered without an extra click + if (el) { + let element = el; + if (el instanceof Event) { + element = el.target; + } + + element.focus(); + } enter = true; } } else if (!this.isFullscreen(el)) { diff --git a/src/lib/__tests__/Fullscreen-test.js b/src/lib/__tests__/Fullscreen-test.js index c6182a6a7..88016cba2 100644 --- a/src/lib/__tests__/Fullscreen-test.js +++ b/src/lib/__tests__/Fullscreen-test.js @@ -43,7 +43,10 @@ describe('lib/Fullscreen', () => { sandbox.stub(fullscreen, 'isFullscreen').returns(true); sandbox.stub(fullscreen, 'emit'); - fullscreen.fullscreenchangeHandler({}); + const element = document.createElement('div'); + sandbox.stub(element, 'focus'); + + fullscreen.fullscreenchangeHandler(element); expect(fullscreen.emit).to.have.been.calledWith('enter'); }); @@ -88,6 +91,35 @@ describe('lib/Fullscreen', () => { window.document.dispatchEvent(event); expect(spy).to.be.called.once; }); + + it('should focus element from passed in event', () => { + sandbox.stub(fullscreen, 'isSupported').returns(true); + sandbox.stub(fullscreen, 'isFullscreen').returns(true); + sandbox.stub(fullscreen, 'emit'); + + const target = document.createElement('div'); + sandbox.stub(target, 'focus'); + const event = { target }; + event.__proto__ = Event.prototype; + + fullscreen.fullscreenchangeHandler(event); + + expect(fullscreen.emit).to.have.been.calledWith('enter'); + expect(target.focus.called).to.be.true; + }); + + it('should focus element from passed element', () => { + sandbox.stub(fullscreen, 'isSupported').returns(true); + sandbox.stub(fullscreen, 'isFullscreen').returns(true); + sandbox.stub(fullscreen, 'emit'); + + const element = document.createElement('div'); + sandbox.stub(element, 'focus'); + fullscreen.fullscreenchangeHandler(element); + + expect(fullscreen.emit).to.have.been.calledWith('enter'); + expect(element.focus.called).to.be.true; + }); }); describe('toggle()', () => { diff --git a/src/lib/shell.html b/src/lib/shell.html index 6f8d6229d..44711cf67 100644 --- a/src/lib/shell.html +++ b/src/lib/shell.html @@ -33,7 +33,7 @@ -
+
From edc740084c3f68db32064088bf9229f004402733 Mon Sep 17 00:00:00 2001 From: Conrad Chan Date: Mon, 5 Nov 2018 14:20:54 -0800 Subject: [PATCH 2/6] Chore: Refactor for unit test --- src/lib/Fullscreen.js | 7 +++---- src/lib/__tests__/Fullscreen-test.js | 1 - 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/src/lib/Fullscreen.js b/src/lib/Fullscreen.js index 04bbac83a..0bdd8b83b 100644 --- a/src/lib/Fullscreen.js +++ b/src/lib/Fullscreen.js @@ -102,10 +102,9 @@ class Fullscreen extends EventEmitter { // Focus on the fullscreen element so keyboard // events are triggered without an extra click if (el) { - let element = el; - if (el instanceof Event) { - element = el.target; - } + // If el has target property, then it is an Event + // otherwise it is a HTMLElement + const element = el.target || el; element.focus(); } diff --git a/src/lib/__tests__/Fullscreen-test.js b/src/lib/__tests__/Fullscreen-test.js index 88016cba2..a97b0f386 100644 --- a/src/lib/__tests__/Fullscreen-test.js +++ b/src/lib/__tests__/Fullscreen-test.js @@ -100,7 +100,6 @@ describe('lib/Fullscreen', () => { const target = document.createElement('div'); sandbox.stub(target, 'focus'); const event = { target }; - event.__proto__ = Event.prototype; fullscreen.fullscreenchangeHandler(event); From 5183d4436961d703a395c298e844b374c590fb3d Mon Sep 17 00:00:00 2001 From: Conrad Chan Date: Mon, 5 Nov 2018 15:39:52 -0800 Subject: [PATCH 3/6] Chore: addressing PR comments --- src/lib/Fullscreen.js | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/lib/Fullscreen.js b/src/lib/Fullscreen.js index 0bdd8b83b..4326eebcc 100644 --- a/src/lib/Fullscreen.js +++ b/src/lib/Fullscreen.js @@ -101,13 +101,11 @@ class Fullscreen extends EventEmitter { if (this.isFullscreen()) { // Focus on the fullscreen element so keyboard // events are triggered without an extra click - if (el) { - // If el has target property, then it is an Event - // otherwise it is a HTMLElement - const element = el.target || el; + // If el has target property, then it is an Event + // otherwise it is a HTMLElement + const element = el.target || el; - element.focus(); - } + element.focus(); enter = true; } } else if (!this.isFullscreen(el)) { From b8e40863b220e807aa398707c4a83db9c95a5602 Mon Sep 17 00:00:00 2001 From: Conrad Chan Date: Mon, 5 Nov 2018 17:09:40 -0800 Subject: [PATCH 4/6] Chore: addressing PR comments --- src/lib/Fullscreen.js | 38 +++++++++++++++++----------- src/lib/__tests__/Fullscreen-test.js | 19 +++++--------- 2 files changed, 29 insertions(+), 28 deletions(-) diff --git a/src/lib/Fullscreen.js b/src/lib/Fullscreen.js index 4326eebcc..0f9cfa242 100644 --- a/src/lib/Fullscreen.js +++ b/src/lib/Fullscreen.js @@ -91,34 +91,42 @@ class Fullscreen extends EventEmitter { * Fires events when the fullscreen state changes * * @private - * @param {HTMLElement|Event} [el] - Fullscreen element + * @param {HTMLElement|Event} el - Fullscreen element * @return {void} */ fullscreenchangeHandler = (el) => { let enter = false; - if (this.isSupported()) { - if (this.isFullscreen()) { - // Focus on the fullscreen element so keyboard - // events are triggered without an extra click - // If el has target property, then it is an Event - // otherwise it is a HTMLElement - const element = el.target || el; - - element.focus(); - enter = true; - } - } else if (!this.isFullscreen(el)) { - enter = true; - } + enter = (this.isSupported() && this.isFullscreen()) || (!this.isSupported() && !this.isFullscreen(el)); if (enter) { + this.focusFullscreenElement(el); this.emit('enter'); } else { this.emit('exit'); } }; + /** + * Focuses the element + * + * @private + * @param {HTMLElement|Event} el - Fullscreen element or event + * @return {void} + */ + focusFullscreenElement = (el) => { + if (!el) { + return; + } + // Focus on the fullscreen element so keyboard + // events are triggered without an extra click + // If el has target property, then it is an Event + // otherwise it is a HTMLElement + const element = el.target || el; + + element.focus(); + }; + /** * Toggles fullscreen mode * diff --git a/src/lib/__tests__/Fullscreen-test.js b/src/lib/__tests__/Fullscreen-test.js index a97b0f386..92a79e031 100644 --- a/src/lib/__tests__/Fullscreen-test.js +++ b/src/lib/__tests__/Fullscreen-test.js @@ -42,6 +42,7 @@ describe('lib/Fullscreen', () => { sandbox.stub(fullscreen, 'isSupported').returns(true); sandbox.stub(fullscreen, 'isFullscreen').returns(true); sandbox.stub(fullscreen, 'emit'); + sandbox.stub(fullscreen, 'focusFullscreenElement'); const element = document.createElement('div'); sandbox.stub(element, 'focus'); @@ -65,6 +66,7 @@ describe('lib/Fullscreen', () => { sandbox.stub(fullscreen, 'isSupported').returns(false); sandbox.stub(fullscreen, 'isFullscreen').returns(false); sandbox.stub(fullscreen, 'emit'); + sandbox.stub(fullscreen, 'focusFullscreenElement'); fullscreen.fullscreenchangeHandler({}); @@ -81,21 +83,11 @@ describe('lib/Fullscreen', () => { expect(fullscreen.emit).to.have.been.calledWith('exit'); }); - it('should be called only once when the fullscreenchange event is emitted', () => { - const spy = sandbox.spy(fullscreen, 'fullscreenchangeHandler'); - // rebind the dom listeners to use the spy - fullscreen.bindDOMListeners(); - - const event = new Event('webkitfullscreenchange'); - - window.document.dispatchEvent(event); - expect(spy).to.be.called.once; - }); - it('should focus element from passed in event', () => { sandbox.stub(fullscreen, 'isSupported').returns(true); sandbox.stub(fullscreen, 'isFullscreen').returns(true); sandbox.stub(fullscreen, 'emit'); + sandbox.stub(fullscreen, 'focusFullscreenElement'); const target = document.createElement('div'); sandbox.stub(target, 'focus'); @@ -104,20 +96,21 @@ describe('lib/Fullscreen', () => { fullscreen.fullscreenchangeHandler(event); expect(fullscreen.emit).to.have.been.calledWith('enter'); - expect(target.focus.called).to.be.true; + expect(fullscreen.focusFullscreenElement.called).to.be.true; }); it('should focus element from passed element', () => { sandbox.stub(fullscreen, 'isSupported').returns(true); sandbox.stub(fullscreen, 'isFullscreen').returns(true); sandbox.stub(fullscreen, 'emit'); + sandbox.stub(fullscreen, 'focusFullscreenElement'); const element = document.createElement('div'); sandbox.stub(element, 'focus'); fullscreen.fullscreenchangeHandler(element); expect(fullscreen.emit).to.have.been.calledWith('enter'); - expect(element.focus.called).to.be.true; + expect(fullscreen.focusFullscreenElement.called).to.be.true; }); }); From a9f02769e014320a1d7faa952f16a8e118029eda Mon Sep 17 00:00:00 2001 From: Conrad Chan Date: Tue, 6 Nov 2018 10:30:07 -0800 Subject: [PATCH 5/6] Chore: adding tests to focusFullscreenElement --- src/lib/Fullscreen.js | 3 -- src/lib/__tests__/Fullscreen-test.js | 59 ++++++++++++---------------- 2 files changed, 25 insertions(+), 37 deletions(-) diff --git a/src/lib/Fullscreen.js b/src/lib/Fullscreen.js index 0f9cfa242..769f7aac6 100644 --- a/src/lib/Fullscreen.js +++ b/src/lib/Fullscreen.js @@ -115,9 +115,6 @@ class Fullscreen extends EventEmitter { * @return {void} */ focusFullscreenElement = (el) => { - if (!el) { - return; - } // Focus on the fullscreen element so keyboard // events are triggered without an extra click // If el has target property, then it is an Event diff --git a/src/lib/__tests__/Fullscreen-test.js b/src/lib/__tests__/Fullscreen-test.js index 92a79e031..ab508d987 100644 --- a/src/lib/__tests__/Fullscreen-test.js +++ b/src/lib/__tests__/Fullscreen-test.js @@ -44,22 +44,22 @@ describe('lib/Fullscreen', () => { sandbox.stub(fullscreen, 'emit'); sandbox.stub(fullscreen, 'focusFullscreenElement'); - const element = document.createElement('div'); - sandbox.stub(element, 'focus'); - - fullscreen.fullscreenchangeHandler(element); + fullscreen.fullscreenchangeHandler({}); expect(fullscreen.emit).to.have.been.calledWith('enter'); + expect(fullscreen.focusFullscreenElement.called).to.be.true; }); it('should emit exit if we are exiting fullscreen and if true fullscreen is supported', () => { sandbox.stub(fullscreen, 'isSupported').returns(true); sandbox.stub(fullscreen, 'isFullscreen').returns(false); sandbox.stub(fullscreen, 'emit'); + sandbox.stub(fullscreen, 'focusFullscreenElement'); fullscreen.fullscreenchangeHandler({}); expect(fullscreen.emit).to.have.been.calledWith('exit'); + expect(fullscreen.focusFullscreenElement.called).to.be.false; }); it('should emit enter if we are entering fullscreen and if true fullscreen is not supported', () => { @@ -82,36 +82,6 @@ describe('lib/Fullscreen', () => { expect(fullscreen.emit).to.have.been.calledWith('exit'); }); - - it('should focus element from passed in event', () => { - sandbox.stub(fullscreen, 'isSupported').returns(true); - sandbox.stub(fullscreen, 'isFullscreen').returns(true); - sandbox.stub(fullscreen, 'emit'); - sandbox.stub(fullscreen, 'focusFullscreenElement'); - - const target = document.createElement('div'); - sandbox.stub(target, 'focus'); - const event = { target }; - - fullscreen.fullscreenchangeHandler(event); - - expect(fullscreen.emit).to.have.been.calledWith('enter'); - expect(fullscreen.focusFullscreenElement.called).to.be.true; - }); - - it('should focus element from passed element', () => { - sandbox.stub(fullscreen, 'isSupported').returns(true); - sandbox.stub(fullscreen, 'isFullscreen').returns(true); - sandbox.stub(fullscreen, 'emit'); - sandbox.stub(fullscreen, 'focusFullscreenElement'); - - const element = document.createElement('div'); - sandbox.stub(element, 'focus'); - fullscreen.fullscreenchangeHandler(element); - - expect(fullscreen.emit).to.have.been.calledWith('enter'); - expect(fullscreen.focusFullscreenElement.called).to.be.true; - }); }); describe('toggle()', () => { @@ -237,4 +207,25 @@ describe('lib/Fullscreen', () => { expect(fullscreen.fullscreenchangeHandler).to.have.been.calledWith(element); }); }); + + describe('focusFullscreenElement()', () => { + it('should focus the element when element passed in', () => { + const element = document.createElement('div'); + sandbox.stub(element, 'focus'); + + fullscreen.focusFullscreenElement(element); + + expect(element.focus.called).to.be.true; + }); + + it('should focus the element when event is passed in', () => { + const element = document.createElement('div'); + sandbox.stub(element, 'focus'); + const event = { target: element }; + + fullscreen.focusFullscreenElement(event); + + expect(element.focus.called).to.be.true; + }); + }); }); From 636b6cb8684d2b5b838531de3adf601ac6763b1c Mon Sep 17 00:00:00 2001 From: Conrad Chan Date: Tue, 6 Nov 2018 11:14:33 -0800 Subject: [PATCH 6/6] Chore: adding back event callback test --- src/lib/__tests__/Fullscreen-test.html | 1 + src/lib/__tests__/Fullscreen-test.js | 22 ++++++++++++++++++++++ 2 files changed, 23 insertions(+) create mode 100644 src/lib/__tests__/Fullscreen-test.html diff --git a/src/lib/__tests__/Fullscreen-test.html b/src/lib/__tests__/Fullscreen-test.html new file mode 100644 index 000000000..52a93d7c6 --- /dev/null +++ b/src/lib/__tests__/Fullscreen-test.html @@ -0,0 +1 @@ +
diff --git a/src/lib/__tests__/Fullscreen-test.js b/src/lib/__tests__/Fullscreen-test.js index ab508d987..7bf992981 100644 --- a/src/lib/__tests__/Fullscreen-test.js +++ b/src/lib/__tests__/Fullscreen-test.js @@ -38,6 +38,18 @@ describe('lib/Fullscreen', () => { }); describe('fullscreenchangeHandler()', () => { + before(() => { + fixture.setBase('src/lib'); + }); + + beforeEach(() => { + fixture.load('__tests__/Fullscreen-test.html'); + }); + + afterEach(() => { + fixture.cleanup(); + }); + it('should emit enter if we are entering fullscreen and if true fullscreen is supported', () => { sandbox.stub(fullscreen, 'isSupported').returns(true); sandbox.stub(fullscreen, 'isFullscreen').returns(true); @@ -82,6 +94,16 @@ describe('lib/Fullscreen', () => { expect(fullscreen.emit).to.have.been.calledWith('exit'); }); + + it('should be called only once when the fullscreenchange event is emitted', () => { + const spy = sandbox.spy(fullscreen, 'fullscreenchangeHandler'); + sandbox.stub(fullscreen, 'focusFullscreenElement').returns(true); + // rebind the dom listeners to use the spy + fullscreen.bindDOMListeners(); + const event = new Event('webkitfullscreenchange', { bubbles: true }); + document.getElementById('test-container').dispatchEvent(event); + expect(spy).to.be.called.once; + }); }); describe('toggle()', () => {