821892 - click-to-play: "Page Info" -> Permissions needs to be aware of plugin permission differentiation

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

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

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

Same as bug 775857, but for the "Page Info" -> Permissions tab.

Comment 1

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

Attachment: wip patch

This is a work-in-progress patch very similar to that in bug 775857.
I don't really know who to ask for feedback on this, but maybe Dao would have some insight or know who else to ask. Thanks!

Comment 2

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

Attachment: patch

Comment 3

[Justin Dolske [:Dolske]]

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

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

::: browser/base/content/pageinfo/pageInfo.xml
@@ +40,5 @@
> +          <xul:radio anonid="allow" label="&permAllow;"/>
> +          <xul:radio anonid="block" label="&permBlock;"/>
> +        </xul:radiogroup>
> +      </xul:hbox>
> +    </content>

Adding more XBL goop for this small bit of UI seem like overkill, and furthers our technical debt. I think you'd be better off moving the JS into pageinfo.js, and have a function to construct the UI via createElement() calls. Or maybe stuff it into a pageinfo.xul as a hidden "template", which you then hbox.cloneNode(true) to duplicate and customize for each row.

Comment 4

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

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

Ok - sounds good. Clearing review for now.

Comment 5

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

Attachment: patch v2

Now with 50% less goop!

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 707280 [details] [diff] [review]
patch v2

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

::: browser/base/content/pageinfo/pageInfo.xul
@@ +376,5 @@
>          <vbox class="permission" id="permPluginsRow">
>            <label class="permissionLabel" id="permPluginsLabel"
>                   value="&permPlugins;" control="pluginsRadioGroup"/>
> +          <hbox id="permPluginTemplate" role="group" aria-labelledby="permPluginsLabel" align="baseline">
> +            <label/>

Why does align="baseline" need to be here?

