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

Parameters in a YouTube URL are not transferred to the amp-youtube component #4518

Closed
pierlon opened this issue Apr 3, 2020 · 3 comments · Fixed by #6423
Closed

Parameters in a YouTube URL are not transferred to the amp-youtube component #4518

pierlon opened this issue Apr 3, 2020 · 3 comments · Fixed by #6423
Assignees
Labels
Bug Something isn't working Changelogged Whether the issue/PR has been added to release notes. Embeds Groomed P1 Medium priority Punted WS:Core Work stream for Plugin core
Milestone

Comments

@pierlon
Copy link
Contributor

pierlon commented Apr 3, 2020

Bug Description

The URL http://www.youtube.com/watch?v=0zM3nApSvMg#t=0m10s contains the t fragment identifier that denotes the video should start at the 10s mark. When it is inserted via the YouTube block in a post, the fragment is kept as seen in the markup below:

<iframe title="Old Spice Pro-Strength Commercial - Neil Patrick Harris" width="580" height="435" src="https://www.youtube.com/embed/0zM3nApSvMg?start=10&amp;feature=oembed" frameborder="0" allow="accelerometer; autoplay; encrypted-media; gyroscope; picture-in-picture" allowfullscreen="" data-origwidth="580" data-origheight="435" style="width: 580px; height: 435px;"></iframe>

The AMP component does not include this parameter, however:

<amp-youtube data-videoid="0zM3nApSvMg" layout="responsive" width="580" height="435" title="Old Spice Pro-Strength Commercial - Neil Patrick Harris" class="i-amphtml-element i-amphtml-layout-responsive i-amphtml-layout-size-defined i-amphtml-media-component i-amphtml-video-interface i-amphtml-layout" i-amphtml-layout="responsive" style="--loader-delay-offset:1ms !important;"><i-amphtml-sizer style="padding-top: 75%;"></i-amphtml-sizer><a placeholder="" href="http://www.youtube.com/watch?v=0zM3nApSvMg#t=0m10s" class="amp-hidden"><amp-img src="https://i.ytimg.com/vi/0zM3nApSvMg/hqdefault.jpg" layout="fill" object-fit="cover" alt="Old Spice Pro-Strength Commercial - Neil Patrick Harris" class="amp-wp-enforced-sizes i-amphtml-element i-amphtml-layout-fill i-amphtml-layout-size-defined i-amphtml-layout" i-amphtml-layout="fill" i-amphtml-auto-lightbox-visited=""><noscript><img src="https://i.ytimg.com/vi/0zM3nApSvMg/hqdefault.jpg" alt="Old Spice Pro-Strength Commercial - Neil Patrick Harris"></noscript><img decoding="async" alt="Old Spice Pro-Strength Commercial - Neil Patrick Harris" src="https://i.ytimg.com/vi/0zM3nApSvMg/hqdefault.jpg" class="i-amphtml-fill-content i-amphtml-replaced-content" style="object-fit: cover;"></amp-img></a><iframe frameborder="0" allowfullscreen="" src="https://www.youtube.com/embed/0zM3nApSvMg?enablejsapi=1&amp;amp=1&amp;playsinline=1" class="i-amphtml-fill-content" allow="autoplay;"></iframe></amp-youtube>

The seek time is not transferred over to the AMP component and so it differs from the non-AMP variant.

Expected Behaviour

The YouTube AMP component should support parameters from the original YouTube embed.

Steps to reproduce

  1. Insert a YouTube block with the following URL: http://www.youtube.com/watch?v=0zM3nApSvMg#t=0m10s

  2. Visit the non-AMP page and verify the video starts at the 10s mark

  3. Visit the AMP page and verify the video does not start at the 10s mark

Screenshots

Additional context

  • WordPress version:
  • Plugin version:
  • Gutenberg plugin version (if applicable):
  • AMP plugin template mode:
  • PHP version:
  • OS:
  • Browser: [e.g. chrome, safari]
  • Device: [e.g. iPhone6]

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

Implementation brief

QA testing instructions

Demo

Changelog entry

@pierlon pierlon added Bug Something isn't working Embeds labels Apr 3, 2020
@westonruter westonruter added this to the v1.6 milestone Apr 3, 2020
@kmyram kmyram added the P0 High priority label Apr 14, 2020
@kmyram kmyram modified the milestones: v1.6, Sprint 28 Apr 14, 2020
@westonruter westonruter added P1 Medium priority and removed P0 High priority labels Apr 16, 2020
@kmyram kmyram modified the milestones: Sprint 28, v1.6 May 27, 2020
@westonruter westonruter removed this from the v1.6 milestone Jun 15, 2020
@kmyram kmyram added the WS:Core Work stream for Plugin core label Aug 5, 2020
@westonruter
Copy link
Member

This may make sense to work on in conjunction with this Jetpack issue: Automattic/jetpack#16946

I think that Jetpack is incorrectly serving an iframe when it should be serving an amp-youtube element, but it may be serving an iframe currently because we're not correctly transforming non-singular videos like playlists.

@westonruter westonruter added this to the v2.1 milestone Sep 28, 2020
@westonruter
Copy link
Member

In fact, there's probably a lot of conversion code that we can scrape from Jetpack to improve our handling of raw embed conversions: https://github.com/Automattic/jetpack/blob/master/modules/shortcodes/youtube.php

@delawski
Copy link
Collaborator

delawski commented Dec 1, 2021

QA Passed

✅ The YouTube video starts at the specified time mark (10s) on AMP and non-AMP version.

Tested on AMP Version 2.2.0-alpha-20211123T020732Z-5405daa

@delawski delawski self-assigned this Dec 1, 2021
@westonruter westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Dec 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Changelogged Whether the issue/PR has been added to release notes. Embeds Groomed P1 Medium priority Punted WS:Core Work stream for Plugin core
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants