Closed Bug 880735 Opened 11 years ago Closed 11 years ago

Reimplement the plugin doorhanger to deal with the new CtP spec

Categories

(Core Graveyard :: Plug-ins, enhancement, P1)

Product:
Core Graveyard
Component:
Plug-ins
Platform:
x86_64
Linux
Type:
enhancement

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
mozilla24

People

(Reporter: benjamin, Assigned: benjamin)

References

(Depends on 2 open bugs)

Details

(Keywords: addon-compat, qawanted, user-doc-needed, Whiteboard: [CtPDefault:P1])

Attachments

(8 files, 3 obsolete files)

part B - add .getPluginTagForType and make .getBlocklistStateForType scriptable, r?johns
4.13 KB, patch
johns
: review+
Details | Diff | Splinter Review
part A - Add a .defaultFallbackType to plugins so that we can avoid re-implementing the ShouldPlay logic in the frontend code. r?johns
8.25 KB, patch
johns
: review+
Details | Diff | Splinter Review
part C - Disabled plugins should be always disabled, site-specific permissions do not override them. I'm sorry for the churn from bug 875454. At least the logic is in one place and easy to fix! r?johns
4.92 KB, patch
johns
: review+
Details | Diff | Splinter Review
part E - Use the new testAndRenew method to make sure that plugin permissions are renewed properly, r?johns
3.33 KB, patch
johns
: review+
Details | Diff | Splinter Review
part F - Remove extra functions, strings, and styles which are no longer needed, r?jaws
15.28 KB, patch
jaws
: review+
Details | Diff | Splinter Review
part G - remove all the styling for the doorhanger, so that we can start with a clean slate, r?jaws
5.24 KB, patch
jaws
: review+
Details | Diff | Splinter Review
part D - New doorhanger frontend code. Removes all logic which shows the plugin doorhanger automatically, including on scripting. Always show the doorhanger when there is a plugin present on the page, even if that plugin is currently enabled. Make the "al
72.16 KB, patch
jaws
: review+
Details | Diff | Splinter Review
part S-Linux - Basic styling so that this can land - add classes to the binding for styling hooks and fix space handling. Make the buttons fill the space, limit the doorhanger size, and add whitespace/separators for the multi-plugin UI, r?jaws
9.53 KB, patch
jaws
: review+
Details | Diff | Splinter Review 
After user research and a bunch of design, we've decided on a new design for the plugin doorhanger and click-to-activate UI. This is currently at http://people.mozilla.com/~lco/CtP/130415%20CtP%20design%20spec.pdf

I've got a bunch of the UI set up but it doesn't work because it has been backed up on a set of backend changes necessary to make things work correctly. It also currently doesn't have any visuals, on which I depend entirely on shorlander.

It is my sincerest hope to land this for the Fx24 cycle, since not having this on the ESR release will cause all sorts of pain with plugin blocklisting. I know that this may conflict with the Australis, and I'm very sorry about that.
Also note: this change removes "click to play" functionality; by default, this is too confusing for most users, and so the only functionality that will be available in Firefox itself will be activate-plugin-on-this-site. This will probably require updates to end-user docs. Those users who want to click to activate particular plugins should still be able to do this using the popular addons (NoScript/Adblock/Flashblock). But the patches in this bug are likely to affect those addons.
Assignee: nobody → benjamin
Severity: normal → enhancement
Keywords: addon-compat, user-doc-needed
Priority: -- → P1
Target Milestone: --- → mozilla24
Blocks: click-to-play
Whiteboard: [CtPDefault:P1]
Depends on: 882858
After some discussion on #security with dveditz and Jesse, we'd like to propose the following string changes for accuracy:

For Insecure/outdated plugins:

Change the string from "Firefox has blocked an <insecure plugin> on <site>" to "Firefox has prevented the <insecure plugin> from running on <site>".
- Jesse's point was that the initial messaging is misleading because it's a plugin on your computer (not the site) that is insecure.


For the latest version of a plugin: 

You might already be doing this, but in case I wasn't clear, for the latest version of a plugin (such as Java plugins, for example), we should be using the "Allow <site> to run <plugin>?", and not the "vulnerable" or "outdated" plugin text. 

We can talk more about this in a meeting if you need more information / want to clarify things :)
I agree with the first part.

I do not agree at all with the second part. We should be presenting the Java plugin as a potential risk even when it is at the latest version, and should not allow it to be univerally enabled.
Maybe we can call it "risky" or "complex" on days when it's not obviously vulnerable?

Failing that, we could just buy a Java 0-day from the black market, and hang on to it just so we can call Java "vulnerable" or "insecure".
Depends on: 883420
I'm not happy with this patch. The only thing I really need to know is "would the plugin host have activated this content automatically if .playPlugin was not called explicitly on this instance". We could probably just cache that information directly and avoid all this. jschoenick thoughts?
Attachment #763045 - Flags: review?(jschoenick)
Yeah so, in bug 875454 I said I wanted site permissions to override the global disabled state of a plugin. Now it's clear that we really don't want that (it makes all kinds of other UI confusing).
Attachment #763046 - Flags: review?(jschoenick)
This works (mainly tested on Linux) It still needs:
* Correct handling of the enter and escape keys and specifying of default buttons
* styling
* removal of lots of old strings and CSS
* Lots of test fixup
* Alter the appearance of the in-page overlays: I think that the behavior is now correct

jaws, I'm still working on removing the obsolete strings and CSS, but I think that this should be good enough for reviews. I'll put up the necessary test changes separately. lco and shorlander are still discussing an issue with whether we want a "cancel" button or a close box, but those remaining changes should be entirely limited to urlbarBindings.xml and shouldn't affect most of the core logic changes here.
Attachment #763048 - Flags: review?(jaws)
Some of the CtP code is shared with mobile. We've been broken by desktop changes in the past. Just an FYI. Please test on devices too.
See Also: → 798278
Attachment #763607 - Flags: review?(jaws)
After discussion with lco, the close button was left out of her mockups but it is important, so I've added it back in.
Attachment #763772 - Flags: review?(jaws)
Comment on attachment 763045 [details] [diff] [review]
part A - Add a .defaultFallbackType to plugins so that we can avoid re-implementing the ShouldPlay logic in the frontend code. r?johns

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

aIgnoreCurrentType is pretty hacky -- I think the proper way to do this would be to migrate ShouldPlay to pluginhost as a GetDefaultStateForDocument() or somesuch, but I have bugs in my queue to touch this so I can save bikeshedding until then as well.

::: content/base/public/nsIObjectLoadingContent.idl
@@ +36,5 @@
>    const unsigned long TYPE_PLUGIN   = 2;
>    const unsigned long TYPE_DOCUMENT = 3;
>    const unsigned long TYPE_NULL     = 4;
>  
> +  const unsigned long PLUGIN_ACTIVE               = 0xFF;

We could have ShouldPlay return a FallbackType and add this to the enum... not sure if cleaner
Attachment #763045 - Flags: review?(jschoenick) → review+
Comment on attachment 763044 [details] [diff] [review]
part B - add .getPluginTagForType and make .getBlocklistStateForType scriptable, r?johns

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

::: dom/plugins/base/nsIPluginHost.idl
@@ +103,5 @@
> +   *
> +   * @throws NS_ERROR_NOT_AVAILABLE if no plugin is available for this MIME
> +   *         type.
> +   */
> +  nsIPluginTag getPluginTagForType(in AUTF8String mimeType);

Should note that the method prefers enabled to disabled plugins, even if the disabled one would otherwise have precedence.

@@ +113,5 @@
> +
> +  /**
> +   * Get the blocklist state for a MIME type.
> +   */
> +  uint32_t getBlocklistStateForType(in string aMimeType);

Why does getStateForType use unsigned long/AUTF8String and this uint32_t/string?
Attachment #763044 - Flags: review?(jschoenick) → review+
Comment on attachment 763046 [details] [diff] [review]
part C - Disabled plugins should be always disabled, site-specific permissions do not override them. I'm sorry for the churn from bug 875454. At least the logic is in one place and easy to fix! r?johns

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

::: content/base/src/nsObjectLoadingContent.cpp
@@ +2210,5 @@
> +      do_QueryInterface(static_cast<nsIObjectLoadingContent*>(this));
> +    nsCOMPtr<nsIRunnable> ev = new nsSimplePluginEvent(thisContent,
> +                             NS_LITERAL_STRING("PluginInstantiated"));
> +    NS_DispatchToCurrentThread(ev);
> +  }

What is this used for? This should be in InstantiatePluginInstance, after the re-entry check.

@@ +2764,5 @@
>    // * Blocklisted plugins are forced to CtP
>    // * Check per-plugin permission and follow that.
>  
>    // Before we check permissions, get the blocklist state of this plugin to set
>    // the fallback reason correctly.

comment got separated from its code

@@ +2780,5 @@
> +  if (blocklistState == nsIBlocklistService::STATE_BLOCKED) {
> +    // no override possible
> +    aReason = eFallbackBlocklisted;
> +    return false;
> +  }    

whitespace
Attachment #763046 - Flags: review?(jschoenick) → review+
Attachment #763047 - Flags: review?(jschoenick) → review+
Comment on attachment 763772 [details] [diff] [review]
part H - Add a close button, add "default" button styling, make the enter key use the correct default button, and fix focus so that the correct elements are focusable and the default button is focused by default, r?jaws

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

::: browser/base/content/browser-plugins.js
@@ +615,5 @@
>      else if (event == "dismissed") {
>        // Once the popup is dismissed, clicking the icon should show the full
>        // list again
>        this.options.primaryPlugin = null;
>      }

I seem we can use `switch` statement in this method. Or it's better that we should use strict equality operator.
> Why does getStateForType use unsigned long/AUTF8String and this
> uint32_t/string?

unsigned long is the "old" name for that type, uint32_t is preferred for new code. I've updated this instance. I used "string" to be similar with the existing C++ method signature and avoid refactoring callsites.
I tested the latest try-server build, and the door hanger works nicely. It focuses properly, and one can tab to the other buttons just fine.
What is still bothering me -- and I am not sure if this is within the scope of this bug -- is the part that gets injected into the actual page. There are two buttons I can tab to, but they have no labels our accessibility APIs can identify. If these are image buttons, they should have an alt attribute with localizable strings indicating what they do, or if side effects of the alt attribute are not desired, use aria-label instead with the same localizable string.
There is also a string saying "Click to activate Adobe Flash" right below, or after, those buttons, in the DOM order. This is neither a button or a link, it just appears to be clickable text.
Marco, great. I haven't touched the in-page overlays yet, but I'm aware of them sucking and they will be fixed.
Comment on attachment 763048 [details] [diff] [review]
part D - New doorhanger frontend code.

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

It would be good to get feedback from David as well.

::: browser/base/content/browser-plugins.js
@@ +3,5 @@
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  const kPrefNotifyMissingFlash = "plugins.notifyMissingFlash";
> +const kPrefSessionPersistSeconds = "plugin.sessionPermission.intervalMinutes";

This is confusing, the const name should reference "minutes", not "seconds".

@@ -32,5 @@
>      } else {
> -      if (pluginElement instanceof HTMLObjectElement) {
> -        pluginsPage = pluginElement.getAttribute("codebase");
> -      } else {
> -        pluginsPage = pluginElement.getAttribute("pluginspage");

Isn't this removal part of bug 874198? That patch has r+, are you waiting to land it?

@@ +242,5 @@
>          return;
>        }
>      }
>  
> +    let browser = gBrowser.getBrowserForDocument(plugin.ownerDocument.defaultView.top.document);

Use doc instead of plugin.ownerDocument.

