1111147 - [EME] Implement context-menu changes for DRM content

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Justin Dolske [:Dolske]]

See user story in bug 1089886.

When the user invokes a the context-menu on DRM content, we should:

1) Disable the "Save Video As…" entry, because it won't actually work. (AIUI this isn't so much about the context menu being a DRM issue, as with actually saving a DRM/MSE stream not working, and even if it did it not being likely to be playable.)

2) Add a content menu section noting that (wording TBD) the media is using DRM, an icon to indicate that's why #1 is disabled, and a Learn More link.

Comment 1

[Justin Dolske [:Dolske]]

Sevaan: We'll need final wording and UI for for this. I also seem to recall that there was discussion about #2 not really being important -- do we still want to do this? It's also kind of weird UI for a content menu.

Comment 2

[Justin Dolske [:Dolske]]

cpearce: Given an arbitrary media element, how does one determine that it's currently playing EME content? Is this possible with existing platform support? (The context menu is running with chrome privs, if that helps.)

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

To detect that a media element is playing EME content with the current platform support, you can listen for an "encrypted" event, followed by a "loadeddata" event. This denotes that we encountered an encrypted file, and then we later became able to play it.

You might want to check that HTMLMediaElement.mediaKeys is non-null before reporting that's we're playing EME content.

You can clear your state related to whether we're playing EME content in the "loadstart" handler, as the same media element could be re-used for non-EME content too.

Comment 4

[Justin Dolske [:Dolske]]

Hmm, an event won't work, because there context menu doesn't maintain state.

Is there an equivalent _property_ on media elements?

I'd want this to work similarly to how the context menu already works on media elements (and, well, everything):

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/nsContextMenu.js#467

[see initMediaPlayerItems()]

Comment 5

[Justin Dolske [:Dolske]]

Sevaan: Still need final UI/wording here, see comment 1.

Comment 6

[Sevaan Franks [:sevaan]]

Kev, ni'ing you re: wording.

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Justin Dolske [:Dolske] from comment #4)
> Hmm, an event won't work, because there context menu doesn't maintain state.
> 
> Is there an equivalent _property_ on media elements?

Do you want to detect whether the media is encrypted, or whether the media is using EME to decrypt encrypted content?

Currently there is no property which denotes whether a media element contains encrypted content, the only way to determine that is to listen for the "encrypted" event.

However you can detect that a media element is *decrypting* encrypted content by checking for the presence of an HTMLMediaElement.mediaKeys attribute, which is basically a proxy to the ContentDecryptionModule underneath. This attribute is set by content JavaScript while setting up an EME session.

However, since the mediaKeys attribute is set by JavaScript, it could be set on a media element that's not got encrypted content (though it would be pointless for content script to do this).

The mediaKeys set on the media element could also be for a keySystem that can't actually decode the resource (though again it would be pointless for content script to do this).

And the mediaKeys set on the media element could not be granted licenses to play the encrypted content by the license server, so the presence of a mediaKeys object by itself does not tell you that an encrypted resource is authorized to be played.

So it's not 100% reliable way to determine whether the content is encrypted.

You could use:

video.mediaKeys != null && video.readyState >= HTMLMediaElement.HAVE_CURRENT_DATA

That means we have a CDM, and it was able to decrypt some content, so it must have been authorized by the DRM system to decode and play content. However, the readyState can drop back to HTMLMediaElement.HAVE_METADATA while we're seeking, so this expression is not 100% reliable, and it doesn't cover the case where the CDM was not authorized to play content, or the video was encrypted and no CDM was provided to decrypt it.

So I recommend you listen on "encrypted", and store a flag to denote whether its been received, and clear it in a "loadstart" handler.

Comment 8

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

