Add custom event for SponsorBlock

[ajayyy]

Fixes #683 and ajayyy/SponsorBlock#359

[igrigorik]

We already dispatch a custom event in:

videospeed/inject.js

Lines 704 to 707 in 113ec89

	video.dispatchEvent(
	new CustomEvent("ratechange", {
	detail: { origin: "videoSpeed", speed: speedvalue }
	})

Is that one not sufficient? Anything we can refactor? My intuition is that we should only be dispatching one event.

[ajayyy]

This custom event is being still being accepted by videospeed, which is then stopping propagation.

videospeed/inject.js

Lines 451 to 455 in 113ec89

	"ratechange",
	function (event) {
	if (coolDown) {
	log("Speed event propagation blocked", 4);
	event.stopImmediatePropagation();

[igrigorik]

Per the current logic, that should only stop propagation is we're in cool-down phase and intentionally want to block another speed update, and proceed otherwise — right? You should be able to catch this event upstream.. what am I missing?

[ajayyy]

Could the issue be that refreshCoolDown is getting called before the CustomEvent fires? That could cause coolDown to always not be false.

videospeed/inject.js

Lines 700 to 717 in 113ec89

	function setSpeed(video, speed) {
	log("setSpeed started: " + speed, 5);
	var speedvalue = speed.toFixed(2);
	if (tc.settings.forceLastSavedSpeed) {
	video.dispatchEvent(
	new CustomEvent("ratechange", {
	detail: { origin: "videoSpeed", speed: speedvalue }
	})
	);
	} else {
	video.playbackRate = Number(speedvalue);
	}
	var speedIndicator = video.vsc.speedIndicator;
	speedIndicator.textContent = speedvalue;
	tc.settings.lastSpeed = speed;
	refreshCoolDown();
	log("setSpeed finished: " + speed, 5);
	}

[ajayyy]

After some testing, ratechange events do work when using the YouTube settings, and only don't get passed to my extension when the buttons on the video speed extension are pressed. So, it seems to be an issue with coolDown being true when it should not be.

Could you explain what the purpose of the cool down is?

[igrigorik]

@ChadBailey implemented the logic and can provide full context. The short version though, if I recall correctly, it helps protect from auto-revert that some players invoke after we force a speed update.

I don't see a reason why we can't or shouldn't emit a ratechange event for button presses. That logic should not be subject to cooldown though; we don't want to throttle user-initiated changes.

[ajayyy]

Okay, that's what I was thinking. I was thinking that this custom event would allow videospeed to hide the ratechange from the website while still allowing other extensions to know about it. This way, it can still prevent the auto-revert issue,

[ChadBailey]

I know this is a wayyy late reply, but I never saw this until now :( I figure I will give the explanation, better late than never.

This effort was done to fix a few problematic sites which would de-apply rate changes directly after applying them since the rate change did not originate from the site itself. If we are finding that this is having unintended side-effects, it may be a good idea to try and implement a second list similar to the disable-list which informs videospeed controller when to enable this behavior.

[ajayyy]

yea, this is what I was thinking. If so, a disable-list like you said could work, or an extra event like the one added here so that it will not be picked up by the site itself, but only by "compatible" extensions.

The change here should have the least chance at breaking something since it just adds a new event

[ajayyy]

Any thoughts about merging this? I'm still getting complaints about SponsorBlock not working with videospeed. No rush though.

[igrigorik]

@ajayyy happy to land this but I don't think we addressed the earlier comment in the PR? Once we get the button-press logic fixed, my understanding is that this new event might not be necessary?

[benjo456]

Any news on when this can get merged? It means I cannot use this extension at all currently

[Danstiv]

Any news?

[FedeCuci]

Any news on closing this PR? Many people use both SB and Video Speed Controller so it would help a lot 👍

[brzzdev]

I'd love to see this merged in. Use both these extensions and having them work together would be great 👍🏻

[ChadBailey]

Unfortunately, I don't think we ever fully landed on a solution...

@ajayyy if you happen to still be around, could you confirm if this was ever resolved/would your implementation correct the problem or is there more work to be done?

(also, terribly sorry for not responding for so long... my github notifications were clogged up by unnecessary subscriptions which had the unfortunate side effect of me ignoring my notifications entirely)

[ajayyy]

No worries. The current solution works, we were just discussing if a more elegant solution would be better instead of just forwarding a new event type for use by SponsorBlock.

To prevent accidentally breaking compatibility on some sites, I personally think merging it as is would be the best option.

[ChadBailey]

The TL;DR of the below text is, this behavior was an intentional mechanism to prevent websites from being able to easily detect the user is using VSC. So it was a feature, not a bug.

I don't know why some websites are coded in such a way to always maintain positive control over speed. Maybe it's merely due to (what I would argue as very bad) design decisions or more nefariously a lack of respect for the end user to make their own decisions... Regardless of the reason, I did not want to "show our hand" that the user is using VSC. The website which has already shown a pattern of desiring to remove the user's ability to change speeds may use this information to build counter-measures and I prefer to discourage that behavior before it begins.

Edit: Sorry, I didn't really leave you with anything actionable... With this said, I suspect this may not be a meaningful protection mechanism. The DOM is still being manipulated to add the controller which I think can be used as a means of detection. Maybe we just give up the feature and refactor to always emit the event? If this makes other extensions and websites able to work with the extension rather than against, perhaps it's the lesser evil.

Below is a FULL breakdown of this mechanism, why it exists, and why it's been designed in the way it's been designed.

---

The problem the code in question addresses is poorly behaving websites which "undo" speed changes directly after VSC requests them. The mechanism in use for preventing this behavior is calling stopImmediatePropagation() on the ratechange event that gets emitted by the browser with every rate change. Problem solved! The browser no longer taddletales on us for requesting an "unauthorized speed change" thus a subsequent request to change it back by the parent site is no longer requested because the parent site has no idea the speed was changed.

Not so fast... the naive solution is to simply block all speed change announcements, but causes lots of new issues. Some websites have speed controls that are very valid - maybe they are like YouTube and have their own custom way of setting video speeds in the player. We don't want to stop users from being able to use the native controls. Maybe the website (like Twitch) has valid reasons to send hidden minor speed tweaks to slowly inch the viewer towards an ideal position in time with the live stream so that everyone watching gets to see the same thing at the same time (very clever, by the way). Clearly, we can't just naively block every speed event so what do we do?

The solution we use is to set a cooldown for a short period of time (1 second) after the user requests a speed change. This does get set back here via the use of a timer

videospeed/inject.js

Lines 412 to 414 in 113ec89

	coolDown = setTimeout(function () {
	coolDown = false;
	}, 1000);

While this cooldown period is active, ratechange event propagation is blocked via stopImmediatePropagation() causing the site to be unaware of our nefarious activities (how dare we change the speed?). Unfortunately, this is unreliable because sometimes the change still slips by. I can't say for sure why but I expect it's either a race condition, the website somehow still knowing and explicitly setting a new speed, or both. So, if we see a ratechange event that wasn't created by us during that cooldown window, we explicitly set it right back to the speed the user requested.

videospeed/inject.js

Line 485 in caacb45

video.playbackRate = tc.settings.lastSpeed;

We're still not done, because the controller also does not see the rate change take place. Ah, so we just emit a custom event instead of listening for a rate change and job done right? Not so fast. While this works just fine when the speed change is requested by VSC, VSC still needs to know when the speed changes by other means so that both it's showing the correct speed, and so that we are aware of the new current speed in order to set the correct speed on our next adjustment attempt.

Finally, we arrive at the current implementation. Any time an event propagation is prevented, a custom event is fired so that the rest of the plugin is aware and able to collect the new speed data that's needed.

That brings us to the problem this PR is supposed to address. I think the reason I decided to only fire this event while propagation was blocked was simply to be courteous and only emit the event as needed? Still, I imagine this would be with every use of VSC assuming the setting was enabled so I don't see a time when it wouldn't be available.

The logic appears to be essentially unchanged from what I can tell, you can see here:

videospeed/inject.js

Lines 721 to 729 in caacb45

	if (tc.settings.forceLastSavedSpeed) {
	video.dispatchEvent(
	new CustomEvent("ratechange", {
	detail: { origin: "videoSpeed", speed: speedvalue }
	})
	);
	} else {
	video.playbackRate = Number(speedvalue);
	}

All change requests should be emitted as the custom event "ratechange". You can pick up on the ones emitted by VSC by checking in the same fashion as is shown here. If you can not see this event being emitted, there may be a bug as I think it's intended to always emit though not an explicitly designed behavior.

This is all based off of vague memories from long ago, so I may not have all of the details correct but I think they are.

[ChadBailey]

Sorry, memories are slowly trickling back in... yea capturing that event probably won't work because it will still get stopped by the listener for ratechange which stops event propagation. Again, I suspect that was intentional.

Not sure where this leaves us, but if @igrigorik agrees this is an unnecessary countermeasure I think the best path forward would be to refactor making us always emit the event. It will still need the additional data of the speed with it though.

Essentially, it would probably go something like this...

OLD

function setSpeed(video, speed) {
  log("setSpeed started: " + speed, 5);
  var speedvalue = speed.toFixed(2);
  if (tc.settings.forceLastSavedSpeed) {
    video.dispatchEvent(
      new CustomEvent("ratechange", {
        detail: { origin: "videoSpeed", speed: speedvalue }
      })
    );
  } else {
    video.playbackRate = Number(speedvalue);
  }
  var speedIndicator = video.vsc.speedIndicator;
  speedIndicator.textContent = speedvalue;
  tc.settings.lastSpeed = speed;
  refreshCoolDown();
  log("setSpeed finished: " + speed, 5);
}

  document.addEventListener(
    "ratechange",
    function (event) {
      if (coolDown) {
        log("Speed event propagation blocked", 4);
        event.stopImmediatePropagation();
      }
      var video = event.target;

      /**
       * If the last speed is forced, only update the speed based on events created by
       * video speed instead of all video speed change events.
       */
      if (tc.settings.forceLastSavedSpeed) {
        if (event.detail && event.detail.origin === "videoSpeed") {
          video.playbackRate = event.detail.speed;
          updateSpeedFromEvent(video);
        } else {
          video.playbackRate = tc.settings.lastSpeed;
        }
        event.stopImmediatePropagation();
      } else {
        updateSpeedFromEvent(video);
      }
    },
    true
  );

NEW

function setSpeed(video, speed) {
  log("setSpeed started: " + speed, 5);
  var speedvalue = speed.toFixed(2);
  video.dispatchEvent(
    new CustomEvent("videospeed_setspeed", {
      detail: { origin: "videoSpeed", speed: speedvalue }
    })
  );
  var speedIndicator = video.vsc.speedIndicator;
  speedIndicator.textContent = speedvalue;
  tc.settings.lastSpeed = speed;
  refreshCoolDown();
  log("setSpeed finished: " + speed, 5);
}

  document.addEventListener(
    "ratechange",
    function (event) {
      if (coolDown) {
        log("Speed event propagation blocked", 4);
        event.stopImmediatePropagation();
      }
      var video = event.target;
      updateSpeedFromEvent(video);
    },
    true
  );
  document.addEventListener(
    "videospeed_setspeed",
    function (event) {
      var video = event.target;
      video.playbackRate = event.detail.speed;
      updateSpeedFromEvent(video);
      }
    },
    true
  );

Edit: This would probably cause an infinite loop by setting the speed each time a ratechange is detected, so it needs more work but gives an idea of the direction of implementation

[igrigorik]

@ChadBailey thanks for digging in on this. I've reread the sketch code above but, I admit, still murky on what the diff is — we're still leveraging stopImmediatePropagation()? In general though, not opposed to refactors, especially if it simplifies the core logic. We may regress in some places, but if we gain better composability and interop with other extensions, that's a tradeoff that may (probably) be worth taking.

Since there are a number of different factors at play here (diff extensions, players, etc), my experience and recommendation: prototype and run it as unpacked to see where things break.

[ChadBailey]

I agree with your sentiments. Ultimately, I believe the following to be true:

1. Offering an olive branch to websites and other extensions who want to improve compatibility is preferred to trying to force compatibility on our end

2. Our additional compatibility measures should probably only be applied when necessary rather than on all sites, unless we find that most sites require it

With that in mind, I think I'm going to try and take a stab at refactoring with those goals in mind over this coming week since it's been refreshed in my mind. I'll keep you all updated on this thread!

If anyone has any differences of opinion let me know, but it sounds to me like we're all on the same page.

[ChadBailey]

I haven't forgotten about this, just de-prioritizing it for now. After I get the refactored version merged in I will be taking a look at API possibilities such as this to allow easy interoperability between other extensions and websites.

See also my comments here #896 (comment)