1094310 - "View Video" context menu doesn't work with mediasource

Status: RESOLVED FIXED

(Toolkit :: Video/Audio Controls, defect)

Description

[Matthew N. [:MattN]]

STR:
1) Load https://www.youtube.com/watch?v=V8SjkDq2ZwI
2) Hold Shift and right-click on the video to see the Firefox context menu
3) Choose "View Video"

Expected result:
* The video opens in a new tab.
* If this isn't technically possible, then the menu item should be disabled

Actual result:
* Nothing happens

Also note that "Copy Video Location" also isn't helpful for the user AFAICT (I'm not familiar with media source extensions) as it gives a URI like:
mediasource:https://www.youtube.com/a1a13094-a089-944a-b867-e7953f0425a6
which I don't think can be navigated to.

Comment 1

[Matthew Gregan [:kinetik]]

(In reply to Matthew N. [:MattN] (focused on Loop) from comment #0)
> * The video opens in a new tab.
> * If this isn't technically possible, then the menu item should be disabled

I don't think it is, because the data feeding media playback is controlled by third-party script in the document.

> Also note that "Copy Video Location" also isn't helpful for the user AFAICT
> (I'm not familiar with media source extensions) as it gives a URI like:
> mediasource:https://www.youtube.com/a1a13094-a089-944a-b867-e7953f0425a6
> which I don't think can be navigated to.

You're right, that's an internal UUID and isn't reusable.

Comment 2

[Matthew Gregan [:kinetik]]

Attachment: p1: disable context menu items for URLs for MSE media

Don't set this.mediaURL for media with an internal "mediasource:" URL, since they can't be reused.  This effectively disables "view video", "copy video location", etc.

Comment 3

[Matthew N. [:MattN]]

Comment on attachment 8517883 [details] [diff] [review]
p1: disable context menu items for URLs for MSE media

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

::: browser/base/content/nsContextMenu.js
@@ +627,5 @@
>        else if (this.target instanceof HTMLCanvasElement) {
>          this.onCanvas = true;
>        }
>        else if (this.target instanceof HTMLVideoElement) {
> +        var mediaURL = this.target.currentSrc || this.target.src;

I think you should use |let| instead of |var| to avoid a possible warning about re-declaring the |mediaURL| variable.

@@ +643,5 @@
>          }
>        }
>        else if (this.target instanceof HTMLAudioElement) {
>          this.onAudio = true;
> +        var mediaURL = this.target.currentSrc || this.target.src;

ditto

@@ +1511,5 @@
>      return !this.focusedWindow.getSelection().isCollapsed;
>    },
>  
> +  isMediaURLReusable: function(aURL) {
> +    return !/^mediasource:/.test(aURL);

I'm guessing we have the same problem with blob URIs too, right?

Comment 4

[Matthew Gregan [:kinetik]]

(In reply to Matthew N. [:MattN] (focused on Loop) from comment #3)
> I think you should use |let| instead of |var| to avoid a possible warning
> about re-declaring the |mediaURL| variable.

Right, thanks!

> I'm guessing we have the same problem with blob URIs too, right?

That seems likely, yeah.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8517883 [details] [diff] [review]
p1: disable context menu items for URLs for MSE media

What Matt said, but also it would be ideal if there was an API on VideoElement (or some ancestor) for determining "has shareable URL" that didn't involve the frontend doing regexp comparisons on string attributes. Is that doable?

Comment 6

[Matthew Gregan [:kinetik]]

Something like that seems possible (and preferable, I agree), but unfortunately I don't have time to work on it right now.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8517883 [details] [diff] [review]
p1: disable context menu items for URLs for MSE media

It will be confusing for the user that some videos have this functionality while others don't. Not sure there's an easy way to address that, though.

Please adjust the patch per Matt's suggestions, r=me with that.

Comment 8

[Matthew Gregan [:kinetik]]

Attachment: bug1094310_v1.patch

Address review comments.

Comment 9

[Matthew Gregan [:kinetik]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ce076d08ad78

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ce076d08ad78