1258053 - Consider to be more lenient to "invalid" Youtube flash embed URLs

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Masatoshi Kimura [:emk]]

+++ This bug was initially created as a clone of Bug #1240471 +++

(In reply to Chris Peterson [:cpeterson] from bug 1240471 comment #60)
> YouTube's Flash video player will happily load the
> invalid URLs. So all the URL parameters (in this example: "&hl=pl&fs=1") are
> ignored.

No, YouTube's Flash video player does NOT ignore the "invalid" URL parameters. It parses the "invalid" parameters somehow. For example, <https://www.youtube.com/v/7LcUOEP7Brc&start=35> will start playback at 0:35.
Rather, I'm not sure these types of URL should be called "invalid". Nothing prevents web apps from taking parameters from the path component. It is up to apps how the URL will be parsed.

> Perhaps we could append "?fs=1" to the new URL only in the case where we
> truncate the invalid query string. Then we wouldn't need to scan the
> original URL to see if the new URL needs "?fs=1" or "&fs=1". We would leave
> valid query strings untouched, respecting the web page's original intention.

How difficult is it to replace the first "&" to "?" if "?" is not found in the URL?

Comment 1

[Masatoshi Kimura [:emk]]

To spec lawyers: the spec explicitly allows path components to have an app-specific structure.
https://tools.ietf.org/html/rfc3986#section-3.3
>   Aside from dot-segments in hierarchical paths, a path segment is
>   considered opaque by the generic syntax.  URI producing applications
>   often use the reserved characters allowed in a segment to delimit
>   scheme-specific or dereference-handler-specific subcomponents.  For
>   example, the semicolon (";") and equals ("=") reserved characters are
>   often used to delimit parameters and parameter values applicable to
>   that segment.  The comma (",") reserved character is often used for
>   similar purposes.  For example, one URI producer might use a segment
>   such as "name;v=1.1" to indicate a reference to version 1.1 of
>   "name", whereas another might use a segment such as "name,1.1" to
>   indicate the same.  Parameter types may be defined by scheme-specific
>   semantics, but in most cases the syntax of a parameter is specific to
>   the implementation of the URI's dereferencing algorithm.

Comment 2

[Andrew Overholt [:overholt]]

qDot and Chris, is this on your radars?

Comment 3

[Chris Peterson [:cpeterson]]

AFAIK, we don't have plans to work on this any time soon. This is not a release blocker.

Kyle worked around the problem reported in bug 1240471 by falling back to YouTube's Flash player when we detect a Flash embed with a URL that would break YouTube's HTML5 player *and* the user has Flash installed. If they don't have Flash, then we rewrite the embed to use YouTube's HTML5 player. The only problem then is that the fullscreen button won't work.

Comment 4

[arni2033]

STR (bug 1240471 comment 53):
 1. Open the following url in Nightly
 data:text/html,<embed src="http://www.youtube.com/v/eoUf1bbMTa8&hl=pl&fs=1" allowfullscreen="true">
 2. Click fullscreen button at the bottom of the video

AR:  Video shows inscription "fullscreen is not available"
ER:  Video should switch to fullscreen

Comment 5

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Based on Chris's assessment in comment 3, this is not likely to be included as a dot-release ride-along. It is not a wontfix for Fx47.

Comment 6

[Masatoshi Kimura [:emk]]

Attachment: patch

Comment 7

[Masatoshi Kimura [:emk]]

Attachment: String changes

I separated out string changes because I consider uplift. The old string is fine enough.

Comment 8

[Masatoshi Kimura [:emk]]

Hm, fullscreen is not working on Stalker.pl even with this patch. The allowfullscreen attribute on embed is not an HTML5 attribute but a parameter to be passed to Plug-ins (i.e. Flash). So it has no ability to enable fullscreen.

We will have to replace the embed element to an iframe element to enable fullscreen. But we should work on it in another bug. This patch is useful in itself because it will no longer discard other parameters such as "hl=pl", "start=35", etc.

Comment 9

[Chris Peterson [:cpeterson]]

Comment on attachment 8763864 [details] [diff] [review]
String changes

Review of attachment 8763864 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, Masatoshi. The "YouTube" string changes LGTM.

Comment 10

[Chris Peterson [:cpeterson]]

Comment on attachment 8763863 [details] [diff] [review]
patch

Review of attachment 8763863 [details] [diff] [review]:
-----------------------------------------------------------------

Kyle should review the embed rewriting logic.

Comment 11

[Masatoshi Kimura [:emk]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3491b435ec484be70ff6f5dcaf023f7e25b3d1d7
Bug 1258053 - Convert YouTube Flash embed URLs that contain params in the path components. r=kmachulis

https://hg.mozilla.org/integration/mozilla-inbound/rev/005cb08c63da6a0d9803ab4d437076147259d4cc
Bug 1258053 - String changes. r=cpeterson

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3491b435ec48
https://hg.mozilla.org/mozilla-central/rev/005cb08c63da

Comment 13

[Masatoshi Kimura [:emk]]

Comment on attachment 8763863 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]: 769117, 1240471
[User impact if declined]: Some embedded YouTube movie will not work correctly. This patch is a prerequisite for bug 1282038.
[Describe test coverage new/current, TreeHerder]: Automated test baked on m-c
[Risks and why]: Low, only affects already broken YouTube embeds
[String/UUID change made/needed]: none. string changes are separated into another bug. This patch does not rely on the string change.

Comment 14

[Chris Peterson [:cpeterson]]

I don't think we should uplift this fix to Beta 48. This code is tricky because it is analyzing some broken YouTube embed HTML and earlier versions of our embed rewriting patches caused some regressions. I think can uplift this to Aurora 49 and let it ride the trains from there.

Comment 15

[Masatoshi Kimura [:emk]]

Comment on attachment 8763863 [details] [diff] [review]
patch

(In reply to Chris Peterson [:cpeterson] from comment #14)
> I don't think we should uplift this fix to Beta 48. This code is tricky
> because it is analyzing some broken YouTube embed HTML and earlier versions
> of our embed rewriting patches caused some regressions. I think can uplift
> this to Aurora 49 and let it ride the trains from there.

OK. Canceling the uplift request to beta.

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8763863 [details] [diff] [review]
patch

Better YouTube embedding sounds good. We still have a couple of weeks left in aurora. Let's try it.

Comment 17

[Carsten Book [:Tomcat]]

has problems applying to aurora, can you take a look ?

Comment 18

[Masatoshi Kimura [:emk]]

Attachment: patch for aurora

Comment 19

[Masatoshi Kimura [:emk]]

Attachment: patch for aurora

Sorry, the previous patch still had a merge error.

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/bf67cbd8360a