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

RESOLVED INVALID

Status

()

defect
RESOLVED INVALID
7 years ago
4 years ago

People

(Reporter: djc, Unassigned)

Tracking

(Depends on 1 bug)

unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox18- wontfix, firefox19- affected, firefox20- affected, firefox-esr17- affected)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

7 years ago
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.

Updated

7 years ago
This is an issue because it means clicking "always activate" or "never activate" has no user-findable way to undo
(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.
(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.
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 ?
(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.
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.
Blocks: 746374
Summary: Click to play whitelist should be hooked up to about:permissions → click-to-play: about:permissions needs to be aware of plugin permission differentiation
Component: Plug-ins → General
Product: Core → Firefox
Version: 15 Branch → unspecified
Posted patch wip patch (obsolete) — Splinter Review
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!
Attachment #692477 - Flags: feedback?(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?
Attachment #692477 - Flags: feedback?(margaret.leibovic) → feedback+
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...
Posted patch patchSplinter Review
Ok - I think I've got this in a reviewable state.
In the meantime, I'll talk to Boriss about getting UI/UX feedback.
Attachment #692477 - Attachment is obsolete: true
Attachment #700051 - Flags: review?(margaret.leibovic)
(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 on attachment 700051 [details] [diff] [review]
patch

I'm actually going to re-work this patch a bit, so clearing review for now.
Attachment #700051 - Flags: review?(margaret.leibovic)
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.
Blocks: 840761
(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.
(Reporter)

Comment 15

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

Would it help to recategorize as Core: Plugins?
I doubt it. We removed the plugin UI entirely from about:plugins for Fx24, and this is a very low-priority item.
Assignee: dkeeler → nobody
(Reporter)

Comment 17

6 years ago
So is there any way for me to review and/or revoke plugin whitelist entries?
Page info > Permissions has that ability, as does the new doorhanger.

Updated

6 years ago
Duplicate of this bug: 875162
Duplicate of this bug: 917174
about:permissions has been removed in bug 933917.
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.