Automattic / jetpack

Security, performance, marketing, and design tools — Jetpack is made by WordPress experts to make WP sites safer and faster, and help you grow your traffic.
https://jetpack.com/
Other
1.58k stars 797 forks source link

Infinite Scroll: Scrolling down to native videos sometimes borks the embed #1439

Open aheckler opened 9 years ago

aheckler commented 9 years ago

To repro:

  1. Install Jetpack.
  2. Turn on Infinite Scroll.
  3. Publish a post with a native video inside it.
  4. Publish a number of posts after the video post.
  5. Load the front page and scroll down until IS loads the post with the video.

You will often see this:

screen shot 2014-12-17 at 3 46 41 pm

After some experimenting, I think this bug was introduced in 3.2, though I can't be 100% sure because it doesn't happen every time. 2028064-t.

EDIT: User was using his own theme, but I reproduced the issue using Twenty Fourteen.

PatTheMav commented 9 years ago

Adam asked my to do some test runs with older Jetpack versions (as I reported this bug happening on my blog) - here are my findings:


Might be that the responsive video feature is breaking things here:

Caches and optimizations had been reset between each try.

digisavvy commented 9 years ago

I'm having the same issue in Jetpack 3.3.2, so I'm just adding info to this issue.

The issue: Infinite scroll renders a number of posts correctly until it gets to a post using the wpvideo shortcode, once rendered infinite scroll ceases to work.

To reproduce I setup infinite scroll w/ Jetpack and got it to where posts were scrolling. Then as a particular post renders using wpvideo I get an error in debug bar: https://www.evernote.com/shard/s2/sh/650da1fc-123f-49b5-9d04-14d47cb1acf1/0b4afa6d8ae2e3048cf68329f6f199f7

And this is the shortcode I used: [wpvideo tFnqC9XQ w=680]

When I remove this shortcode, then all posts render fine.

I tested this on my own theme and on twenty eleven verifying behavior in both themes.

PatTheMav commented 9 years ago

Any news which release a fix might land in?

chrisdc commented 9 years ago

Is it possible we're seeing 2 different errors? Testing in Twenty Fourteen I find the video doesn't appear at all and I get the same error as @digisavvy (in the latest version and 3.1.1). I haven't been able to reproduce the effect in the original screenshot yet.

PatTheMav commented 8 years ago

Hey guys - just FYI this bug is still alive and kicking and breaking all embedded media using media element.js (player controls are inserted twice and the container is misplaced) - any idea in which version a fix might land?

kraftbj commented 8 years ago

Hi @PatTheMav - all of our development takes place on GitHub. We currently have it slated for 4.1, but it doesn't look like we have a patch in place yet, so (as seen by the shifting versions above) it may be pushed again.

allancole commented 8 years ago

Hi @PatTheMav,

I’m looking into what might be causing this issue, so we can get on a path to fixing it. However, on my end, I am unable to reproduce the error. I don’t see the player control/container issues you mentioned, and IS loads successfully before and after a post with a video is loaded.

It’s also unclear what kind of video is creating the error. The original ticket implies a manually uploaded video (ie: [video] shortcode) while digisavvy’s comment mentions VideoPress (ie: [wpvideo] shortcode).

So far, I’ve tried [video], [wpvideo], and [embed] and all seem to load correctly without a problem.

Could you provide some more details on the steps you went through to replicate the issue?

ccing: kraftbj

PatTheMav commented 8 years ago

@allancole - I'll compile some details when I'm home from work - luckily it's 100% reproducible on my blog.

allancole commented 8 years ago

Okay, great! Thanks @PatTheMav

PatTheMav commented 8 years ago

So here's what I could dig up on the quick:

Requirements

Code sources

The post itself consists of the shortcode only (as explained above it's automatically generated by WP's gallery/media functions):

[video mp4="PATH_TO_MP4.mp4" preload="auto"][/video]

Here's the resulting HTML markup delivered by Infinite Scroll:

