Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: don't always use fastSeek when available. #7527

Merged
merged 5 commits into from
Nov 17, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions src/js/control-bar/progress-control/seek-bar.js
Original file line number Diff line number Diff line change
Expand Up @@ -256,7 +256,6 @@ class SeekBar extends Slider {

// Stop event propagation to prevent double fire in progress-control.js
event.stopPropagation();
this.player_.scrubbing(true);

this.videoWasPlaying = !this.player_.paused();
this.player_.pause();
Expand All @@ -269,13 +268,19 @@ class SeekBar extends Slider {
*
* @param {EventTarget~Event} event
* The `mousemove` event that caused this to run.
* @param {boolean} mouseDown this is a flag that should be set to true if `handleMouseMove` is called directly. It allows us to skip things that should not happen if coming from mouse down but should happen on regular mouse move handler. Defaults to false
*
* @listens mousemove
*/
handleMouseMove(event) {
handleMouseMove(event, mouseDown = false) {
if (!Dom.isSingleLeftClick(event)) {
return;
}

if (!mouseDown && !this.player_.scrubbing()) {
this.player_.scrubbing(true);
gkatsev marked this conversation as resolved.
Show resolved Hide resolved
}

let newTime;
const distance = this.calculateDistance(event);
const liveTracker = this.player_.liveTracker;
Expand Down
3 changes: 2 additions & 1 deletion src/js/slider/slider.js
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ class Slider extends Component {
this.on(doc, 'touchmove', this.handleMouseMove_);
this.on(doc, 'touchend', this.handleMouseUp_);

this.handleMouseMove(event);
this.handleMouseMove(event, true);
gkatsev marked this conversation as resolved.
Show resolved Hide resolved
}

/**
Expand All @@ -192,6 +192,7 @@ class Slider extends Component {
* @param {EventTarget~Event} event
* `mousedown`, `mousemove`, `touchstart`, or `touchmove` event that triggered
* this function
* @param {boolean} mouseDown this is a flag that should be set to true if `handleMouseMove` is called directly. It allows us to skip things that should not happen if coming from mouse down but should happen on regular mouse move handler. Defaults to false.
*
* @listens mousemove
* @listens touchmove
Expand Down
31 changes: 31 additions & 0 deletions test/unit/controls.test.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
/* eslint-env qunit */
import EventTarget from '../../src/js/event-target.js';
import VolumeControl from '../../src/js/control-bar/volume-control/volume-control.js';
import MuteToggle from '../../src/js/control-bar/mute-toggle.js';
import VolumeBar from '../../src/js/control-bar/volume-control/volume-bar.js';
Expand All @@ -8,6 +9,7 @@ import Slider from '../../src/js/slider/slider.js';
import PictureInPictureToggle from '../../src/js/control-bar/picture-in-picture-toggle.js';
import FullscreenToggle from '../../src/js/control-bar/fullscreen-toggle.js';
import ControlBar from '../../src/js/control-bar/control-bar.js';
import SeekBar from '../../src/js/control-bar/progress-control/seek-bar.js';
import TestHelpers from './test-helpers.js';
import document from 'global/document';
import sinon from 'sinon';
Expand Down Expand Up @@ -141,6 +143,35 @@ QUnit.test('calculateDistance should use changedTouches, if available', function
slider.dispose();
});

QUnit.test("SeekBar doesn't set scrubbing on mouse down, only on mouse move", function(assert) {
const player = TestHelpers.makePlayer();
const scrubbingSpy = sinon.spy(player, 'scrubbing');
const seekBar = new SeekBar(player);
const doc = new EventTarget();

// mousemove is listened to on the document.
// Specifically, we check the ownerDocument of the seekBar's bar.
// Therefore, we want to mock it out to be able to trigger mousemove
seekBar.bar.dispose();
seekBar.bar.el_ = new EventTarget();
seekBar.bar.el_.ownerDocument = doc;

seekBar.trigger('mousedown');
assert.ok(scrubbingSpy.calledWith(), 'called scrubbing as a getter');
assert.notOk(scrubbingSpy.calledWith(true), 'did not set scrubbing true');

player.scrubbing(false);

scrubbingSpy.resetHistory();

doc.trigger('mousemove');
assert.ok(scrubbingSpy.calledWith(), 'called scrubbing as a getter');
assert.ok(scrubbingSpy.calledWith(true), 'did set scrubbing true');

seekBar.dispose();
player.dispose();
});

QUnit.test('playback rate button is hidden by default', function(assert) {
assert.expect(1);

Expand Down