search

hypothesis / client

The Hypothesis web-based annotation client.
Other
641 stars 198 forks source link

youtube url auto-activation ignores start time #164

Closed judell closed 6 years ago

judell commented 7 years ago

Steps to reproduce

  1. Post this link into a new annotation and save the annotation: https://youtu.be/nffPZ1kLDHM?t=95

Expected behaviour

Video begins at the 95-second mark

Actual behaviour

Video begins at the 0-second mark

judell commented 7 years ago

Bump: https://hypothesis.zendesk.com/agent/tickets/810

lyzadanger commented 6 years ago

At present, the embed-generating logic for both YouTube share links (cited example in description here is a share link) and YouTube watch links (which look like: https://www.youtube.com/watch?v=e_-1aFNlb1s) chuck out all URL parameters except for—relevant to watch links only—the v parameter, which is the video's UID. Ergo the t parameter is ignored.

Obviously we need to change it not to do that, but it opens up a few questions/options, as there are a slew of other YouTube parameters that can be communicated via URL param:

  1. We could implement for start-time support specifically (t param), which would technically satisfy this issue, but feels a little rickety and all other valid YouTube URL params would still be ignored, or
  2. We could leave all params intact. This is the easiest approach and would allow support of other YouTube URL params. Do consider, though, that it means that autoplay=1, for example, would be allowed. It may also raise other security or irritation concerns I'm not thinking of at this moment, or
  3. We could retain whitelisted params, but that may well raise more maintenance and complexity concerns than we want to deal with here.

Opinions? /cc @robertknight

judell commented 6 years ago

Note that we have, waiting in the wings (http://steelwagstaff.info/adding-interactivity-to-web-annotation/) a generalized iframe embedder.

lyzadanger commented 6 years ago

@judell I believe this should be comfortably independent of that—I reviewed the pertinent commit and it appears to leave YouTube- and Vimeo- specific embedding alone. That is, making changes to the embedding of supported videos shouldn't hinder the arbitrary/H5P embedding support.

judell commented 6 years ago

Yep, agreed. That was just to alert you to the existence of a complementary (and really interesting) capability. Beyond H5P, it's an enabler of things like this: http://jonudell.net/h/using-hypothesis-and-check-together.mp4

robertknight commented 6 years ago

@lyzadanger - I like the suggestion of a whitelist myself. Looking at the params described here, there are several ("enablejsapi", "origin", "autoplay", "widget_referrer") which I'm fairly sure we wouldn't want to get from user-provided links.

judell commented 6 years ago

See also https://via.hypothes.is/http://boffosocko.com/2018/01/07/reply-to-annotating-web-audio-by-jon-udell/

tl;dr It would be nice to honor media fragments when we autolink mp3/mp4 urls (which I did not realize happens!)

lyzadanger commented 6 years ago

@judell Are these things being tracked in another issue somewhere? I'd hate for them to get lost when this issue is resolved (specific to the YouTube params).

judell commented 6 years ago

Not yet, but I'll start one. The media-frag thing just came up in discussion last night.