@@ +618,5 @@
>      return pluginNeedsActivation;
>    },
>  
> +  _clickToPlayNotificationEventCallback: function PH_ctpEventCallback(event) {
> +    if (event == "beforeshown") {

This will need to get changed to "showing".

@@ +634,5 @@
>                             .getInterface(Ci.nsIDOMWindowUtils);
> +
> +    let principal = Services.scriptSecurityManager.getNoAppCodebasePrincipal(browser.currentURI);
> +
> +    let centerActions = []; 

nit, trailing whitespace

@@ +655,3 @@
>  
> +      // Add the per-site permissions and details URLs to pluginInfo here
> +      // because they are more expensive to compute and so we avoide it in

s/avoide/avoid/

@@ +655,5 @@
>  
> +      // Add the per-site permissions and details URLs to pluginInfo here
> +      // because they are more expensive to compute and so we avoide it in
> +      // the tighter loop above.
> +      let pobject = Services.perms.getPermissionObject(principal,

s/pobject/permissionObj/

@@ +657,5 @@
> +      // because they are more expensive to compute and so we avoide it in
> +      // the tighter loop above.
> +      let pobject = Services.perms.getPermissionObject(principal,
> +                      pluginInfo.permissionString, false);
> +      if (null == pobject) {

nit, put truthy case first, so:

if (permissionObj) {
 ... 
} else {
 ...
}

@@ +696,5 @@
> +    let expireType;
> +    let expireTime;
> +
> +    switch (aNewState) {
> +    case "allownow":

please indent the case statements, and thus increase the indentation of the case bodies.

@@ +704,5 @@
> +      permission = Ci.nsIPermissionManager.ALLOW_ACTION;
> +      expireType = Ci.nsIPermissionManager.EXPIRE_SESSION;
> +      expireTime = Date.now() + Services.prefs.getIntPref(kPrefSessionPersistSeconds) * 60 * 1000;
> +      break;
> +      

nit, trailing whitespace

@@ +725,5 @@
> +      expireTime = 0;
> +      break;
> +
> +    default:
> +      Components.utils.reportError(Error("Unexpected plugin state: " + aNewState));

Cu.reportError

@@ +751,3 @@
>        // canActivatePlugin will return false if this isn't a known plugin type,
>        // so the pluginHost.getPermissionStringForType call is protected
> +      if (gPluginHandler.canActivatePlugin(plugin) && 

nit, trailing whitespace

@@ +771,5 @@
> +      return;
> +    }
> +
> +    let haveVulnerablePlugin = plugins.some(function(plugin) {
> +      let bls = plugin.pluginFallbackType;

I'm not sure what bls is short for (probably blocklist state, but that seems unnecessarily terse), please rename bls to fallbackType.

@@ +783,4 @@
>        dismissed = false;
> +
> +    let primaryPluginPermission = null;
> +    if (aPrimaryPlugin !== undefined) {

if (aPrimaryPlugin) {

@@ +796,2 @@
>      PopupNotifications.show(aBrowser, "click-to-play-plugins",
> +                            "dummy", icon,

"dummy" will be user-facing, what are the plans for fixing this?

::: browser/base/content/browser.css
@@ +505,5 @@
>  #click-to-play-plugins-notification {
>    -moz-binding: url("chrome://browser/content/urlbarBindings.xml#click-to-play-plugins-notification");
>  }
>  
> +row.plugin-popupnotification-centeritem {

no need for the tagName here, just use .plugin-popupnotification-centeritem

::: browser/base/content/urlbarBindings.xml
@@ +1391,5 @@
> +                      anonid="center-item-menulist">
> +          <xul:menupopup>
> +            <xul:menuitem anonid="allownow" value="allownow" />
> +            <xul:menuitem anonid="allowalways" value="allowalways" />
> +            <xul:menuitem anonid="block" value="block" />

These should use DTD-style strings. I don't think it is necessary for the strings to be located in a string bundle.

@@ +1402,5 @@
>      </resources>
>      <implementation>
> +      <constructor><![CDATA[
> +        var self = this;
> +        function setLabel(id, string) {

Moving allownow/allowalways/block to use DTD-style strings in the markup, similar to &checkForUpdates will allow us to remove this function and the lines that call it.

@@ +1412,5 @@
> +        setLabel("block", "pluginBlockNow.label");
> +
> +        document.getAnonymousElementByAttribute(this, "anonid", "center-item-label").value = this.action.pluginName;
> +
> +        let curState;

Initialize curState to "block".

@@ +1423,5 @@
> +          }
> +        }
> +        else {
> +          curState = "block";
> +        }

Remove this else block since it won't be necessary if curState is initialized to "block".

@@ +1430,5 @@
> +        let warningString = "";
> +        let linkString = "";
> +
> +        let link = document.getAnonymousElementByAttribute(this, "anonid", "center-item-link");
> +        let url = this.action.detailsLink;

Leave url undefined here and set it to this.action.details at the top of the else block below.

@@ +1451,5 @@
> +            break;
> +          case Ci.nsIBlocklistService.STATE_BLOCKED:
> +            document.getAnonymousElementByAttribute(this, "anonid", "center-item-menulist").hidden = true;
> +            warningString = gNavigatorBundle.getString("pluginActivateBlocked.label");
> +            linkString = gNavigatorBundle.getString("pluginActivate.learnMore");            

nit, trailing whitespace

@@ +1489,5 @@
> +                xbl:inherits="popupid">
> +        <xul:box class="click-to-play-plugins-notification-description-box" flex="1">
> +          <xul:description>
> +            <html:span anonid="click-to-play-plugins-notification-description" />
> +            &#32;

This is the space character, why is this necessary? It seems purely presentational, is this working around a CSS issue that you encountered?

@@ +1501,5 @@
> +          </xul:columns>
> +          <xul:rows>
> +            <children includes="row"/>
> +            <xul:hbox pack="start" anonid="plugin-notification-showbox">
> +              <xul:button label="Show All" class="plugin-notification-showbutton"

This needs to be localized.

@@ +1510,5 @@
> +        <xul:hbox class="click-to-play-plugins-notification-button-container"
> +                  pack="center" align="center">
> +          <xul:button anonid="primarybutton"
> +                      class="popup-notification-menubutton"
> +                      oncommand="document.getBindingParent(this)[this.getAttribute('action')]()"

This seems too tricky for most :)

@@ +1511,5 @@
> +                  pack="center" align="center">
> +          <xul:button anonid="primarybutton"
> +                      class="popup-notification-menubutton"
> +                      oncommand="document.getBindingParent(this)[this.getAttribute('action')]()"
> +                      label="1" flex="1"/>

Setting label="1" seems wrong. Was this (and below) intended?

@@ +1526,5 @@
>      <resources>
>        <stylesheet src="chrome://global/skin/notification.css"/>
>      </resources>
>      <implementation>
> +      <!-- 0==single element 1==multiple collapsed 2==multiple expanded -->

Hiding these in this comment is too hard to find lol, can you do something like http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#63 ?

@@ +1539,4 @@
>        <constructor><![CDATA[
>          const XUL_NS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
> +        let self = this;
> +        var first = true;

|first| is unused, you can delete it.

@@ +1543,1 @@
>          this.notification.options.centerActions.forEach(function(action) {

Use an arrow function here and then you can get rid of the |let self = this;| dance.

this.notification.options.centerActions.forEach((action) => {
  ...
});

@@ +1546,2 @@
>            item.action = action;
> +          

nit, trailing whitespace

@@ +1552,5 @@
> +          this._setState(0);
> +        } else if (this.notification.options.primaryPlugin) {
> +          this._setState(1);
> +        } else {
> +          this._setState(2);

0,1,2 need to be replaced with constants or some way to clarify what they mean.

@@ +1587,5 @@
> +              if (this.notification.options.primaryPlugin == child.action.permissionString) {
> +                child.hidden = false;
> +              }
> +              else {
> +                child.hidden = true;

child.hidden = this.notification.options.primaryPlugin !=
               child.action.permissionString;

@@ +1608,5 @@
> +        <body><![CDATA[
> +          var action = this.notification.options.centerActions[0];
> +          var host = action.pluginPermissionHost;
> +
> +          let label, linkLabel, linkUrl, pb, sb;

Rename pb and sb to more verbose names.

@@ +1626,5 @@
> +            case Ci.nsIBlocklistService.STATE_NOT_BLOCKED:
> +              label = "pluginEnabled.message";
> +              linkLabel = "pluginActivate.learnMore";
> +              break;
> +                

nit, trailing whitespace

@@ +1628,5 @@
> +              linkLabel = "pluginActivate.learnMore";
> +              break;
> +                
> +            case Ci.nsIBlocklistService.STATE_BLOCKED:
> +              Components.utils.reportError(Error("Cannot happen!"));

Cu.reportError

@@ +1648,5 @@
> +          }
> +          else if (action.pluginTag.enabledState == Ci.nsIPluginTag.STATE_DISABLED) {
> +            var dialogStrings = Services.strings.createBundle("chrome://global/locale/dialog.properties");
> +
> +            this._setupLink("pluginActivateDisabled.manage", "about:blank");

Linking to about:blank doesn't seem helpful. Is this a temporary link here or did you intend to hide the link?

@@ +1649,5 @@
> +          else if (action.pluginTag.enabledState == Ci.nsIPluginTag.STATE_DISABLED) {
> +            var dialogStrings = Services.strings.createBundle("chrome://global/locale/dialog.properties");
> +
> +            this._setupLink("pluginActivateDisabled.manage", "about:blank");
> +            document.getAnonymousElementByAttribute(this, "anonid", "click-to-play-plugins-notification-link").addEventListener("click", function(event) {

This line and a few below are getting too long, can you please break them into multiple statements?

@@ +1686,5 @@
> +            case Ci.nsIBlocklistService.STATE_NOT_BLOCKED:
> +              label = "pluginActivateNew.message";
> +              linkLabel = "pluginActivate.learnMore";
> +              break;
> +                

nit, trailing whitespace

@@ +1729,5 @@
> +          if (pluginName) {
> +            args.unshift(pluginName);
> +          }
> +          var bases = gNavigatorBundle.getFormattedString(baseString, args).
> +            split("__host__", 2);

This part here is confusing to me. Are localizers going to know that the first part of the string is the part that is going to be used? This seems like it will be easy for things to go wrong here.

@@ +1732,5 @@
> +          var bases = gNavigatorBundle.getFormattedString(baseString, args).
> +            split("__host__", 2);
> +
> +          span.appendChild(document.createTextNode(bases[0]));
> +          var hostSpan = document.createElementNS("http://www.w3.org/1999/xhtml", "em");

You can just use |.textContent =| here.

@@ +1773,5 @@
> +      <method name="_singleActivateAlways">
> +        <body><![CDATA[
> +          gPluginHandler._updatePluginPermission(this.notification,
> +            this.notification.options.centerActions[0],
> +            "allownow");

for _singleActivateAlways, it seems you actually would want "allowalways" here.

@@ +1783,5 @@
> +          for (var item of this._items) {
> +            if (item.action.pluginTag.enabledState == Ci.nsIPluginTag.STATE_DISABLED) {
> +              continue;
> +            }
> +            if (item.action.blocklistState == Ci.nsIBlocklistService.STATE_BLOCKED) {

let action = item.action;
if (action.pluginTag.enabledState == Ci.nsIPluginTab.STATE_DISABLED) ||
    action.blocklistState == Ci.nsIBlocklistService.STATE_BLOCKED) {
  continue;
}

::: browser/locales/en-US/chrome/browser/browser.properties
@@ +118,5 @@
>  crashedpluginsMessage.submitButton.label=Submit a crash report
>  crashedpluginsMessage.submitButton.accesskey=S
>  crashedpluginsMessage.learnMore=Learn More…
> +
> +pluginActivateNew.message=Allow %2$S to run "%1$S"?

Please add localization notes for each of the string replacements here and below.

@@ +121,5 @@
> +
> +pluginActivateNew.message=Allow %2$S to run "%1$S"?
> +pluginActivateMultiple.message=Allow %S to run plugins?
> +pluginActivate.learnMore=Learn More…
> +pluginActivateOutdated.message=Firefox has prevented the outdated plugin "%S" from running on %S.

We can't use "Firefox" here (or below), this needs to be replaced with brandShortName.

::: modules/libpref/src/init/all.js
@@ +1835,5 @@
>  pref("plugin.default.state", 2);
>  
> +// How long in minutes we will allow a plugin to work after the user has chosen
> +// to allow it "now"
> +pref("plugin.sessionPermission.intervalMinutes", 60);

plugin.sessionPermissionNow.intervalInMinutes

@@ +1838,5 @@
> +// to allow it "now"
> +pref("plugin.sessionPermission.intervalMinutes", 60);
> +// How long in days we will allow a plugin to work after the user has chosen
> +// to allow it persistently.
> +perf("plugin.persistentPermission.intervalDays", 90);

plugin.persistentPermissionAlways.intervalInDays
Attachment #763048 - Flags: review?(jaws)
Attachment #763048 - Flags: review-
Attachment #763048 - Flags: feedback?(dkeeler)
Comment on attachment 763592 [details] [diff] [review]
part F - Remove extra functions, strings, and styles which are no longer needed, r?jaws

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

::: browser/locales/en-US/chrome/browser/browser.properties
@@ +122,3 @@
>  pluginActivateOutdated.label=Outdated plugin
>  pluginActivate.updateLabel=Update now…
> +# LOCALIZATION NOTE: 

nit, trailing whitespace here and above.
Attachment #763592 - Flags: review?(jaws) → review+
(In reply to Jared Wein [:jaws] from comment #21)
> @@ +1732,5 @@
> > +          var bases = gNavigatorBundle.getFormattedString(baseString, args).
> > +            split("__host__", 2);
> > +
> > +          span.appendChild(document.createTextNode(bases[0]));
> > +          var hostSpan = document.createElementNS("http://www.w3.org/1999/xhtml", "em");
> 
> You can just use |.textContent =| here.

Sorry, I misread this. You can leave this as-is.
Status: NEW → ASSIGNED
Attachment #763607 - Flags: review?(jaws) → review+
Comment on attachment 763772 [details] [diff] [review]
part H - Add a close button, add "default" button styling, make the enter key use the correct default button, and fix focus so that the correct elements are focusable and the default button is focused by default, r?jaws

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

Clearing review until questions are answered. My questions are not intended to say this approach is right or wrong, I'm just trying to figure out the implications first.

::: browser/base/content/browser-plugins.js
@@ +609,5 @@
>      }
> +    else if (event == "shown") {
> +      // When the notification is shown, the focus isn't on anything useful.
> +      // Ask the binding to focus the primary element.
> +      PopupNotifications.panel.firstChild.setupFocus();

If the user is interacting with the page, will this move focus out of the page or location bar? For example, if a webpage starts loading and I decide to start typing in the location bar while it's loading, will my focus unexpectedly change to the dropdown and cause my typing to stop appearing in the location bar?

::: browser/base/content/urlbarBindings.xml
@@ +1496,5 @@
> +          <xul:toolbarbutton anonid="closebutton"
> +                             class="messageCloseButton popup-notification-closebutton tabbable"
> +                             xbl:inherits="oncommand=closebuttoncommand"
> +                             tooltiptext="&closeNotification.tooltip;"/>
> +         

nit, trailing whitespace

@@ +1840,4 @@
>      </implementation>
> +    <handlers>
> +      <handler event="keypress" keycode="VK_ENTER" group="system" action="this._accept(event);"/>
> +      <handler event="keypress" keycode="VK_RETURN" group="system" action="this._accept(event);"/>

What if the user has tabbed to a different button? This looks like it will accept the default action regardless of what is actually focused.
Attachment #763772 - Flags: review?(jaws)
I'm going to add another comment pointing out specific review fixes in a second. Part D now folds in/subsumes part H
Attachment #763048 - Attachment is obsolete: true
Attachment #763772 - Attachment is obsolete: true
Attachment #763048 - Flags: feedback?(dkeeler)
Attachment #765899 - Flags: review?(jaws)
> This is confusing, the const name should reference "minutes", not "seconds".

Fixed at https://bugzilla.mozilla.org/attachment.cgi?id=765899&action=diff#a/browser/base/content/browser-plugins.js_sec2
 
> Isn't this removal part of bug 874198? That patch has r+, are you waiting to
> land it?

It is. Unfortunately that patch got folded into this one during a git rebase, so my plan is just to land them as one since splitting them apart is now hard.

> s/avoide/avoid/

Oops missed this comment, I have it fixed locally.

> please indent the case statements, and thus increase the indentation of the
> case bodies.

I've done this, although I'm surprised since we typically don't do this in C++-land.

> @@ +796,2 @@
> >      PopupNotifications.show(aBrowser, "click-to-play-plugins",
> > +                            "dummy", icon,
> 
> "dummy" will be user-facing, what are the plans for fixing this?

It's not user-facing because of the custom XBL. I just need to pass some string here to satisfy PopupNotifications.jsm

> ::: browser/base/content/urlbarBindings.xml
> @@ +1391,5 @@
> > +                      anonid="center-item-menulist">
> > +          <xul:menupopup>
> > +            <xul:menuitem anonid="allownow" value="allownow" />
> > +            <xul:menuitem anonid="allowalways" value="allowalways" />
> > +            <xul:menuitem anonid="block" value="block" />
> 
> These should use DTD-style strings. I don't think it is necessary for the
> strings to be located in a string bundle.

Done via: https://bugzilla.mozilla.org/attachment.cgi?id=765899&action=diff#a/browser/base/content/urlbarBindings.xml_sec2
and https://bugzilla.mozilla.org/attachment.cgi?id=765899&action=diff#a/browser/locales/en-US/chrome/browser/browser.dtd_sec2

please check that I've done this correctly.

> This is the space character, why is this necessary? It seems purely
> presentational, is this working around a CSS issue that you encountered?

As discussed on IRC, this space separates the description from the link with a breaking space. Raw spaces are discarded while parsing XUL/XBL, so this space was necessary. I have left it in because I don't know of another way to accomplish it.

> > +          <xul:button anonid="primarybutton"
> > +                      class="popup-notification-menubutton"
> > +                      oncommand="document.getBindingParent(this)[this.getAttribute('action')]()"
> 
> This seems too tricky for most :)

It's now:
oncommand="document.getBindingParent(this)._onButton(this);"

and _onButton is:
 	
let methodName = aButton.getAttribute("action");
this[methodName]();


> > +      <!-- 0==single element 1==multiple collapsed 2==multiple expanded -->
> 
> Hiding these in this comment is too hard to find lol, can you do something
> like
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/
> tabbrowser.xml#63 ?

See <field name="_states"> at https://bugzilla.mozilla.org/attachment.cgi?id=765899&action=diff#a/browser/base/content/urlbarBindings.xml_sec3


> @@ +1543,1 @@
> >          this.notification.options.centerActions.forEach(function(action) {
> 
> Use an arrow function here and then you can get rid of the |let self =
> this;| dance.

I just switched to for...of.

> @@ +1648,5 @@
> > +          }
> > +          else if (action.pluginTag.enabledState == Ci.nsIPluginTag.STATE_DISABLED) {
> > +            var dialogStrings = Services.strings.createBundle("chrome://global/locale/dialog.properties");
> > +
> > +            this._setupLink("pluginActivateDisabled.manage", "about:blank");
> 
> Linking to about:blank doesn't seem helpful. Is this a temporary link here
> or did you intend to hide the link?

about:blank is never actually used, because the click handler for the link opens the addons manager. I just needed some URL so that the text-link styling was correct.


> ::: browser/locales/en-US/chrome/browser/browser.properties
> @@ +118,5 @@
> >  crashedpluginsMessage.submitButton.label=Submit a crash report
> >  crashedpluginsMessage.submitButton.accesskey=S
> >  crashedpluginsMessage.learnMore=Learn More…
> > +
> > +pluginActivateNew.message=Allow %2$S to run "%1$S"?
> 
> Please add localization notes for each of the string replacements here and
> below.

I have added notes, please check that they are correct.

> We can't use "Firefox" here (or below), this needs to be replaced with
> brandShortName.

In order to do this using the XUL stringbundle I did https://bugzilla.mozilla.org/attachment.cgi?id=765899&action=diff#a/browser/base/content/browser.js_sec2 please check

> ::: browser/base/content/browser-plugins.js
> @@ +609,5 @@
> >      }
> > +    else if (event == "shown") {
> > +      // When the notification is shown, the focus isn't on anything useful.
> > +      // Ask the binding to focus the primary element.
> > +      PopupNotifications.panel.firstChild.setupFocus();
> 
> If the user is interacting with the page, will this move focus out of the
> page or location bar? For example, if a webpage starts loading and I decide
> to start typing in the location bar while it's loading, will my focus
> unexpectedly change to the dropdown and cause my typing to stop appearing in
> the location bar?

The doorhanger should only open currently based on user action. I discovered during testing that when the doorhanger opens, for some reason the focus gets confused and no element has focus (probably the same as http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/notification.xml#186). This code should just restore focus to the element that should have had it anyway.


> @@ +1840,4 @@
> >      </implementation>
> > +    <handlers>
> > +      <handler event="keypress" keycode="VK_ENTER" group="system" action="this._accept(event);"/>
> > +      <handler event="keypress" keycode="VK_RETURN" group="system" action="this._accept(event);"/>
> 
> What if the user has tabbed to a different button? This looks like it will
> accept the default action regardless of what is actually focused.

It doesn't, but I've added a clarifying comment: the event.isDefaultPrevented check in the event handler ensures that if the enter key is handled directly by a button then it will not trigger the default action.
Comment on attachment 765899 [details] [diff] [review]
part D - New doorhanger frontend code. Removes all logic which shows the plugin doorhanger automatically, including on scripting. Always show the doorhanger when there is a plugin present on the page, even if that plugin is currently enabled. Make the "al

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

Two things I would investigate:
1. Does this work properly after drag-and-dropping a tab to/from a new/separate window?
2. Does this affect shumway?

Otherwise, I think this is a good approach.

::: browser/locales/en-US/chrome/browser/browser.properties
@@ +136,5 @@
> +# These strings are used when an unsafe plugin has no update available.
> +# %1$S is the plugin name, %2$S is the domain, and %3$S is brandShortName.
> +pluginActivateVulnerable.message=%3$S has prevented the unsafe plugin "%1$S" from running on %2$S.
> +pluginActivateVulnerable.label=Vulnerable plugin!
> +pluginActivate.riskLabel=What's the risk?…

Why a question mark and then an ellipsis? I think just the question mark is fine.
Attachment #765899 - Flags: feedback+
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #26)
> > This is the space character, why is this necessary? It seems purely
> > presentational, is this working around a CSS issue that you encountered?
> 
> As discussed on IRC, this space separates the description from the link with
> a breaking space. Raw spaces are discarded while parsing XUL/XBL, so this
> space was necessary. I have left it in because I don't know of another way
> to accomplish it.

This seems potentially l10n-unfriendly. In other cases we've included the link in the string to allow for full localizer-customizability, e.g. in http://hg.mozilla.org/mozilla-central/annotate/61c3c8b85563/browser/locales/en-US/chrome/browser/browser.dtd#l653 (though that should have an l10n note...).

> about:blank is never actually used, because the click handler for the link
> opens the addons manager. I just needed some URL so that the text-link
> styling was correct.

If you set the link's onclick attribute the styling will be correct without an href. Since this seems to be a single-purpose link, you should just be able to just set onclick="gPluginHandler.managePlugins();"?

> > We can't use "Firefox" here (or below), this needs to be replaced with
> > brandShortName.
> 
> In order to do this using the XUL stringbundle I did
> https://bugzilla.mozilla.org/attachment.cgi?id=765899&action=diff#a/browser/
> base/content/browser.js_sec2 please check

Please don't add a browser.js global (gBrandBundle), just use a local variable where needed.

> The doorhanger should only open currently based on user action. I discovered
> during testing that when the doorhanger opens, for some reason the focus
> gets confused and no element has focus (probably the same as
> http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/
> notification.xml#186). This code should just restore focus to the element
> that should have had it anyway.

Do you need to fix this here? Is this a problem with your binding specifically or with notifications in general? I'd like to investigate separately in another bug if it's not specific to this scenario. notification.xml#186 shouldn't be relevant because that's related to focused elements being hidden.
This includes gavin's comments. I have removed the focus bits for now, for a followup if necessary. I think it would be more fragile to have the link in the localized string, especially because it's not part of the same sentence and so shouldn't need repositioning.
I have verified that this does appear to work correctly after drag-drop (and it passes the drag-drop automated tests), and it also at least passes the plugin-preview automated tests and was designed to keep working correctly in that case. I did not spend a lot of time thinking about that case, though: is it used by anything other than shumway?
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #31)
> is it used by anything other than shumway?

Not as far as I know - if those tests pass I'd say you're good in that respect.
I think I can just copy these rules directly for win/mac, but I haven't tested those yet. My hope is still to land this this weekend.
Attachment #766087 - Flags: review?(jaws)
Comment on attachment 766012 [details] [diff] [review]
part D - New doorhanger frontend code. Removes all logic which shows the plugin doorhanger automatically, including on scripting. Always show the doorhanger when there is a plugin present on the page, even if that plugin is currently enabled. Make the "al

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

r=me with the following changes. Sorry for the slow turn-around-time.

::: browser/base/content/browser-plugins.js
@@ +798,2 @@
>      PopupNotifications.show(aBrowser, "click-to-play-plugins",
> +                            "dummy", icon,

change "dummy" to an empty string

::: browser/base/content/pageinfo/permissions.js
@@ +298,5 @@
>    document.querySelector(".permPluginTemplateRadioBlock").removeAttribute("id");
>  }
>  
>  function initPluginsRow() {
> +  var vulnerableLabel = document.getElementById("browserBundle").getString("pluginActivateVulnerable.label");

I see it was already like this, but please replace var with let.

::: browser/base/content/urlbarBindings.xml
@@ +1429,5 @@
> +        if (this.action.pluginTag.enabledState == Ci.nsIPluginTag.STATE_DISABLED) {
> +          document.getAnonymousElementByAttribute(this, "anonid", "center-item-menulist").hidden = true;
> +          warningString = gNavigatorBundle.getString("pluginActivateDisabled.label");
> +          linkString = gNavigatorBundle.getString("pluginActivateDisabled.manage");
> +          url = "about:blank";

Setting the url to about:blank shouldn't be necessary. You should also be able to setAttribute("onclick", function () ...); to get the same behavior.

@@ +1730,5 @@
> +        <body><![CDATA[
> +          try {
> +            var bsn = this._brandShortName;
> +          }
> +          catch (e) {

This can be removed, bsn is never referenced, and caching the value of this._brandShortName shouldn't give us any perf or readability wins.

::: browser/locales/en-US/chrome/browser/browser.properties
@@ +136,5 @@
> +# These strings are used when an unsafe plugin has no update available.
> +# %1$S is the plugin name, %2$S is the domain, and %3$S is brandShortName.
> +pluginActivateVulnerable.message=%3$S has prevented the unsafe plugin "%1$S" from running on %2$S.
> +pluginActivateVulnerable.label=Vulnerable plugin!
> +pluginActivate.riskLabel=What's the risk?…

Drop the ellipsis here.

@@ +148,5 @@
> +pluginEnabledOutdated.message=Outdated plugin "%S" is enabled on %S.
> +pluginEnabledVulnerable.message=Insecure plugin "%S" is enabled on %S.
> +
> +# LOCALIZATION NOTE (pluginActivateNow.label): This button will enable the
> +# plugin in the current session for an hour, auto-renewed if the site keeps

s/for an hour/for a short time (about an hour)/

I'm suggesting this change because we may tweak the pref and I don't want this to get out of date.

@@ +150,5 @@
> +
> +# LOCALIZATION NOTE (pluginActivateNow.label): This button will enable the
> +# plugin in the current session for an hour, auto-renewed if the site keeps
> +# using the plugin.
> +pluginActivateNow.label=Allow Now

browser.dtd says to make sure to keep these identical strings updated in browser.properties. Please include a similar notice here to say that these strings are duplicated in browser.dtd.
Attachment #766012 - Flags: review?(jaws) → review+
Comment on attachment 766087 [details] [diff] [review]
part S-Linux - Basic styling so that this can land - add classes to the binding for styling hooks and fix space handling. Make the buttons fill the space, limit the doorhanger size, and add whitespace/separators for the multi-plugin UI, r?jaws

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

We are trying to remove duplication within the theme files. Instead of copying/pasting these styles to windows/browser.css and osx/browser.css, please move these new styles to /browser/themes/shared/plugin-doorhanger.inc.css and %include the new CSS file from the three browser.css files.

::: browser/base/content/urlbarBindings.xml
@@ +1758,5 @@
>            span.appendChild(document.createTextNode(bases[0]));
>            var hostSpan = document.createElementNS("http://www.w3.org/1999/xhtml", "em");
>            hostSpan.appendChild(document.createTextNode(host));
>            span.appendChild(hostSpan);
> +          span.appendChild(document.createTextNode(bases[1] + " "));

I'm not sure if adding the space character will cause problems for certain locales. I don't know of any, so I'm ok with this, but I don't think anybody will notice the issue until these patches hit the Release channel.

::: browser/themes/linux/browser.css
@@ +2295,5 @@
> +  width: 28em;
> +}
> +
> +.click-to-play-plugins-notification-center-box {
> +  background: white;

We usually use some off-shade of white, or just put a low-opacity white on top of -moz-dialog. Also, using the shorthand property of 'background' will make it harder in the future to see what properties were intentionally set.

Please change this to:
background-color: rgba(255,255,255,.3);

@@ +2303,5 @@
> +  border-top: 1px solid ThreeDShadow;
> +}
> +
> +.center-item-label {
> +  margin-bottom: 0px;

When using 0px for a length unit in CSS, just use 0 as the value as there is no need to specify a unit of measurement.
Attachment #766087 - Flags: review?(jaws) → review+
Part A, .defaultFallbackType - https://hg.mozilla.org/mozilla-central/rev/051b257f38ca
Part B, .getPluginTagForType - https://hg.mozilla.org/mozilla-central/rev/4ae14fad4744
Part C, site permission cannot override the disabled state - https://hg.mozilla.org/mozilla-central/rev/fa7166cddea2
Part E, objectloadingcontent should use .updateExpireTime - https://hg.mozilla.org/mozilla-central/rev/e32abcc03d44
Part D, main UI changes - https://hg.mozilla.org/mozilla-central/rev/d317e0e8afb7
Part F, remove leftover strings, functions, and styles - https://hg.mozilla.org/mozilla-central/rev/501a4ba3506a
Test fixups - https://hg.mozilla.org/mozilla-central/rev/3da4f4ddc833
Part S, basic styling - https://hg.mozilla.org/mozilla-central/rev/50332b66c7a1
Undo a string change s/insecure/unsafe/ without key change that flod pointed out in IRC - https://hg.mozilla.org/mozilla-central/rev/1bc0960d55fb
Part S followup, move styles into the shared directory - https://hg.mozilla.org/mozilla-central/rev/1da2051be457

I have a bunch of followup bugs to file and I'm also going to write up a testing/localization guide.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Depends on: 886403
I test this feature on latest-mozilla-central.

I have a question.
Why we keep doorhanger icon after activate plugins in pages? (I don't think we need it...)
(In reply to Tetsuharu OHZEKI [:saneyuki_s] from comment #37)
> Why we keep doorhanger icon after activate plugins in pages?

To provide (discoverable) access to changing the plugin behaviour, e.g. to block it again.
Depends on: 886940
Testing and localization guide is now available at http://benjamin.smedbergs.us/tests/ctptests/
Keywords: qawanted
Depends on: 887088
Depends on: 887165
Depends on: 886995
Mozilla must have the worst user research team in the whole damn universe
(In reply to Georg Fritzsche [:gfritzsche] from comment #38)
> (In reply to Tetsuharu OHZEKI [:saneyuki_s] from comment #37)
> > Why we keep doorhanger icon after activate plugins in pages?
> 
> To provide (discoverable) access to changing the plugin behaviour, e.g. to
> block it again.

To add to what Georg's said, there are a couple of user-facing reasons why it's important to give people the ability to enable or disable plugins easily:

1. We want user choices to be forgiving; users should be allowed to make mistakes.

Many people don't understand what plugins are, and how enabling a plugin might affect their page. They might click to enable a plugin just to see what it does or because they didn't read the doorhanger, which potentially opens them up to greater security risk or performance slow downs. We wanted to make it easy for people to enable the plugin, and just as easy to disable it if they decide that they realize they don't need it on the page after all. After all, we'd like to discourage people from keeping unnecessary / dangerous plugins enabled.


2. We want people to be aware that there are plugins on a page. 

In the previous design, once plugins were enabled, there wasn't any indication that the page contained potentially risky or detrimental content. The page looked just like any other website. This is especially problematic in the case where the user has permanently enabled a plugin, and won't remember that decision 5 months down the road. Keeping the icon present is a reminder of that decision.

----

I would like to know what is the concern with keeping the icon in the address bar though. If it's a matter of keeping that space less cluttered, we have two potentially follow ups to the design:

The short term tweak is to have two flavors of the plugin indicator so the user knows when they're enabled or disabled. For example, the indicator could be blue if plugins are disabled (because it hints to the user that an action may be needed on his part) or grey when plugins are enabled (or vice versa). By having a state that's not as attention-grabbing, we can minimize distractions in the URL bar.

The longer-term tweak that the UX team is discussing at length is the general problem of permission indicators in the URL bar. It's highly likely that we'll be moving the ability to modify permissions into the site identity panel (the Larry Menu) in the long run. This means that the user would not see an indicator in the URL bar once they've made the choice to enable to plugin. However, this is a longer term change that we are still figuring out how best to implement
I always thought click to play was a great feature for a browser to have. A lot of extensions give you mostly the functionality of click to play but also add a lot of other stupid stuff that changes how Firefox works. The previous click to play worked great.
Now I will have to click allow once in the upper left hand corner of my 24inch monitor each time I want to watch a youtube video. My mother won't understand your doorhanger and once she has clicked allow for each website she uses she will no longer be nagged by it. Of course she won't be protected by Firefox either.
QA Contact: paul.silaghi
Depends on: 887773
I've started testing this on today's nightly.
Couple of issue so far:
1. 'Block plugin' requires page reload. Cannot be done instantly?
2. Unable to whitelist if the plugin is in 'allow now'. You have to block the plugin or restart the browser first.
-For a single plugin, there is no 'allow and remember'
-For multiple plugins the options are available, but switching to 'allow and remember' and viceversa has no effect.
(In reply to Ferdinand from comment #42)

I think there's some misunderstandings about how the updated feature works, and I believe it will actually be better for the use cases you outlined below.

> I always thought click to play was a great feature for a browser to have. A
> lot of extensions give you mostly the functionality of click to play but
> also add a lot of other stupid stuff that changes how Firefox works. The
> previous click to play worked great.
> Now I will have to click allow once in the upper left hand corner of my
> 24inch monitor each time I want to watch a youtube video.

Actually, if you click to allow a plugin, it will be enabled on the entire site. So you don't have to click on every youtube video on each page, for example. This is something you would have had to do on the previous implementation. 

We've enabled click-to-play via the doorhanger for a few key reasons:
1. It is better protection against click-jacking
2. It allows us to give the user clearer choices for enabling the plugin "long term" (more on this below). The previous implementation made it easy for the user to enable the plugin once, but he/she would have to do it every time they visit the site.
3. It give us a more consistent experience for plugins that are visible on the page and those that are smaller / invisible.


> My mother won't understand your doorhanger and once she has clicked allow for each website
> she uses she will no longer be nagged by it. Of course she won't be
> protected by Firefox either.

We've added nuance to the way that the plugin doorhanger works for this very case. I also do not want a user to remain unprotected just because he or she has said that the plugin should be enabled all the time. So now, the button choices are:

1. Allow Now
- will enable the plugin on the entire site with an inactivity timer (currently set to 60 minutes, I believe). If the user does not engage with the site for 60 minutes, or if he closes Firefox, the plugin will be reblocked

2. Allow and Remember
- will enable the plugin on the entire site with a longer inactivity timer (currently set to 3 months, I believe).If the user does not engage with the site for 3 months the plugin will be reblocked. 

Besides this, we also have different messages to block the plugin if its status changes-- if it becomes outdated, if we know there are vulnerabilities, etc.
(In reply to Larissa Co [:lco] from comment #44)
> Actually, if you click to allow a plugin, it will be enabled on the entire
> site. So you don't have to click on every youtube video on each page, for
> example. This is something you would have had to do on the previous
> implementation. 

This is something that a specific class of advanced users, like me and possibly others here, really liked with the old implementation, though. Unfortunately, this isn't possible any more as we deemed it too confusing for "normal" users.
I really only like to activate Flash for very specific uses, though, and usually only this one time - I found it even saves me CPU time (and therefore probably power) to only run Flash videos on commonly used sites on demand. Still, it's a small audience this appeals to.
Maybe it is the fact that English is not my original language, but if I set plug-ins to "click to play" I expect for no plug-in elements to play until clicked.

Another thing I'm against is the special casing of Adobe Flash, I understand is pervasive, but is still a plug-in so if mozilla is advertising a "click to play" feature for plug-ins yet it allows one plug-in because it is popular there's a broken promise right there.

Finally in 99% of the cases I want to allow maybe one Flash element that is useful and not the bunch of others that are ads or some other form of annoyances.

Guess is time to go see if Flashblock still works on Nightly.
(In reply to Larissa Co [:lco] from comment #44)
> (In reply to Ferdinand from comment #42)
> 
> I think there's some misunderstandings about how the updated feature works,
> and I believe it will actually be better for the use cases you outlined
> below.
> 
> > I always thought click to play was a great feature for a browser to have. A
> > lot of extensions give you mostly the functionality of click to play but
> > also add a lot of other stupid stuff that changes how Firefox works. The
> > previous click to play worked great.
> > Now I will have to click allow once in the upper left hand corner of my
> > 24inch monitor each time I want to watch a youtube video.
> 
> Actually, if you click to allow a plugin, it will be enabled on the entire
> site. So you don't have to click on every youtube video on each page, for
> example. This is something you would have had to do on the previous
> implementation. 
> 
> We've enabled click-to-play via the doorhanger for a few key reasons:
> 1. It is better protection against click-jacking
> 2. It allows us to give the user clearer choices for enabling the plugin
> "long term" (more on this below). The previous implementation made it easy
> for the user to enable the plugin once, but he/she would have to do it every
> time they visit the site.
> 3. It give us a more consistent experience for plugins that are visible on
> the page and those that are smaller / invisible.
> 
> 
> > My mother won't understand your doorhanger and once she has clicked allow for each website
> > she uses she will no longer be nagged by it. Of course she won't be
> > protected by Firefox either.
> 
> We've added nuance to the way that the plugin doorhanger works for this very
> case. I also do not want a user to remain unprotected just because he or she
> has said that the plugin should be enabled all the time. So now, the button
> choices are:
> 
> 1. Allow Now
> - will enable the plugin on the entire site with an inactivity timer
> (currently set to 60 minutes, I believe). If the user does not engage with
> the site for 60 minutes, or if he closes Firefox, the plugin will be
> reblocked
> 
> 2. Allow and Remember
> - will enable the plugin on the entire site with a longer inactivity timer
> (currently set to 3 months, I believe).If the user does not engage with the
> site for 3 months the plugin will be reblocked. 
> 
> Besides this, we also have different messages to block the plugin if its
> status changes-- if it becomes outdated, if we know there are
> vulnerabilities, etc.

You have misunderstood my usage. I have a lot of youtube tabs and they all start playing when they are loaded. Each background tab I open with youtube begins playing. Like alex_mayorga said we don't want any plugins until we click on that select plugin. (It is also a great way of blocking most annoying ads)
Plugins should not be part of the web and IMHO should stay gray boxes until activated. If you want to add a temporary allow all plugins for this tab go for it. But please keep blocking plugins by default until asked for by the user. Plugins are not welcome guests.
Please take discussion to the thread on dev-firefox.
(In reply to Paul Silaghi [QA] from comment #43)
> I've started testing this on today's nightly.
> Couple of issue so far:
> 1. 'Block plugin' requires page reload. Cannot be done instantly?
> 2. Unable to whitelist if the plugin is in 'allow now'. You have to block
> the plugin or restart the browser first.
> -For a single plugin, there is no 'allow and remember'
> -For multiple plugins the options are available, but switching to 'allow and
> remember' and viceversa has no effect.
3. The multiple plugins on a page scenario doesn't look so effective to me. If I click on a plugin content, it means I want to enable that plugin. In this case I need to change the plugin from the "blocked" state first, unlike the single plugin scenario, where you don't need to change anything. Why not set the clicked plugin on "allow" directly ?
4. Where is this coming from ? http://img69.imageshack.us/img69/1476/sezo.png
I clicked on "update" => it took me to http://java.com/en/download/index.jsp
Now everytime I go to a java page and try to activate java, I'm redirected instantly to http://java.com/en/download/index.jsp, without being able to play any java content, even with a NEW profile. Finally after about 1h, the choice was somehow forgotten and everything was back to normal, the security window appeared again.
Larissa, Benjamin, any thoughts please ?
You should probably read the bug deps ;-)

> > 1. 'Block plugin' requires page reload. Cannot be done instantly?

Bug 883404.

> > 2. Unable to whitelist if the plugin is in 'allow now'. You have to block
> > the plugin or restart the browser first.
> > -For a single plugin, there is no 'allow and remember'

This is by design.

> > -For multiple plugins the options are available, but switching to 'allow and
> > remember' and viceversa has no effect.

This is a bit surprising, please file.

> 3. The multiple plugins on a page scenario doesn't look so effective to me.
> If I click on a plugin content, it means I want to enable that plugin. In
> this case I need to change the plugin from the "blocked" state first, unlike
> the single plugin scenario, where you don't need to change anything. Why not
> set the clicked plugin on "allow" directly ?

Because we want to show people the current state. And because we expect multiple plugins to be an edge case that mainly is going to happen if an attack site is trying to exploit plugins, so we actually want it to be a bit harder to make changes without making an explicit decision.

> 4. Where is this coming from ? http://img69.imageshack.us/img69/1476/sezo.png
> I clicked on "update" => it took me to http://java.com/en/download/index.jsp
> Now everytime I go to a java page and try to activate java, I'm redirected
> instantly to http://java.com/en/download/index.jsp, without being able to
> play any java content, even with a NEW profile. Finally after about 1h, the
> choice was somehow forgotten and everything was back to normal, the security
> window appeared again.

This is a Windows security feature which has nothing to do with our UI. If you have an up-to-date Java this shouldn't show up.
Depends on: 883404
No longer blocks: 888292
Depends on: 888292
(In reply to Paul Silaghi [QA] from comment #49)
> 4. Where is this coming from ? http://img69.imageshack.us/img69/1476/sezo.png
> I clicked on "update" => it took me to http://java.com/en/download/index.jsp
> Now everytime I go to a java page and try to activate java, I'm redirected
> instantly to http://java.com/en/download/index.jsp, without being able to
> play any java content, even with a NEW profile. Finally after about 1h, the
> choice was somehow forgotten and everything was back to normal, the security
> window appeared again.
> Larissa, Benjamin, any thoughts please ?

This is a known Java issue, see https://www.java.com/en/download/faq/expire_date.xml#redirect
This change is Windows Vista UAC all over again. How many people are just going to allow all plugins thus voiding any security benefits thus might have. There was nothing wrong with the previous implementation which matched both chrome and opera.

How dumb are these users you used to justify gutting one of the few new features which actually added something to Firefox rather than removing it. Where is this user research? What is the point of removing months of work for an inferior implementation?
Depends on: 888510
(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #45)
> (In reply to Larissa Co [:lco] from comment #44)
> > Actually, if you click to allow a plugin, it will be enabled on the entire
> > site. So you don't have to click on every youtube video on each page, for
> > example. This is something you would have had to do on the previous
> > implementation. 
> 
> This is something that a specific class of advanced users, like me and
> possibly others here, really liked with the old implementation, though.
> Unfortunately, this isn't possible any more as we deemed it too confusing
> for "normal" users.
> I really only like to activate Flash for very specific uses, though, and
> usually only this one time - I found it even saves me CPU time (and
> therefore probably power) to only run Flash videos on commonly used sites on
> demand. Still, it's a small audience this appeals to.

In my opinion CtP is already an advanced feature. I would assume that anyone using it is probably an advanced user, so there was really no reason to dumb it down. The way it is now, it's far less useful for those that would seek to use it. And maybe a little easier to use for those that probably won't even know it exists.
Depends on: 888705
Blocks: 875724
Depends on: 889331
Depends on: 888908
Depends on: 887617
No longer depends on: 887617
Depends on: 889788
I'm having some problems with this change in my setup, I have the navigation toolbar hidden at all times, so navigating to a webpage with a plugin makes the doorhanger show all the time, and it's annoying because I have to click in the webpage to hide it.

It's just too intrusive if you have the navigation toolbar hidden.
Comment 54, see the dependency tree and bug 888908.
Is the patch to bug 888908 going to be applied at some point or is it disregarded?
Depends on: 895939
Depends on: 896418
Depends on: 896435, 896965, 896676, 896438
Depends on: 896995
No longer depends on: 896438
No longer depends on: 896995
No longer depends on: 896676
No longer depends on: 896435
Depends on: 897479
Depends on: 801406
Depends on: 901451
Depends on: 902444
Depends on: 905084
Depends on: 916593
Depends on: 916542
Depends on: 949486
Depends on: 966767
If this feature is going to be useful, it has to be easy to use. Right now, it takes an advanced user to activate it (about:config) and it's done to cater to non-advanced users (must double-confirm every click!).

To be constructive, I suggest the following compromise: have the option to disable the hanger confirmation. This can be done either in 1) the hanger itself, or 2) in the FF options where you activate click-to-play.
Depends on: 962007
Depends on: 1258687
Depends on: 1343584
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: