Paused/finished YouTube videos automatically start playing when scrolling up far enough to load new posts

This has been assigned both to @eviltrout and @Osama over the years, we have invested lots of time here and simply can not come up with a fix.

I am tagging this #pr-welcome … but keep in mind … difficulty level on this is somewhere around 11.

image

13 Likes

Sorry to resurrect this, but I’ve noticed this starts happening now even when scrolling down. Not sure if something changed this bug recently to the worse, or if it’s a different bug altogether. Confirmed with several people in our instance. This makes video sharing threads quite unbearable, when the earlier videos start suddenly playing.

I second this, happens with either scroll up or down and replays all previously played videos randomly (I imagine it has something to do with how Discourse processes previous content when loading new on a Topic, but I honestly don’t know).

I have mixed feelings about this, I don’t think people should have the expectation that a video from 600+ posts above will continue playing as you scroll down endlessly in a megatopic? The video isn’t visible, and hasn’t been visible for a while if you are scrolling down.

e.g. if you want to play audio for a long time while you browse around, on whatever site you’re visiting, open it in a new tab?

It shouldn’t, in my opinion. If the video is paused or finished it shouldn’t replay itself on its own just because you scrolled down. (Which is the described behaviour even though the title is misleading)

As a matter of fact, there is a an alternative for people that want “people to keep playing” as you go around, if they find the tab approach too much.

1 Like

Does this look better?

This still happen on 3.0 stable. Funny thing is one video refresh when scrolling UP, on DOWN its still paused, another random video do the oposite. Stay paused when scroll UP and reftesh play on scroll down.

Very annoying

We have not yet implemented any changes here.

Our plan is to rebuild our topic view over the next year or 2 which should hopefully improve the situation. There is no easy fix here, we tried.

2 Likes

Is there something wrong or hard with patching virtual-dom to fix this problem?

A PR is welcome, we tried but it was simply too hard

2 Likes

Very interesting! Moving to a fork of vdom isn’t ideal, but we might be able to consider it as long as we can prove there are no regressions (and we move it to our own GitHub organisation).

Were you able to run the vdom test suite to confirm there are no regressions? How easy would be it be to add a new vdom test for this prepend behaviour?

2 Likes

I’ll first address the second question. This prepend behavior is essentially the same, it just eliminates some extra work. I’ll try to illustrate what vdom does originally and after the patch.

Suppose there are 20 posts and 10 are being loaded on top after scroll.
Originally vdom makes this sequence of transformations:
[20 old] → [20 old, 10 new] // 10 new elements are appended to the end
[20 old, 10 new] → // all 30 elements are removed
→ [10 new, 20 old] // all 30 elements are inserted into their place
After the patch the sequence becomes:
[20 old] → [20 old, 10 new] // 10 new elements are appended to the end
[20 old, 10 new] → [20 old] // 10 new elements are removed from the end
[20 old] → [10 new, 20 old] // 10 new elements are prepended
As you can see there’s still extra work: in principle you can prepend those elements right away, but this way I could just add some code in one place and not bother about rewriting what’s already there.
Each of the transformations becomes a dom operation, so youtube videos start playing because originally old elements are reinserted (which triggers them being rerendered), and after the patch they stay in place.

2 Likes

Here is an excerpt from virtual-dom’s package.json

"scripts": {
    "test": "node ./test/index.js | tap-spec",
    "dot": "node ./test/index.js | tap-dot",
    "start": "node ./index.js",
    "cover": "istanbul cover --report html --print detail ./test/index.js",
    "view-cover": "istanbul report html && opn ./coverage/index.html",
    "browser": "run-browser test/index.js",
    "phantom": "run-browser test/index.js -b | tap-spec",
    "dist": "browserify --standalone virtual-dom index.js > dist/virtual-dom.js",
    "travis-test": "npm run phantom && npm run cover && istanbul report lcov && ((cat coverage/lcov.info | coveralls) || exit 0)",
    "release": "npm run release-patch",
    "release-patch": "git checkout master && npm version patch && git push origin master --tags && npm publish",
    "release-minor": "git checkout master && npm version minor && git push origin master --tags && npm publish",
    "release-major": "git checkout master && npm version major && git push origin master --tags && npm publish"
  },

Of these ‘test’, ‘dot’, ‘cover’, ‘view-cover’, ‘browser’, ‘phantom’, ‘travis-test’ seem relevant to testing.
‘browser’, ‘phantom’, ‘travis-test’ give a parsing error due to newer javascript constructs in my code. Others pass. If I change this code

        var prepend = simulate.every(item => item && item.key)
        prepend &&= aChildren.every((item, i) => item.key === bChildren[i + shift].key)

to this code

        var prepend = true
        for (var i = 0; prepend && i < simulate.length; i++) {
            prepend = simulate[i] && simulate[i].key
        }
        for (var i = 0; prepend && i < aChildren.length; i++) {
            prepend = aChildren[i].key === bChildren[i + shift].key
        }

then all pass. I can push this change to keep javascript consistently old if satisfying all these scripts is desirable.

2 Likes

:+1: please do - let’s keep their existing test suite working

I just forked virtual-dom into GitHub - discourse/virtual-dom: A Virtual DOM and diffing algorithm - please can you make a PR against that repo?

3 Likes
3 Likes

I should also note that my change to virtual-dom is very narrow. It targets prepends specifically and falls back on the original algorithm in all other cases. And that original algorithm is still imperfect if you look at the problem generally (it’s not hard to come up with examples, when it would touch old elements unnecessarily). On the other hand it handles appends, removes, single inserts just fine. And with this you need to get real fancy with the post stream to break out. So practically speaking solving the problem generally might be a bit of an overkill, though of course you can sleep better when you do.

I pushed some new tests. Are you planning to review the PR any time soon?

1 Like

Thanks Aleksey - I will try to get it reviewed within the next week

4 Likes