<div class="entry-content">
    <div class="jetpack-video-wrapper">
        <div class="wp-video" style="width: 640px; ">
            <!--[if lt IE 9]><script>document.createElement('video');</script><![endif]-->
            <video class="wp-video-shortcode" controls="controls" height="360" id="video-1120-1" preload="auto" width="640">
                <source src="PATH_TO_MP4?_=1" type="video/mp4">
                </source>
                <a href="PATH_TO_MP4.mp4">
                    PATH_TO_MP4.mp4
                </a>
            </video>
        </div>
    </div>
    <a class="more-link" href="LINK_TO_POST" rel="bookmark">
        Read more
        <span class="screen-reader-text">
            POST_TITLE
        </span>
    </a>
</div>

And here's the HTML that's actually inserted into the <main> node:

https://gist.github.com/PatTheMav/45c220b2b1654a3c880cdb0e226e26f5

Note how

<span class="mejs-offscreen">
            Video Player
        </span>

is inserted twice and there's two nested divs with ids mep_0 and mep_1 and both sharing the same aria-label and CSS class:

aria-label="Video Player" class="mejs-container svg wp-video-shortcode mejs-video" 

Screenshot

Here's an updated preview of how the result looks in my current theme (the issue persists across themes and active plugins):

2016-06-06_-_infinite_scroll_bug

allancole commented 8 years ago

Thanks for putting this together with such detail @PatTheMav. I’ll try to replicate again with this in mind and report back shortly with my findings.

allancole commented 8 years ago

So I did some initial testing and was able to replicate this issue:

xcbxmfmw0e

However, I can’t seem to reproduce it for every theme I try. I need to do more testing for other variables but of the themes I tested, Twenty Fourteen was the only one missing Responsive Video Support:

add_theme_support( 'jetpack-responsive-videos' );

I’m thinking that might be part of the problem here.

I also noticed that the error repeats the duplicated code as the page scrolls. Essentially, the player markup gets duplicated every time the Infinite Scroll script updates the URL to match the posts pagination (/page/2). This is a choppy example but you can see it happening here:

video-error

I’ll do some more testing soon to see if anything else might give us some clues.

PatTheMav commented 8 years ago

add_theme_support( 'jetpack-responsive-videos' );

Good find - I added it to my current theme (I'm currently using the "Intergalactic" theme), which fixed the displacement of both player elements, but there's still two players on top of each other (and eating up CPU cycles without loading the actual video for some reason). Which also removes any responsiveness from it.

allancole commented 8 years ago

Interesting. I don’t two videos placed on top of each other when looking at themes that use add_theme_support( 'jetpack-responsive-videos' ). However, I did notice that the player design changes when the responsive support is added.

Without add_theme_support( 'jetpack-responsive-videos' ): Markup: https://gist.github.com/allancole/c5f710cb28b30bcc10ced10ba8b6cc7f

With add_theme_support( 'jetpack-responsive-videos' ): Markup https://gist.github.com/allancole/fa9c4deaccc7be809b785a2003c0d637

Notice how the play button and controls look different?

@PatTheMav, which one of these players look like the one you see on your site?

I’m still doing tests on this end, and hadn’t been checking for memory leaks, but I’ll make sure to do that next time.

PatTheMav commented 8 years ago

@allancole - judging by the playback controls it's the second one, but independent of add_theme_support( 'jetpack-responsive-videos' ) being added to the theme or not.

PatTheMav commented 7 years ago

So I see that there is no bugfix planned anymore after three years (and it still breaks all video content on consecutive features) - too bad, but priorities are priorities.. 😕

stale[bot] commented 6 years ago

This issue has been marked as stale. This happened because:

No further action is needed. But it's worth checking if this ticket has clear reproduction steps and it is still reproducible. Feel free to close this issue if you think it's not valid anymore — if you do, please add a brief explanation.

github-actions[bot] commented 3 years ago

This issue has been marked as stale. This happened because:

No further action is needed. But it's worth checking if this ticket has clear reproduction steps and it is still reproducible. Feel free to close this issue if you think it's not valid anymore — if you do, please add a brief explanation.