775857 - click-to-play: about:permissions needs to be aware of plugin permission differentiation

Status: RESOLVED INVALID

(Firefox :: General, defect)

Description

[Dirkjan Ochtman (:djc)]

There's already an option in about:permissions, but it doesn't seem to actually reflect my permission for a given domain to always let plugins play.

Comment 1

[John Schoenick [:johns]]

This is an issue because it means clicking "always activate" or "never activate" has no user-findable way to undo

Comment 2

[Alex Keybl [:akeybl]]

(In reply to John Schoenick [:johns] from comment #1)
> This is an issue because it means clicking "always activate" or "never
> activate" has no user-findable way to undo

This is out of scope for FF18 at this point, and can be resolved by simply updating your blocklisted plugin. The general CTP user scenario is less important, since it's not a shipping feature.

Comment 3

[Paul Silaghi, QA [:pauly]]

(In reply to Alex Keybl [:akeybl] from comment #2)
> This is out of scope for FF18 at this point, and can be resolved by simply
> updating your blocklisted plugin.

If the user chooses to "always activate" or "never activate" he will not be able to see the CTP blocked/normal overlay again on that domain. Indeed, everything's good in FF 18, this is a Nightly 20 regression from bug 746374.

Comment 4

[Paul Silaghi, QA [:pauly]]

I come back here. Comment 3 is general CTP related only. For CTP blocklist, I don't see at all any entry for undoing in page info/permissions, even in FF 18b4. Is this known ?

Comment 5

[Alex Keybl [:akeybl]]

(In reply to Paul Silaghi [QA] from comment #4)
> I come back here. Comment 3 is general CTP related only. For CTP blocklist,
> I don't see at all any entry for undoing in page info/permissions, even in
> FF 18b4. Is this known ?

Yep, and comment 2 still stands.

Comment 6

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

My understanding is that this bug is basically about updating about:permissions to handle the changes bug 746374 made (i.e. differentiating plugin permissions based on type). Updating the summary accordingly.

Comment 7

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: wip patch

This is the gist of what I think could/should be done. Note that this doesn't expose the differentiation between regular- and vulnerable-plugin click-to-play. I think that might get a bit cluttered/complicated/confusing. As it is, this UI operates based on whatever a plugin's current status is (if that status changes, the permission could potentially change).

Margaret - since it looks like you started about:permissions, I'd appreciate if you would take a look at this. Thanks!

Comment 8

[:Margaret Leibovic]

Comment on attachment 692477 [details] [diff] [review]
wip patch

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

Sorry for the slow turnaround. I tested this out and it's looking good to me, but we may want to ask Boriss to give this a UX once-over (she did the UX for the original about:permissions), or whoever is the UX person owning click-to-play interactions.

::: browser/components/preferences/aboutPermissions.js
@@ +425,5 @@
>      this._observersInitialized = true;
>      Services.obs.notifyObservers(null, "browser-permissions-preinit", null);
>    },
>  
> +  // XXX copied this from browser-plugins.js - is there a way to share?

I'm not sure if there's a way to share this the way browser-plugins.js is included right now. I've recently been thinking it would be cool to make a plugins JS module that could be shared between mobile/desktop (since there's a lot of copied logic between them), and if that happened, maybe we could load that module here as well to make a call to makeNicePluginName.

@@ +457,5 @@
> +      permissionEntry.setAttribute("mimeType", mimeType);
> +      permissionEntry.setAttribute("permString", permString);
> +      permissionEntry.setAttribute("class", "pluginPermission");
> +      permissionEntries.push(permissionEntry);
> +      this._supportedPermissions.push(permString);

I don't see anywhere else where you use this._supportedPermissions.

@@ +458,5 @@
> +      permissionEntry.setAttribute("permString", permString);
> +      permissionEntry.setAttribute("class", "pluginPermission");
> +      permissionEntries.push(permissionEntry);
> +      this._supportedPermissions.push(permString);
> +      Object.defineProperty(PermissionDefaults, permString, { value: PermissionDefaults.UNKNOWN });

What is permString here? I believe we only use PermissionDefaults to show the UI for the "All Sites" case, where it looks like you're not actually showing specific plugin types, so maybe this line isn't necessary.

@@ +467,5 @@
> +      let labelB = entryB.getAttribute("label");
> +      return labelA < labelB ? -1 : (labelA == labelB ? 0 : 1);
> +    });
> +
> +    for (var permissionEntry of permissionEntries) {

What's the reason for using var here?

@@ +795,5 @@
>     *        e.g. "cookie", "geo", "indexedDB", "popup", "image"
>     */
>    updatePermission: function(aType) {
>      let allowItem = document.getElementById(aType + "-" + PermissionDefaults.ALLOW);
> +    if (allowItem) {

Why do you need to check whether allowItem and denyItem exist now?

@@ +815,3 @@
>          document.getElementById("plugins-pref-item").hidden = false;
> +        document.getElementById("plugins-default-box").hidden = false;
> +        document.getElementById("plugins-box").hidden = true;

Instead of all these calls to hide/show individual elements, is there a way we could do this with CSS? Perhaps toggle an attribute on the plugins-pref-item hbox, then have CSS rules to show/hide things appropriately.

@@ +832,5 @@
>        permissionValue = this._selectedSite.getPermission(aType, result) ?
>                          result.value : PermissionDefaults[aType];
>      }
>  
> +    // XXX force the "plugin-whatever" bindings to instantiate?

Did you notice problems with the binding being created?

Comment 9

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Thanks for the feedback - I have some comments that may explain why I'm doing some of the things I'm doing.

The main issue is that since click-to-play permissions are per-plugin, and we don't know what plugins users will have installed, the actual permission strings used are determined at runtime. So, we can't do things like enumerate them beforehand (as is done, for example, at aboutPermissions.js:383 in defining _supportedPermissions).

(In reply to Margaret Leibovic [:margaret] from comment #8)
> @@ +457,5 @@
> > +      permissionEntry.setAttribute("mimeType", mimeType);
> > +      permissionEntry.setAttribute("permString", permString);
> > +      permissionEntry.setAttribute("class", "pluginPermission");
> > +      permissionEntries.push(permissionEntry);
> > +      this._supportedPermissions.push(permString);
> 
> I don't see anywhere else where you use this._supportedPermissions.

this._supportedPermissions is used by other parts of aboutPermissions.js to handle observing permission changes and updating the UI. This line adds the detected-at-runtime click-to-play plugin permissions to the supported plugins list.

> @@ +458,5 @@
> > +      permissionEntry.setAttribute("permString", permString);
> > +      permissionEntry.setAttribute("class", "pluginPermission");
> > +      permissionEntries.push(permissionEntry);
> > +      this._supportedPermissions.push(permString);
> > +      Object.defineProperty(PermissionDefaults, permString, { value: PermissionDefaults.UNKNOWN });
> 
> What is permString here? I believe we only use PermissionDefaults to show
> the UI for the "All Sites" case, where it looks like you're not actually
> showing specific plugin types, so maybe this line isn't necessary.

Again, permString is a runtime-determined click-to-play plugin permission. I'm adding these permissions to PermissionDefaults mostly so aboutPermissions.js:832 (setting permissionValue) works properly.

> @@ +795,5 @@
> >     *        e.g. "cookie", "geo", "indexedDB", "popup", "image"
> >     */
> >    updatePermission: function(aType) {
> >      let allowItem = document.getElementById(aType + "-" + PermissionDefaults.ALLOW);
> > +    if (allowItem) {
> 
> Why do you need to check whether allowItem and denyItem exist now?

This has to do with my trouble getting the bindings to instantiate. updatePermission can run before the constructors for the pluginPermission bindings I added, in which case allowItem/denyItem will not exist for each plugin type.

> @@ +832,5 @@
> >        permissionValue = this._selectedSite.getPermission(aType, result) ?
> >                          result.value : PermissionDefaults[aType];
> >      }
> >  
> > +    // XXX force the "plugin-whatever" bindings to instantiate?
> 
> Did you notice problems with the binding being created?

I'll keep working on this, though, to see if I can restructure things so updatePermission won't run without those bindings...

Comment 10

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: patch

Ok - I think I've got this in a reviewable state.
In the meantime, I'll talk to Boriss about getting UI/UX feedback.

Comment 11

[Jennifer Morrow [:Boriss] (UX)]

Attachment: Mockup: Artifact around activated window, text change in about:addons

(In reply to David Keeler from comment #10)
> Created attachment 700051 [details] [diff] [review]
> patch
> 
> Ok - I think I've got this in a reviewable state.
> In the meantime, I'll talk to Boriss about getting UI/UX feedback.

This looks great!  I’m just getting one weird artifact issue:

Full outline of plugin area only appears once it has been clicked to be played (can be reproduced on URL http://web.sldrm.video.msn.com/d1/sldrm.html).

Also, could we move the “Click to Play” label three pixels to the left in about:addons and uncapitalize “to” and “play”?

Comment 12

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

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

I'm actually going to re-work this patch a bit, so clearing review for now.

Comment 13

[Jennifer Morrow [:Boriss] (UX)]

Considering that Click-to-play is not (as far as I know) a permission users can set for an individual page, this category in about:permissions should appear in the "All sites" category but not for an individual site.

I also noted in Bug 549697#c48 that rather than a drop-down for each plug-in, we should have a "Click to play" checkbox identical to the one in about:addons.

Comment 14

[Georg Fritzsche [:gfritzsche]]

(In reply to Jennifer Morrow [:Boriss] (Firefox UX) from comment #13)
> Considering that Click-to-play is not (as far as I know) a permission users
> can set for an individual page, this category in about:permissions should
> appear in the "All sites" category but not for an individual site.

A per-site setting is needed, the click-to-play doorhanger is what currently allows you to set per-site permissions.

Comment 15

[Dirkjan Ochtman (:djc)]

Even with the latest CtP revisions, this hasn't been fixed.

Would it help to recategorize as Core: Plugins?

Comment 16

[Benjamin Smedberg]

I doubt it. We removed the plugin UI entirely from about:plugins for Fx24, and this is a very low-priority item.

Comment 17

[Dirkjan Ochtman (:djc)]

So is there any way for me to review and/or revoke plugin whitelist entries?

Comment 18

[Benjamin Smedberg]

Page info > Permissions has that ability, as does the new doorhanger.

Comment 21

[Dão Gottwald [:dao]]

about:permissions has been removed in bug 933917.