(In reply to Chris Pearce (:cpearce) from comment #7)
> So I recommend you listen on "encrypted", and store a flag to denote whether
> its been received, and clear it in a "loadstart" handler.

That sounds overly complicated. Why can't we introduce a chrome-only property on media elements for this? Or expose a isMediaElementDRMed(element) utility method somewhere?

Comment 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #8)
> (In reply to Chris Pearce (:cpearce) from comment #7)
> > So I recommend you listen on "encrypted", and store a flag to denote whether
> > its been received, and clear it in a "loadstart" handler.
> 
> That sounds overly complicated. Why can't we introduce a chrome-only
> property on media elements for this?

We could, but the logic to do this in Gecko would be equivalent, for the reasons I outlined above, so it seems simpler to implement it in JavaScript.

I don't understand why the context menu or video controls can't set a listener on the media element to track this. Can you elaborate why this won't work?

Comment 10

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

(In reply to Chris Pearce (:cpearce) from comment #9)
> We could, but the logic to do this in Gecko would be equivalent, for the
> reasons I outlined above, so it seems simpler to implement it in JavaScript.

Implementing it in core code (i.e. closer to the core "are we using DRM" decision) will ensure that it won't break down the road. It's never a good idea for the front-end to make indirect guesses about things the platform already knows (or should know).

Wouldn't it be pretty easy to have MP4Reader::ReadMetadata set or clear a "using EME" flag on HTMLMediaElement (via mDecoder->GetOwner()), depending on mDemuxer->Crypto().valid?

Comment 11

[Chris Pearce [:cpearce (Not reading bugmail)]]

OK, that makes sense. Thanks for elaborating. We'll get a chrome only chromeonly isEncrypted attribute added. Filed bug 1124491.

Comment 12

[Sevaan Franks [:sevaan]]

Matej, could you look at the wording for this? When a user right-clicks on DRM-protected content (to save a video, or try interact with is in some way) we want to display a little message in the context menu that states that some features and functionality will be missing. Mockup of context menu: http://cl.ly/image/1X471R2N1o2Q

Thanks!

Comment 13

[Matej Novak [:matej]]

There seems to be work happening on DRM in a number of different bugs, each involving different people. I'm adding Javaun here so we can align everything. This one looks similar to another string I'm working on. Can this one wait until that one is locked?

Comment 14

[Sevaan Franks [:sevaan]]

Sure thing, Matej. I'll just upload a mockup with Lorem Ipsum for now.

Comment 15

[Sevaan Franks [:sevaan]]

Attachment: EME Context Menus

Context menus attached. Mousing over works like any other menu item, except it takes user to a page to learn more.

Comment 16

[:Gijs (he/him)]

(In reply to Sevaan Franks [:sevaan] from comment #15)
> Created attachment 8556022 [details]
> EME Context Menus
> 
> Context menus attached. Mousing over works like any other menu item, except
> it takes user to a page to learn more.

Can we avoid the tiny text and just have it be "learn more about DRM" or something? This has platform compat (different font sizes) and accessibility issues written all over it unfortunately. :-(

Comment 17

[Sevaan Franks [:sevaan]]

Is it possible to change the colour so we can differentiate it from other menu items?

Comment 18

[:Gijs (he/him)]

(In reply to Sevaan Franks [:sevaan] from comment #17)
> Is it possible to change the colour so we can differentiate it from other
> menu items?

On OS X and on the default (XP luna, win7 aero, win8 non-high-contrast) windows themes, yes. Elsewhere, no, because we can't really predict the background color of the menu and therefore can't sanely pick a different one.

Why do we need to? It has an icon, unlike anything else in the menu, and gets the end spot with a separator - does it not stand out enough as-is? And why do we think it needs to stand out more? :-)

Comment 19

[Sevaan Franks [:sevaan]]

Attachment: EME Context Menus 2

Updated to include Gjis's suggestions for comparison.

Philipp, what do you think?

Comment 20

[:Gijs (he/him)]

(In reply to Sevaan Franks [:sevaan] from comment #19)
> Created attachment 8556050 [details]
> EME Context Menus 2
> 
> Updated to include Gjis's suggestions for comparison.
> 
> Philipp, what do you think?

Comment 21

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Do we need to stick to the default font size in the OS X version?
That size hasn't really been made for longer texts.

Also, do we know what the actual message here will be — in case it gets much longer/shorter which would probably influence the styling.

Comment 22

[Sevaan Franks [:sevaan]]

:Matej is locking a similar string before tackling this one. But yes, the length of the message may influence the design.

Comment 23

[:Gijs (he/him)]

My suggestion to use "Learn more about DRM..." was made specifically to avoid the need to wrap and/or have longer+smaller text.

Besides all the serious accessibility and implementation issues with the originally proposed solution / user story, I basically don't see the value proposition of trying to explain a very complex issue in a single sentence in small font, when that information is available in a much more legible and complete manner by actually clicking/using the menuitem. We also already provide a sentence-long bit of info in the doorhanger.

Finally, menuitems are the wrong vehicle for providing information - they're meant for things you can *do*, which "Learn more about DRM" is, but a plain description of the fact that there's DRM on this video wouldn't be.

Kev, any chance we can rethink what we want this to do?

Comment 24

[Javaun Moradi [:javaun]]

I'm taking this one off Kev's plate. We just locked the door hanger text on Friday. I'll get with Matej. We'll try to get it shorter and see how that impacts other goals.

Comment 25

[Justin Dolske [:Dolske]]

+1 to everything in comment 23. A simple "Learn more about DRM" entry avoids a number of issues, and seems like a better cost/benefit for this relatively-hidden UI.

Comment 26

[Matej Novak [:matej]]

(In reply to Justin Dolske [:Dolske] from comment #25)
> +1 to everything in comment 23. A simple "Learn more about DRM" entry avoids
> a number of issues, and seems like a better cost/benefit for this
> relatively-hidden UI.

That works for me.

Comment 27

[Javaun Moradi [:javaun]]

This works. I'll send this up the chain as the recommendation.

Comment 28

[:Gijs (he/him)]

(In reply to Javaun Moradi [:javaun] from comment #27)
> This works. I'll send this up the chain as the recommendation.

OK. Does this mean this is ready to be worked on, or do we need to wait for confirmation from "up the chain"? :-)

(and if so, is it possible that a decision will be made on or before Tuesday 10th, so we can pick this up in the next iteration if not now?)

Comment 29

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

We should not block on implementing it, it can be tweaked later.

Comment 30

[:Gijs (he/him)]

Sevaan, am I right in thinking we want this to use the same icon as the notification one in the urlbar?

Comment 31

[:Gijs (he/him)]

Attachment: update nsContextMenu for EME,

This assumes the icon is OK, and that the design by Sevaan is intentional in hiding the myriad of disabled items, leaving only 'save video' as an example of what is disabled (it seems to me that there's little point in showing all the other items (send/share/cast audio/video, save snapshot) disabled with the same icon).

Comment 32

[Florian Quèze [:florian]]

Comment on attachment 8561801 [details] [diff] [review]
update nsContextMenu for EME,

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

::: browser/base/content/browser-context.inc
@@ +220,5 @@
>                  accesskey="&viewImageDescCmd.accesskey;"
>                  oncommand="gContextMenu.viewImageDesc(event);"
>                  onclick="checkForMiddleClick(this, event);"/>
>        <menuitem id="context-savevideo"
> +                class="menuitem-iconic"

I assume you have checked that this:
- doesn't cause any unindented side effects on any of the 3 supported platforms in the case where we are not showing the icon.
- works on Linux (I remember the icon behavior being slightly different there; but it may have been only for menulist items).

Also, I don't see the icon on this disabled 'save as' menuitem in the mockups attached to this bug. There's an icon in the mockup linked from comment 12, but I don't think it was meant to be final. Is there a final UX mockup that wasn't attached?

::: browser/themes/shared/contextmenu.inc.css
@@ +84,5 @@
> +  list-style-image: url("chrome://browser/skin/drm-icon.svg#chains");
> +}
> +
> +menupopup[drmmedia=true] #context-savevideo,
> +menupopup[drmmedia=true] #context-saveaudio {

Would a child selector (>) work here?

Comment 33

[:Gijs (he/him)]

Attachment: update nsContextMenu for EME,

On OSX the menuitem-iconic thing indents if there's no icon, so I've made it dynamic. I tried checking Ubuntu, but I can't work out how to show images in GTK menus (there's meant to be a gconf setting for it, changing it seems to have no effect on my machine... not sure what I'm doing wrong, if anything). In any case, when the icon is not shown, both when drm is off and when it's not shown because GTK, the menu itself looks fine. I also adjusted the child selector. I'm going to assume Sevaan can clarify the use of the icon for the save item (or not) and/or whether the icon is final. Thanks for the quick review. :-)

Comment 34

[Florian Quèze [:florian]]

(In reply to :Gijs Kruitbosch from comment #33)

> I can't work out how to show images in GTK menus

The magic bit of CSS hiding icons from GTK menus is http://hg.mozilla.org/mozilla-central/annotate/2cb22c058add/toolkit/content/xul.css#l386

It looks like you can workaround it by setting the menuitem-with-favicon class.

Comment 35

[:Gijs (he/him)]

(In reply to Florian Quèze [:florian] [:flo] from comment #34)
> (In reply to :Gijs Kruitbosch from comment #33)
> 
> > I can't work out how to show images in GTK menus
> 
> The magic bit of CSS hiding icons from GTK menus is
> http://hg.mozilla.org/mozilla-central/annotate/2cb22c058add/toolkit/content/
> xul.css#l386
> 
> It looks like you can workaround it by setting the menuitem-with-favicon
> class.

Yes, but that's not the point - we should be obeying that line of code for these icons (I'm pretty sure that we decided bookmarks/history favicons are a special case - that's what that class is used for), and it should be possible to configure GTK to turn that feature on/off. :-\

Comment 36

[Florian Quèze [:florian]]

Comment on attachment 8562085 [details] [diff] [review]
update nsContextMenu for EME,

Setting the flag to r+ with the assumption that you'll wait for Sevaan's answer about whether the icons in the 'Save as' menu items are actually wanted.

Comment 37

[Sevaan Franks [:sevaan]]

(In reply to :Gijs Kruitbosch from comment #30)
> Sevaan, am I right in thinking we want this to use the same icon as the
> notification one in the urlbar?

Yes, it will be the same icon.

(In reply to :Gijs Kruitbosch from comment #31) 
> This assumes the icon is OK, and that the design by Sevaan is intentional in
> hiding the myriad of disabled items, leaving only 'save video' as an example
> of what is disabled (it seems to me that there's little point in showing all
> the other items (send/share/cast audio/video, save snapshot) disabled with
> the same icon).

Okay, so I have been chatting with Gijs on IRC about this and would like to solicit other opinions. In the original mockup I hid items just for illustrative purposes, and included an icon next to the disabled Save Video button, which was to act as an asterisk pointing people to the footer below. However, this was not meant to be an instruction to hide everything...however, it does look much neater this way.

Question 1: Should we hide everything, except the disabled Save Video As item?

Question 2: Do we need an icon next to all affected menu items? Or will the icon just in the footer (like in the current attached mockups) suffice?

Comment 38

[Sevaan Franks [:sevaan]]

I have a meeting with phlsa to discuss this later this morning too.

Comment 39

[:Gijs (he/him)]

(In reply to Sevaan Franks [:sevaan] from comment #37)
> (In reply to :Gijs Kruitbosch from comment #30)
> > Sevaan, am I right in thinking we want this to use the same icon as the
> > notification one in the urlbar?
> 
> Yes, it will be the same icon.
> 
> (In reply to :Gijs Kruitbosch from comment #31) 
> > This assumes the icon is OK, and that the design by Sevaan is intentional in
> > hiding the myriad of disabled items, leaving only 'save video' as an example
> > of what is disabled (it seems to me that there's little point in showing all
> > the other items (send/share/cast audio/video, save snapshot) disabled with
> > the same icon).
> 
> Okay, so I have been chatting with Gijs on IRC about this and would like to
> solicit other opinions. In the original mockup I hid items just for
> illustrative purposes, and included an icon next to the disabled Save Video
> button, which was to act as an asterisk pointing people to the footer below.
> However, this was not meant to be an instruction to hide
> everything...however, it does look much neater this way.
> 
> Question 1: Should we hide everything, except the disabled Save Video As
> item?

To be clear, 'everything' here is:
- "share video" (needs a media URL that works on its own, which is never the case for DRM'd video)
- "send video" (ditto)
- "view video" (ditto)
- "copy video url" (ditto)
- "copy audio url" (ditto)
- "save video snapshot" (disabled in core code for DRM'd video)

Looking at the patch, it looks like I accidentally included the casting video submenu too - that should probably stay visible (although I'm not sure if it'll work or not - I can't test that).

> Question 2: Do we need an icon next to all affected menu items? Or will the
> icon just in the footer (like in the current attached mockups) suffice?

I like the idea of having the icon for the visible "save video as" thing as the prime (and most easily understood) example of what the DRM restricts. If you're watching $latesthollywoodmovie on $sitewithdrm, you don't get to save it to your harddisk separately.

Comment 40

[Justin Dolske [:Dolske]]

(In reply to :Gijs Kruitbosch from comment #39)

> > Question 1: Should we hide everything, except the disabled Save Video As
> > item?
> 
> To be clear, 'everything' here is:
> - "share video" (needs a media URL that works on its own, which is never the
> case for DRM'd video)
> - "send video" (ditto)
> - "view video" (ditto)
> - "copy video url" (ditto)
> - "copy audio url" (ditto)
> - "save video snapshot" (disabled in core code for DRM'd video)

My default position would be that, AIUI, general good-UI practice is to disable items instead of hiding them (when they're not available for use but normally would be). It's also worth noting that we should be doing the same thing for general (non-DRM) MSE (Media Source Extensions), since there is again no single URL to represent the media. (And looks like the existing checks already work; on YouTube I get a context menu with these items disabled.)

I think we should probably just do the same thing here, with the only subtle difference being that Save Video gets an icon, and Save Snapshot is also disabled. But I'm open to changing that if we feel strongly that hiding the entries is important for clarity.

Out of curiousity... Taking a look at the stats on https://bwinton.github.io/d3Experiments/contextmenu.html# (which are old and crusty, but still roughly useful), it looks like the usage breakdown is:
 - share video: 14 clicks
 - send video: 5 clicks
 - view video: 494 clicks
 - copy video url: 920 clicks
 - copy audio url: 90 clicks
 - save snapshot: 113 clicks
also,
 - save video: 8313 clicks
 - save audio: 3443 clicks

That implies to me that Save, Copy URL, and View Video are fairly commonly used (in relative terms), whereas the others are not.


> > Question 2: Do we need an icon next to all affected menu items? Or will the
> > icon just in the footer (like in the current attached mockups) suffice?
> 
> I like the idea of having the icon for the visible "save video as" thing as
> the prime (and most easily understood) example of what the DRM restricts.

Concur, I think it's fine just to have the one icon.

Comment 41

[Sevaan Franks [:sevaan]]

Okay, thanks for your input. I like the proposed solution. Let's do it.

Comment 42

[Justin Dolske [:Dolske]]

Oh, I slightly misread Gijs's answer to #2. I think I don't have a strong opinion on if the icon should be next to Save Video or just the footer. Either seems acceptable to me.

Comment 43

[Sevaan Franks [:sevaan]]

Attachment: EME Context Menus

I spoke with :phlsa about this, and we are of the opinion that we don't need the icon next to Save Video. After mocking it up with the new text (Learn more about DRM) it seems confusing to have it.

Updated mockup attached.

Comment 44

[:Gijs (he/him)]

Comment on attachment 8562085 [details] [diff] [review]
update nsContextMenu for EME,

I'll upload an updated patch tomorrow.

Comment 45

[:Gijs (he/him)]

Attachment: update nsContextMenu for EME,

Now with only an icon for 'learn more' and just disabling things rather than hiding them.

Comment 46

[:Gijs (he/him)]

Note that per discussions with the platform team, EME will be restricted to mediasource-only sources "soon", and so the existing checks for a reusable mediaURL should be enough to ensure save/share/send/cast items are disabled.

Comment 47

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/6280b92abfcb

Comment 48

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6280b92abfcb

Comment 49

[:Gijs (he/him)]

Uplifted in bug 1136165.

Comment 50

[Bogdan Maris, Desktop QA]

Did some exploratory testing around this on Aurora and Nightly under Windows 7 64-bit and Mac OS X 10.9.5 and found one new issues that was added to the dependences.

Comment 51

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

According to Lawrence: "EME will not be enabled for testing in 37 and no further EME related changes will be approved for 37. EME testing on Beta will now target 38 Beta 1.". Removing the qe-verify flag as this was already verified for Firefox 38 and won't be verified for Firefox 37.

Comment 52

[mcdavis941 (sporadically reading bugmail)]

- (2015-03-05) (Fx37) https://hg.mozilla.org/releases/mozilla-beta/rev/38ce715f4de4