From fb1fdd0194632206a6ea4b30efbed3396d34cb2e Mon Sep 17 00:00:00 2001 From: bhh1988 Date: Mon, 22 May 2017 16:50:25 -0700 Subject: [PATCH] Fix: Seek time doesn't match filmstrip timecode (#131) Previously, hovering over a time in the scrubber to seek to that point would display a timecode that doesn't match the actual time that the seek would take you to. This was more apparent in longer videos. For example, hovering over 10:00 in a 1hr video, and clicking at that point would actually take you to 9:30. This was due to two different ways the timecodes (the filmstrip timecode versus the seeked timecode) were calculated. --- src/lib/viewers/media/MediaControls.js | 2 +- src/lib/viewers/media/Scrubber.js | 19 +++++--- .../media/__tests__/MediaControls-test.js | 8 +++- .../viewers/media/__tests__/Scrubber-test.js | 43 +++++++++++++++++-- 4 files changed, 62 insertions(+), 10 deletions(-) diff --git a/src/lib/viewers/media/MediaControls.js b/src/lib/viewers/media/MediaControls.js index 5766f3865..5cd846462 100644 --- a/src/lib/viewers/media/MediaControls.js +++ b/src/lib/viewers/media/MediaControls.js @@ -721,7 +721,7 @@ class MediaControls extends EventEmitter { * @return {Object} */ computeFilmstripPositions(pageX, rectLeft, rectWidth, filmstripWidth) { - const time = ((pageX - rectLeft) * this.mediaEl.duration) / rectWidth; // given the mouse X position, get the relative time + const time = this.timeScrubber.computeScrubberPosition(pageX) * this.mediaEl.duration; // given the mouse X position, get the relative time const frame = Math.floor(time / this.filmstripInterval); // get the frame number to show let frameWidth = filmstripWidth / FILMSTRIP_FRAMES_PER_ROW; // calculate the frame width based on the filmstrip width let left = -1 * (frame % FILMSTRIP_FRAMES_PER_ROW) * frameWidth; // get the frame position in a given row diff --git a/src/lib/viewers/media/Scrubber.js b/src/lib/viewers/media/Scrubber.js index effd67893..eea6827ea 100644 --- a/src/lib/viewers/media/Scrubber.js +++ b/src/lib/viewers/media/Scrubber.js @@ -162,19 +162,28 @@ class Scrubber extends EventEmitter { this.convertedEl.style.width = `${this.convertedValue * 100}%`; } + /** + * Returns the relative X position of mouse on scrubber + * + * @param {number} pageX - Mouse X position + * @return {number} A float between 0 and 1, for the relative position of pageX + */ + computeScrubberPosition(pageX) { + const rect = this.scrubberEl.getBoundingClientRect(); + const relPos = (pageX - rect.left) / rect.width; + return Math.max(Math.min(relPos, MAX_VALUE), MIN_VALUE); + } + /** * Calculates the position of the scrubber handle based on mouse action * * @private - * @param {Event} event - the instance of the class + * @param {Event} event - The mouse event that this handler is subscribed to * @return {void} */ scrubbingHandler(event) { - const rect = this.scrubberEl.getBoundingClientRect(); const pageX = event.pageX; - let newValue = (pageX - rect.left) / rect.width; - - newValue = Math.max(Math.min(newValue, MAX_VALUE), MIN_VALUE); + const newValue = this.computeScrubberPosition(pageX); this.setValue(newValue); this.emit('valuechange'); diff --git a/src/lib/viewers/media/__tests__/MediaControls-test.js b/src/lib/viewers/media/__tests__/MediaControls-test.js index 515c82602..1c61e50e8 100644 --- a/src/lib/viewers/media/__tests__/MediaControls-test.js +++ b/src/lib/viewers/media/__tests__/MediaControls-test.js @@ -752,10 +752,11 @@ describe('lib/viewers/media/MediaControls', () => { duration: 100 }; mediaControls.filmstripInterval = 1; + sandbox.stub(mediaControls.timeScrubber, 'computeScrubberPosition').returns(0.25); const positions = mediaControls.computeFilmstripPositions(400, 200, 1000, null); - expect(positions.time).to.equal(20); + expect(positions.time).to.equal(25); expect(positions.left).to.equal(0); expect(positions.top).to.equal(0); expect(positions.containerLeft).to.equal(120); @@ -766,6 +767,7 @@ describe('lib/viewers/media/MediaControls', () => { duration: 100 }; mediaControls.filmstripInterval = 1; + sandbox.stub(mediaControls.timeScrubber, 'computeScrubberPosition').returns(0.2); const positions = mediaControls.computeFilmstripPositions(400, 200, 1000, 16000); @@ -780,6 +782,7 @@ describe('lib/viewers/media/MediaControls', () => { duration: 1100 }; mediaControls.filmstripInterval = 1; + sandbox.stub(mediaControls.timeScrubber, 'computeScrubberPosition').returns(0.2); const positions = mediaControls.computeFilmstripPositions(400, 200, 1000, 16000); @@ -794,6 +797,7 @@ describe('lib/viewers/media/MediaControls', () => { duration: 2000 }; mediaControls.filmstripInterval = 5; + sandbox.stub(mediaControls.timeScrubber, 'computeScrubberPosition').returns(0.6); const positions = mediaControls.computeFilmstripPositions(800, 200, 1000, 16000); @@ -808,6 +812,7 @@ describe('lib/viewers/media/MediaControls', () => { duration: 100 }; mediaControls.filmstripInterval = 1; + sandbox.stub(mediaControls.timeScrubber, 'computeScrubberPosition').returns(0.01); const positions = mediaControls.computeFilmstripPositions(210, 200, 1000, 16000); @@ -822,6 +827,7 @@ describe('lib/viewers/media/MediaControls', () => { duration: 100 }; mediaControls.filmstripInterval = 1; + sandbox.stub(mediaControls.timeScrubber, 'computeScrubberPosition').returns(0.99); const positions = mediaControls.computeFilmstripPositions(1190, 200, 1000, 16000); diff --git a/src/lib/viewers/media/__tests__/Scrubber-test.js b/src/lib/viewers/media/__tests__/Scrubber-test.js index 94be9a9dc..aa058e54c 100644 --- a/src/lib/viewers/media/__tests__/Scrubber-test.js +++ b/src/lib/viewers/media/__tests__/Scrubber-test.js @@ -122,14 +122,51 @@ describe('lib/viewers/media/Scrubber', () => { it('should adjust the scrubber value to the current scrubber handle position value in the video', () => { stubs.setValue = sandbox.stub(scrubber, 'setValue'); stubs.emit = sandbox.stub(scrubber, 'emit'); - scrubber.scrubberEl.style.width = '25px'; - scrubber.scrubbingHandler({ pageX: 100 }); + stubs.scrubberPosition = sandbox.stub(scrubber, 'computeScrubberPosition').returns(0.5); - expect(stubs.setValue).to.be.calledWith(1); + scrubber.scrubbingHandler({ pageX: 50 }); + + expect(stubs.scrubberPosition).to.be.calledWith(50); + expect(stubs.setValue).to.be.calledWith(0.5); expect(stubs.emit).to.be.calledWith('valuechange'); }); }); + describe('computeScrubberPosition()', () => { + it('should compute correct scrubber position', () => { + sandbox.stub(scrubber.scrubberEl, 'getBoundingClientRect').returns({ + left: 20, + width: 100 + }); + + const position = scrubber.computeScrubberPosition(30); + + expect(position).to.equal(0.1); + }); + + it('should cap the scrubber position to 1', () => { + sandbox.stub(scrubber.scrubberEl, 'getBoundingClientRect').returns({ + left: 20, + width: 100 + }); + + const position = scrubber.computeScrubberPosition(130); + + expect(position).to.equal(1); + }); + + it('should floor the scrubber position to 0', () => { + sandbox.stub(scrubber.scrubberEl, 'getBoundingClientRect').returns({ + left: 20, + width: 100 + }); + + const position = scrubber.computeScrubberPosition(10); + + expect(position).to.equal(0); + }); + }); + describe('mouseDownHandler()', () => { beforeEach(() => { stubs.scrub = sandbox.stub(scrubber, 'scrubbingHandler');