::: browser/base/content/pageinfo/permissions.js
@@ +239,5 @@
>    }
>  }
> +
> +// XXX copied this from browser-plugins.js - is there a way to share?
> +function makeNicePluginName(aName) {

Can you add this as a method on nsIPluginHost?

@@ +269,5 @@
> +    let permissionEntry = permPluginTemplate.cloneNode(true);
> +    let permString = pluginHost.getPermissionStringForType(mimeType);
> +    permissionEntry.setAttribute("permString", permString);
> +    // XXX this whole "childNodes[i]" business is dumb - is there a better way?
> +    let label = permissionEntry.childNodes[0];

Using a class would be easier to read and would also allow reordering without breaking this code.

@@ +275,5 @@
> +    let radioGroup = permissionEntry.childNodes[2];
> +    radioGroup.setAttribute("id", permString + "RadioGroup");
> +    radioGroup.childNodes[0].setAttribute("id", permString + "#0");
> +    radioGroup.childNodes[1].setAttribute("id", permString + "#1");
> +    radioGroup.childNodes[2].setAttribute("id", permString + "#2");

Can these be changed from IDs to data-permString attributes? Then we shouldn't need to split the attribute value later just to get back the permstring.

Comment 7

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

(In reply to Jared Wein [:jaws] from comment #6)
> ::: browser/base/content/pageinfo/pageInfo.xul
> @@ +376,5 @@
> >          <vbox class="permission" id="permPluginsRow">
> >            <label class="permissionLabel" id="permPluginsLabel"
> >                   value="&permPlugins;" control="pluginsRadioGroup"/>
> > +          <hbox id="permPluginTemplate" role="group" aria-labelledby="permPluginsLabel" align="baseline">
> > +            <label/>
> 
> Why does align="baseline" need to be here?

This makes the children's text labels all line up nicely.

> ::: browser/base/content/pageinfo/permissions.js
> @@ +239,5 @@
> >    }
> >  }
> > +
> > +// XXX copied this from browser-plugins.js - is there a way to share?
> > +function makeNicePluginName(aName) {
> 
> Can you add this as a method on nsIPluginHost?

Filed bug 836841.

> @@ +275,5 @@
> > +    let radioGroup = permissionEntry.childNodes[2];
> > +    radioGroup.setAttribute("id", permString + "RadioGroup");
> > +    radioGroup.childNodes[0].setAttribute("id", permString + "#0");
> > +    radioGroup.childNodes[1].setAttribute("id", permString + "#1");
> > +    radioGroup.childNodes[2].setAttribute("id", permString + "#2");
> 
> Can these be changed from IDs to data-permString attributes? Then we
> shouldn't need to split the attribute value later just to get back the
> permstring.

The problem is that these ids need to be "[whatever permission value]#0" (or 1 or 2) to be interoperable with the rest of the code without serious modifications to how this whole thing works. So, rather than having another attribute on the radio buttons, I figured it would be easier to just use split to get the permString back.

Comment 8

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

Attachment: patch v3

Ignoring what would change when bug 836841 gets fixed, could you have another look at this? I'm hoping to get this uplifted to aurora before the merge (but if that doesn't happen, it's not the end of the world).

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 708798 [details] [diff] [review]
patch v3

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

This also needs to update about:permissions.

Can you upload a screenshot of what this looks like?

::: browser/base/content/pageinfo/pageInfo.xul
@@ +375,5 @@
>          </vbox>
>          <vbox class="permission" id="permPluginsRow">
>            <label class="permissionLabel" id="permPluginsLabel"
>                   value="&permPlugins;" control="pluginsRadioGroup"/>
> +          <hbox id="permPluginTemplate" role="group" aria-labelledby="permPluginsLabel" align="baseline">

The align="baseline" should be moved to CSS.

::: browser/base/content/pageinfo/permissions.js
@@ +256,5 @@
> +function fillInPluginPermissionTemplate(aPluginName, aPermissionString) {
> +  let permPluginTemplate = document.getElementById("permPluginTemplate");
> +  permPluginTemplate.setAttribute("permString", aPermissionString);
> +  let attrs = [
> +    [ ".permPluginTemplateLabel", "value", aPluginName ],

This will make the entry "Adobe Flash", but it would be better if it was "Activate Adobe Flash".

Comment 10

[Benjamin Smedberg]

I am concerned about the storage mechanism being used here, which appears to be keyed on the plugin name. I do not think we should store any data based on plugin names, which can change in unpredictable ways and can contain version numbers. I understand that there is code which tries to strip out the versions, but that is guesswork at best.

The only thing which is constant about plugins is their MIME type, and we should try to key any sort of plugin permission off of the MIME type. See bug 830267 comment 4 for related issues with the addon manager disabled/enabled state so that we can correctly implement bug 549697.

Comment 11

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

Attachment: pageInfo.png

Comment 12

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

(In reply to Jared Wein [:jaws] from comment #9)
> This also needs to update about:permissions.

That's being handled in bug 775857.

> Can you upload a screenshot of what this looks like?

Sure thing - see pageInfo.png

> The align="baseline" should be moved to CSS.

I understand what you're saying, but is that really necessary? It takes something that is accomplished in one line and spreads it out to multiple lines in three other files in other directories (it would need to be in browser/themes/whatever/pageInfo.css, right?). I understand separating the styling from the structure, but I don't think we'll ever want the text to be unaligned, so there's no reason to style that differently.

> This will make the entry "Adobe Flash", but it would be better if it was
> "Activate Adobe Flash".

Maybe the screenshot will explain why I did it this way.

Comment 13

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

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #10)
> I am concerned about the storage mechanism being used here, which appears to
> be keyed on the plugin name. I do not think we should store any data based
> on plugin names, which can change in unpredictable ways and can contain
> version numbers. I understand that there is code which tries to strip out
> the versions, but that is guesswork at best.
> 
> The only thing which is constant about plugins is their MIME type, and we
> should try to key any sort of plugin permission off of the MIME type. See
> bug 830267 comment 4 for related issues with the addon manager
> disabled/enabled state so that we can correctly implement bug 549697.

The use of the plugin names is just for display purposes - all storage of user preferences is handled by the permission manager using pluginHost.getPermissionStringForType as the key (which uses the mime type).

Comment 14

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

Jared - does the screenshot look okay? If so, are you okay with the rest of the changes in the patch? (basically, I'm asking r? again) (see also comment 12)

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to David Keeler (:keeler) from comment #14)
> Jared - does the screenshot look okay? If so, are you okay with the rest of
> the changes in the patch? (basically, I'm asking r? again) (see also comment
> 12)

Yeah, I'll take a look at it tomorrow.

Comment 16

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

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #10)
> I am concerned about the storage mechanism being used here, which appears to
> be keyed on the plugin name. I do not think we should store any data based
> on plugin names, which can change in unpredictable ways and can contain
> version numbers. I understand that there is code which tries to strip out
> the versions, but that is guesswork at best.
> 
> The only thing which is constant about plugins is their MIME type, and we
> should try to key any sort of plugin permission off of the MIME type. See
> bug 830267 comment 4 for related issues with the addon manager
> disabled/enabled state so that we can correctly implement bug 549697.

Oh - I think I might see what you're saying. The permission key returned by pluginHost.getPermissionStringForType should really use a canonical MIME type for the plugin it maps to, not the plugin filename as it currently does. This issue seems familiar - is there a bug on it?

Comment 17

[Benjamin Smedberg]

Other than bug 830267 which is not directly about the API, no.

Comment 18

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

Filed bug 838290.

Comment 19

[Jared Wein [:jaws] (please needinfo? me)]

We still need to have per-plugin "activate always" in the popup notification for this to be a worthwhile pursuit. Most users won't find the Page Info dialog, so it can't be the only place to set this.

Comment 20

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

Agreed - this isn't a complete solution to managing plugin permissions. However, due to the changes from bug 746374, what was previously a relief valve for people who needed to reset their plugin permissions (i.e. Page Info -> Permissions) no longer works. At a bare minimum, we need to be able to point people to this mechanism on support articles, etc. So, this is definitely worthwhile.

Comment 21

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 708798 [details] [diff] [review]
patch v3

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

This looks good to me, but please file a bug to add/figure-out the changes needed for the popup-notification.

Comment 22

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

Thanks, Jared! I think bug 838106 should cover any per-plugin changes necessary in the popup notification.
Try run: https://tbpl.mozilla.org/?tree=Try&rev=0af1ddfe0c2b

Comment 23

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

Wrong try options. Hopefully this is right: https://tbpl.mozilla.org/?tree=Try&rev=55f129007576

Comment 24

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

Attachment: patch v3.1

Had to fix up the tests a bit. Carrying over r+.
Try run looked good: https://tbpl.mozilla.org/?tree=Try&rev=07ee55ba0d30
Checkin: https://hg.mozilla.org/integration/mozilla-inbound/rev/f30b42547a84

Comment 25

[Ryan VanderMeulen [:RyanVM]]

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

Comment 26

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

Comment on attachment 712326 [details] [diff] [review]
patch v3.1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 746374
User impact if declined: users cannot undo permissions set by the click-to-play popup notification
Testing completed (on m-c, etc.): landed on m-c
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: none

Comment 27

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

Bug 746374 didn't land on esr17, so I believe it is unaffected by this bug.

Comment 28

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 712326 [details] [diff] [review]
patch v3.1

low risk - will approve bug 839193

Comment 29

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

Thanks, Lukas!

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

Also, the bug that caused this landed on 20, so 18 and 19 aren't affected.

Comment 30

[Jennifer Morrow [:Boriss] (UX)]

Do we really need click-to-play be able to be individually set for a page?  Are we sure there's real user value here?  After all, there's some consistency problems present in a temporary fix here.  The temporary nature of having both about:permissions and about:addons puts us in a strange position with specific regard to addons-related-to-permissions.  These controls should be accessible in only one place once we get combined Preferences under control and consolidated, but I realize that could be a ways off and if we feel that per-site-click-to-play is important, we'll need a temporary solution.

But, if we make that temporary solution accessible in both about:permissions and about:addons, we're reflecting a "global" state of click-to-play in about:addons which may not be accurate if users have changed click-to-play settings for individual pages.  That's the big drawback.  Why not instead have only global click-to-play for now, which can be viewed in both "All Sites" in about:permissions and on individual extensions in the addons manager?  Then we get a neat 1:1 correlation: click-to-play can be changed in either place and changes the one the user isn't looking at.

Comment 31

[Benjamin Smedberg]

Per-site click-to-play is important. My suggestion in bug 838106 was to expose the per-site settings via the "Options" button in the addon manager.

Comment 32

[Paul Silaghi, QA [:pauly]]

I see all possible plugins listed (even the ones that aren't used) for a specific domain (eg. youtube). Is this ok? Wouldn't be better to list only the used plugins?

Comment 33

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

Well, I think it's difficult to accurately say what plugins a domain uses, and even if it were per-page, the plugins a page uses can change easily (in theory, if not in practice), so it's better to just say, "Here's all your plugins; this page may or may not use them all, but you can set their permissions to whatever you want". (For comparison, the dialog has an images section whether or not the page actually has any images on it.)

Comment 34

[Paul Silaghi, QA [:pauly]]

Looks odd to me a list with 30 plugins on a page that doesn't use any of them (google.com).