igrigorik / videospeed

HTML5 video speed controller (for Google Chrome)
https://chrome.google.com/webstore/detail/video-speed-controller/nffaoalbilbmmfgbnbgppjihopabppdk
MIT License
3.82k stars 544 forks source link

Breaks/doesn't work with the Videopress videos #194

Open sainaen opened 7 years ago

sainaen commented 7 years ago

Demo: https://videopress.com/

To partially “unbreak” it: close the speed controls panel before clicking on the video. But this isn't a full solution, as the hotkeys don't work.

After a little investigation, I found that

I don't see how this could be fixed on the extension side, except by blacklisting, but it's not easy to do either: Videopress may be enabled on arbitrary domains. Maybe some heuristic could be implemented that checks for the __vd_<something>_video pattern in class name of a parent element, but this feels too hacky and fragile.

igrigorik commented 7 years ago

Yikes. Thanks for reporting this.

It looks like it's an Automattic / JetPack feature.. I filed a support request on their site pointing to this bug. Fingers crossed, hopefully we'll get someone from their team to chime in.

I don't see how this could be fixed on the extension side, except by blacklisting, but it's not easy to do either: Videopress may be enabled on arbitrary domains. Maybe some heuristic could be implemented that checks for the _vd_video pattern in class name of a parent element, but this feels too hacky and fragile.

Exactly, this is really fragile and a burden to maintain.. not to mention, we lose the ability to control video speed! Let's give Automattic folks a few days and see if they're able to chime in.

dbtlr commented 7 years ago

I'm one of the maintainers for VideoPress. I just wanted to chime in and let you guys know the situation from our side. If I'm understanding the issue correctly, the videospeed extension is having trouble targeting a VideoPress video because our container classes are constantly updating?

Unfortunately, there is no easy or simple fix for this. When the HTML5 version of VideoPress was released, we wrote it with an in-house state aware library that was written specifically to be React like, but 1/10th the file size. Part of its internal mechanism for maintaining its state on a page with several directly embedded videos is to have classes that are generated and maintained by the library itself.

This was a very early design decision within the library, as its entire DOM is meant to be virtual and maintained as such. We've had a few discussions about this decision, as the developer who wrote decided on this direction and wrote the library has since moved off it, but it would require a large-scale rewrite that we're not prepared to do right now.

I'm not entirely sure there is much we can do right at this moment to make this work for you.

One potential solution might be that we're looking at making VideoPress and its libraries OSS in the near future, which could also help by virtue of at least making what is happening more transparent AND allow you to potentially hack on it a bit to try and introduce some better hooks. We'd love to see that happen.

I'll ping back on this thread if anything else changes, but for right now I don't have a ton of options for you here.

igrigorik commented 7 years ago

@dbtlr thanks for chiming in!

Based on the notes @sainaen shared earlier..

There is nothing inherently broken about a state-aware approach you guys took, it just needs to be more permissive of other content on the page; your script doesn't run in isolation in most cases. :)

Re, OSS: any idea when that might happen? :)

dbtlr commented 7 years ago

@igrigorik I totally agree with everything you said and I don't mean to blame it on the state-aware approach itself. Just the library that was written to achieve state awareness treats the whole video player as a virtual DOM, which doesn't make integrating outside of it very easy.

In terms of OSS, no ETA. We're still working out the specifics there. Hopefully it isn't too far in the future.

igrigorik commented 7 years ago

@dbtlr any updates on WP side? :)

dbtlr commented 7 years ago

@igrigorik I'm not sure if/when OSS is going to be occurring with the VideoPress player at this point.

What I can say for certain at this point, is that due to the complexity of the approach used to manage the virtual DOM in the VideoPress player, it is unlikely that we will be doing the necessary refactors to support this type of introspection in the near future.

I wish I could provide a better answer right now, but VideoPress updates and changes just are not on our near term roadmap, compared to other updates we're looking to do with our other products.

ChadBailey commented 4 years ago

Due to the age and consequences of this bug, I'm going to take a deeper look before 0.7. I suspect as part of the restructure effort I mentioned in #589 we may be able to restructure in such a way that we no longer need to update the elements which would fix this (and potentially also solve for other unknown unknown side-effects of mutating a website's state)

Assignment to 0.7 is for investigation only, just adding the version here so I can easily find it/not forget. If #589 is fixed, I think it will require a fairly major rewrite that I anticipate will happen in a future version (potentially, marking the beginning of 1.0?)

StefMattana commented 4 years ago

That's been reported again in our ticket queues. The user shared a gif to show the issue at their end.

I suggested to use this Chrome add-on but that doesn't solve the conflict really. I know that's an old thread but if anyone could look at that also to confirm it's a Sad Christmas, that'd be nice :)