523784 - Soft blocked items are not disabled if user clicks "Cancel"

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Justin Dolske [:Dolske]]

I just restarted Minefield (trunk) on my Windows 7 box, and after it launched I got the blocklist prompt warning me about the WPF plugin that we blocklisted last weekend (bug 522777). The prompt has two buttons -- "Restart Now" and "Cancel". I clicked Cancel, and then restarted myself a bit later. I was surprised to find that the plugin was not disabled (it shows the red "!" tag on the icon, and has a "Disable" button waiting to be clicked).

Two things should be fixed here:

1) The meaning of "Cancel" is unclear. "Restart Later" would be better, and we should nag the user every, say, hour to restart. [Note sure what to do if the user unchecks the checkbox to *not* block the plugin... Dynamically change the dialog buttons so there's just "Ok"?]

2) If I do select Cancel / Restart Later, the plugin should be blocked upon the next restart. It wasn't, perhaps because we're taking "Cancel" to mean "I don't want to block this." Which isn't a good assumption, seeing as the user's only other choice is an immediate restart.

Nominating for blocking, because this significantly reduces the effectiveness of the blocklist.

Comment 1

[Dave Townsend [:mossop]]

This only impacts the soft-block case. If the user clicks cancel we don't mark the soft-blocked items to be disabled on the next restart, regardless of whether the checkbox is ticked or not. Hard blocked items will still get blocked after the restart.

Comment 2

[Mike Beltzner [:beltzner, not reading bugmail]]

Can someone point me to a screenshot of what this looks like? I agree that soft-blocking should allow the user to keep the add-on enabled, but it sounds like the checkbox does that?

Boriss: can you work with Dave and Justin to figure out if it's possible to do this without altering strings? Don't think it blocks, but I'd take a patch here.

Comment 3

[Dave Townsend [:mossop]]

Here is example UI: https://bug455906.bugzilla.mozilla.org/attachment.cgi?id=345402. Right now if the checkbox is ticked but the user clicks cancel then the add-on does not get disabled.

Comment 4

[Justin Scott [:fligtar]]

This is a pretty good explanation of why 75% of a recently softblocked add-on are still enabled. Softblocking is basically worthless with this bug: everyone is going to hit cancel.

The *only* way a user should be able to opt out of a softblock is by unchecking the box. 

I pretty much never request blocking, but doing it for this bug. This bug makes the decision between hardblocking and softblocking one of "do we really want to block this or should we just whisper a suggestion?" rather than what we've been using "is this so ridiculously serious that users shouldn't have the ability to opt out or is it just normal levels of horrible?"

Comment 5

[Justin Scott [:fligtar]]

I also think this should be backported to 3.x

Comment 6

[Alex Faaborg [:faaborg] (Firefox UX)]

>The *only* way a user should be able to opt out of a softblock is by unchecking
>the box. 

I strongly agree with this

>I pretty much never request blocking, but doing it for this bug.

I also agree that this needs to be a hardblocker, we punted on 596343 with the logic that we could take care of the extreme performance cases with extension blocking, but only 25% effectiveness is just going to push users to other products (when they are in an extreme performance case)

Comment 7

[Mike Beltzner [:beltzner, not reading bugmail]]

removing uiwanted; we should tick the box by default (which I think we do) and apply in even when the user clicks cancel in the soft-block case. For extra points, replace "Cancel" with "OK" (borrow it from elsewhere) since really, that's what that button does once we make that change.

Comment 8

[Mehdi Mulani [:mmm] (I don't check this)]

The "Restart Later" string is already in tree:
http://mxr.mozilla.org/mozilla-central/source/toolkit/locales/en-US/chrome/mozapps/update/updates.properties#55
and we could mess with the |init| code for the dialog to use it there.

Comment 9

[Mehdi Mulani [:mmm] (I don't check this)]

planning to hack this out later today.

Comment 10

[Mehdi Mulani [:mmm] (I don't check this)]

Attachment: Patch v1.

Apparently the string freeze means we can't even copy strings, so I pulled the one in from the updater.

I'm a bit concerned about the functionality, I ran a few test cases and it seemed to work but I wasn't sure of how to test this, or even if we have the functionality in place.

Comment 11

[Mehdi Mulani [:mmm] (I don't check this)]

Attachment: Screenshot with patch v1 applied.

Comment 12

[Justin Dolske [:Dolske]]

Comment on attachment 512001 [details] [diff] [review]
Patch v1.

Driveby!

>--- a/toolkit/mozapps/extensions/content/blocklist.js
>+  // XXX: In bug 523784, we want to change the text of the "Cancel" button
>+  // in the dialog without a string change. To do this we use strings from the
>+  // "updates.properties" bundleset.
>+  var updateStringsBundle = document.getElementById("updateStrings");

Since you're using script here anyway, seems like you might as well just use the string bundle service... (probably need to Cu.import() it first).

  let bundle = Services.strings.createBundle("chrome://mozapps/locale/update/updates.properties");
  let label = bundle.GetStringFromName("restartLaterButton");

>+  var cancelButton = document.
>+    getAnonymousElementByAttribute(document.documentElement, "dlgtype", "cancel");

  let cancelButton = document.documentElement.getButton("cancel");

Comment 13

[Mehdi Mulani [:mmm] (I don't check this)]

(In reply to comment #12)
> >--- a/toolkit/mozapps/extensions/content/blocklist.js
> >+  // XXX: In bug 523784, we want to change the text of the "Cancel" button
> >+  // in the dialog without a string change. To do this we use strings from the
> >+  // "updates.properties" bundleset.
> >+  var updateStringsBundle = document.getElementById("updateStrings");
> 
> Since you're using script here anyway, seems like you might as well just use
> the string bundle service... (probably need to Cu.import() it first).
> 
>   let bundle =
> Services.strings.createBundle("chrome://mozapps/locale/update/updates.properties");
>   let label = bundle.GetStringFromName("restartLaterButton");
Woo-hoo! Fewer hacks in less places.


> >+  var cancelButton = document.
> >+    getAnonymousElementByAttribute(document.documentElement, "dlgtype", "cancel");
> 
>   let cancelButton = document.documentElement.getButton("cancel");
Less code in fewer lines, I like it.

Thanks for the feedback, dolske.
Will post a patch later today or tomorrow with the fixes.

Comment 14

[Mehdi Mulani [:mmm] (I don't check this)]

Attachment: Patch v2.

Comment 15

[Mehdi Mulani [:mmm] (I don't check this)]

Comment on attachment 512001 [details] [diff] [review]
Patch v1.

Whoops, hit enter too soon.

Second patch incorporates dolske's feedback.

Comment 16

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 512067 [details] [diff] [review]
Patch v2.

As discussed on IRC: Missing the accesskey.

Comment 17

[Mehdi Mulani [:mmm] (I don't check this)]

Attachment: Patch v3.

Patch v3.

Adds accesskey and switched to using setAttribute for the label.

Comment 18

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 512073 [details] [diff] [review]
Patch v3.

Someone wanna grace this with either blocking+ or approval+? See comment 4 for justification.

Comment 19

[Mehdi Mulani [:mmm] (I don't check this)]

CC'ing Beltzner as he brought this to my attention and I think he wants this to block.

About backporting this to 1.9.1 and 1.9.2, is it alright if we use the same hack? This will make it easier to maintain and clean up post Firefox 4.

Comment 20

[Alex Limi (:limi) — Firefox UX Team]

I agree, this should probably block, especially considering we didn't get the ability to opt in to third-party extensions in time for FF4. 

The only defense mechanism we have right now is this, and it's essentially useless since nobody wants to restart the browser as the very first thing they do once it has started.

Comment 21

[Justin Dolske [:Dolske]]

Let's call this a blocker -- we've already shipped previous releases with this behavior, but Fligtar's data indicates that this makes softblocks ineffective, and thus removes an important tool for dealing with problematic addons.

Comment 22

[Justin Dolske [:Dolske]]

(In reply to comment #19)

> About backporting this to 1.9.1 and 1.9.2, is it alright if we use the same
> hack? This will make it easier to maintain and clean up post Firefox 4.

Yes; we _very_ rarely change strings on branch.

Comment 23

[Mehdi Mulani [:mmm] (I don't check this)]

CC'ing Axel as this relates to localizations.

Comment 24

[Mehdi Mulani [:mmm] (I don't check this)]

Attachment: Tests v1.

This tests both the functionality of the "Restart Later" button and that it is renamed from "Cancel".

Comment 25

[Axel Hecht [:Pike]]

Softblockers final with strings don't make sense, and string re-use isn't really compelling. In particular if it's a "found this one in the tree" like comment 8 says. Semantics that are subtle in English can turn things upside down in other languages.

IMHO, we should punt this off to post fx4, given how often we already shipped this.

Comment 26

[Mike Shaver (:shaver emeritus)]

The meaning here is the same, I don't understand which semantics you're concerned about.  Could you give a concrete example?  In the previous location we're restarting the browser to apply a change, and we're doing the same thing now.

If there's a string problem here, let's discuss the specifics of it, but we can't reason about it in the abstract and theoretical.

Comment 27

[Mehdi Mulani [:mmm] (I don't check this)]

Attachment: Test v2.

Changed testing around closing of window to avoid a possible intermittent orange.
Also cleaned up comparison function messages.

Comment 28

[Axel Hecht [:Pike]]

String re-use is, in general, searching for the counter-example localization. There's still stuff that surprises me, so please don't ask me to comment with a "yeah, that's obviously fine for all languages".

This one *may* be safe, in the sense that I don't know a counter example.

That's as fishy as it is.

Comment 29

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 512321 [details] [diff] [review]
Test v2.

Looks good.

>+  let windowObserver = function(aSubject, aTopic, aData) {
>+    if (aTopic != "domwindowopened")
>+      return;

Nit: Might want to be extra paranoid and check the location of the document this event is for (and just return early if its for a different document).

Comment 30

[Mike Shaver (:shaver emeritus)]

Thanks, Axel.  I think the risk here is outweighed by the benefits, especially given that we're relying increasingly on the blocklist for FF4 correctness and stability.

Axel: could you send a note to localizers-all saying that we're going to reuse the string, and asking for people to let us know if that causes a problem for their locale?

Comment 31

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to comment #29)
> Nit: Might want to be extra paranoid and check the location of the document
> this event is for (and just return early if its for a different document).

Clarification: I'll leave it up to you to decide how paranoid you want to be today.

Comment 32

[Bill Gianopoulos [:WG9s]]

It seems to me that if it says something is going to be softblocked "Cancel" can have no other meaning than "Don't do it!"

I see that perhaps you don;t want to restart right away, and i have no idea how to do this during a string freeze period, but clearly this needs three choices.  "Don't Block", "Block doing Restart now" and "Block doing Restart later".

Comment 33

[Axel Hecht [:Pike]]

Oh, drats, found something:

This is significantly changing the width of the UI elements, and could require changes to blocklist.style. Both 'pl' and 'gd' have long translations and already had to bump the width of the blocklist dialog, you'd basically had to run through all locales and check for regressions.

mxr queries at hand are http://mxr.mozilla.org/l10n-central/search?string=restartLaterButton&find=updates.properties and http://mxr.mozilla.org/l10n-central/search?string=blocklist.style&find=blocklist.dtd$ (I wish how to test good for CSS vals in l10n)

Comment 34

[Mike Beltzner [:beltzner, not reading bugmail]]

Nice catch, Axel.

The other option was to re-use something like the general wizard "OK" string. It will make the dialog a little less usable (OK is a strange action to take there) but again, the benefits outweigh that oddity.

Comment 35

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to comment #33)
> already had to bump the width of the blocklist dialog, you'd basically had to
> run through all locales and check for regressions.
> 
> (I wish how to test good for CSS vals in l10n)

If there is pressure on it we could come up with a Mozmill test. I can execute it against any localized build for the blocklist dialog.

Comment 36

[Axel Hecht [:Pike]]

Henrik, independent of this bug, the blocklist dialog sounds like a good candidate to add to the l10n crop tests. Not sure how much work it is to convert the testcase in this bug to one. Also, how this dialog behaves if you add plenty of add-ons.

Comment 37

[Ed Morley [:emorley]]

I'm presuming this bug is reason there were 49K crashes due to Skype in the last 14 days (bug 546632 comment 49), even though it has been softblocked since 20th Jan (bug 627278)? 

Once this patch lands, will people who've clicked cancel previously, get shown the dialog again on next app launch? If not, will it at least get shown by the extensions compatibility checker during the user's next minor update?

Comment 38

[Axel Hecht [:Pike]]

Can we get a screenshot of the dialog without this patch to have something to compare to? No idea how I'd bring up that dialog myself right now.

Comment 39

[Mehdi Mulani [:mmm] (I don't check this)]

Attachment: Screenshot without patch or addons.

I brought this one up without any add-ons.

An easy way to bring up this dialog in recent versions of Firefox is to navigate to about:config, open the Web Console (Tool -> Web Console) and run the following bit of JS:

var ww = Components.classes["@mozilla.org/embedcomp/window-watcher;1"].getService(Components.interfaces.nsIWindowWatcher);
ww.openWindow(null, "chrome://mozapps/content/extensions/blocklist.xul", "", "chrome,centerscreen,dialog,modal,titlebar", {});

Note that it is a modal dialog.

Comment 40

[Mehdi Mulani [:mmm] (I don't check this)]

(In reply to comment #39)
> Created attachment 512513 [details]
> Screenshot without patch or addons.
> 
> I brought this one up without any add-ons.
> 
> An easy way to bring up this dialog in recent versions of Firefox is to
> navigate to about:config, open the Web Console (Tool -> Web Console) and run
> the following bit of JS:
> 
> var ww =
> Components.classes["@mozilla.org/embedcomp/window-watcher;1"].getService(Components.interfaces.nsIWindowWatcher);
> ww.openWindow(null, "chrome://mozapps/content/extensions/blocklist.xul", "",
> "chrome,centerscreen,dialog,modal,titlebar", {});
> 
> Note that it is a modal dialog.

Actually, pasting that bit into the Error Console also works and is one step quicker :P

Comment 41

[Axel Hecht [:Pike]]

Attachment: polish fx4 status quo screenshot

For reference, this is how the UI looks in Polish with the standard cancel right now. The substitute instead of "Anuluj" would be "Uruchom ponownie później".

Comment 42

[Axel Hecht [:Pike]]

PS: Filed bugs on the 5 locales with translated CSS: bn-IN, ml, mr, or, ta.

Comment 43

[Mehdi Mulani [:mmm] (I don't check this)]

Attachment: Basic mozmill test.

Simple mozmill test that opens the dialog and creates a screenshot.

Comment 44

[(no longer active)]

Renominating this to be a hardblocker, because it blocks bug 616518 which is a hardblocker.

Comment 45

[Axel Hecht [:Pike]]

I'm not convinced that bug 616518 is actually talking about a softblock, thus it may not actually depend on this one. Not that I understand bug 616518 technically.

Comment 46

[Mike Beltzner [:beltzner, not reading bugmail]]

Uhm, did my suggestion of using "OK" instead of "Restart Later" go ignored? Seems to solve the issue that Axel is rightly worried about.

I agree that this isn't a hard blocker. It is, however, very important, and I'm curious to know if my suggestion will help us forward.

Comment 47

[Bill Gianopoulos [:WG9s]]

It seem to me the choices are easy. It is really just a case of tyring to figure out how to make them clear without either changing strings or decideing strings need ot be changed. There are thrtee distinct choices.

1.  THnk you so much for warning me,l but I want to run the add-on anyway.

2.  I want to block the add-on and think a borwser restart to ensure that is warranted.

3.

Comment 48

[Bill Gianopoulos [:WG9s]]

(In reply to comment #47)
> It seem to me the choices are easy. It is really just a case of tyring to
> figure out how to make them clear without either changing strings or decideing
> strings need ot be changed. There are thrtee distinct choices.
> 
> 1.  THnk you so much for warning me,l but I want to run the add-on anyway.
> 
> 2.  I want to block the add-on and think a borwser restart to ensure that is
> warranted.
> 
> 3.

I have no idea how that got submitted while I was editing.

1.  Thank you so much for warning me, but I want to run the add-on anyway.

2.  I want to block the add-on and think a browser restart to ensure that is warranted.

3.  I want o block the add-on, but can;t do a restart right now.  Please block it on next restart.

However doing that and making it all work with l10n is what is needed.

There should be 3 choices, so any 2 choice solution is just wrong.  Just my opinion, though I could be wrong.

Comment 49

[Mehdi Mulani [:mmm] (I don't check this)]

Attachment: Screenshots of the dialog in almost all locales.

I ran a test through mozmill to generate these images. For kn, ml, si and te I manually tested them (as I did not have the correct fonts the first time through).

However, I was unable to test bn-BD, bn-IN and or. For each of these, I couldn't get the correct fonts installed. If anyone could try those, it would be awesome.
In order to bring up the dialog, you can evaluate the following from the Web Console on about:config,

var ww = Components.classes["@mozilla.org/embedcomp/window-watcher;1"].getService(Components.interfaces.nsIWindowWatcher);
var win = ww.openWindow(null, "chrome://mozapps/content/extensions/blocklist.xul", "",
"chrome,centerscreen,dialog,titlebar", {});
setTimeout(function() {
  var strings = Components.classes["@mozilla.org/intl/stringbundle;1"].getService(Components.interfaces.nsIStringBundleService);
  var bundle = strings.createBundle("chrome://mozapps/locale/update/updates.properties");
  win.document.documentElement.getButton("cancel").label = bundle.GetStringFromName("restartLaterButton");
}, 2000);

I was not able to get this to work in the Error Console though.

Comment 50

[Mehdi Mulani [:mmm] (I don't check this)]

Attachment: For reference, the mozmill test I used to generate the screenshots.

Comment 51

[Mike Beltzner [:beltzner, not reading bugmail]]

Couple of things:

1. Those screenshots look promising; no obvious cutoffs, though some of the screenshots showed unicode placeholders.

2. Once this gets Axel's signoff, this has implicit a=beltzner

Comment 52

[Mehdi Mulani [:mmm] (I don't check this)]

(In reply to comment #51)
> Couple of things:
> 
> 1. Those screenshots look promising; no obvious cutoffs, though some of the
> screenshots showed unicode placeholders.
Yeah, for 7 of the locales it showed placeholders. I took a screenshot for 4 of these after adding the fonts but couldn't get the last 3 to work, as per comment 49.

> 2. Once this gets Axel's signoff, this has implicit a=beltzner
Sweet!

Comment 53

[Axel Hecht [:Pike]]

a=me.

I think we should rephrase the comment in blocklist.js, though. Let's not make it a XXX comment and hack, but call it out as a feature, and file a bug if we do hit a counter example at some point.

That, or we should file a follow up bug right away and reference that in the comment. I prefer the first, though.

Comment 54

[Mehdi Mulani [:mmm] (I don't check this)]

Attachment: Patch v4.

Tiny comment change.

Comment 55

[Mehdi Mulani [:mmm] (I don't check this)]

http://hg.mozilla.org/mozilla-central/rev/a57c236d61de
http://hg.mozilla.org/mozilla-central/rev/64d579f7746b

Comment 56

[Justin Scott [:fligtar]]

Thanks Medhi! Should there be a separate bug for backporting to 3.x or can that be handled here as well?

We can start softblocking again for 4.x issues once it lands in a beta, but I still don't want to softblock in 3.x until it's fixed there.

Comment 57

[Justin Scott [:fligtar]]

Mehdi*!

Comment 58

[Ed Morley [:emorley]]

Once this patch lands, will people who've clicked cancel previously, get shown
the dialog again on next app launch? (eg for the Skype softblock) If not, will it at least get shown by the extensions compatibility checker during the user's next minor update?

Comment 59

[Axel Hecht [:Pike]]

We'll need to evaluate the l10n-impact of this patch on 1.9.2 independently, sadly.

Comment 60

[Axel Hecht [:Pike]]

Screw reload :-(

Comment 61

[Mehdi Mulani [:mmm] (I don't check this)]

Attachment: Screenshots of the dialog in Firefox 3.6

Re-ran the test through Firefox 3.6 using mozmill, looks good in all locales.

Comment 62

[Mehdi Mulani [:mmm] (I don't check this)]

Attachment: Patch and tests for 1.9.2 branch.

Had to change patch and tests to not use Services.

Comment 63

[Axel Hecht [:Pike]]

ack on the 3.6 screenshots, I'm fine with backporting this to 1.9.2

Comment 64

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 513195 [details] [diff] [review]
Patch and tests for 1.9.2 branch.

>+  let bundle = strings.
>+              createBundle("chrome://mozapps/locale/update/updates.properties");

Should line this up (or put on the same line) - it looks funny not being lined up.

Comment 65

[LegNeato]

Comment on attachment 513195 [details] [diff] [review]
Patch and tests for 1.9.2 branch.

a=LegNeato for 1.9.2.15

Comment 66

[Mehdi Mulani [:mmm] (I don't check this)]

landed on 1.9.2 as well, target: 1.9.2.15
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/6fa1baa4446a

Comment 67

[Daniel Veditz [:dveditz]]

The "3.6.15" we're releasing today does not fix this bug, the release containing this bug fix has been renamed to "3.6.16" and the bugzilla flags will be updated to reflect that soon. Today's release is a re-release of 3.6.14 plus a fix for a bug that prevented many Java applets from starting up.

Comment 68

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Dave, where do we store the decision of the user to disable the soft-blocked item or not. When I run the test a second time I do not see the checkbox anymore and the item is blocked immediately.

Comment 69

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0) Gecko/20100101 Firefox/4.0

So which tests do we have in the testsuite and which have to be created as manual tests? It's not that clear right now. Thanks.

Comment 70

[Mehdi Mulani [:mmm] (I don't check this)]

(In reply to comment #69)
> Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0)
> Gecko/20100101 Firefox/4.0
> 
> So which tests do we have in the testsuite and which have to be created as
> manual tests? It's not that clear right now. Thanks.

So, there are two parts to blocking an add-on: the UI setting a |disable| property and then the back-end removing the add-on based on the |disable| property. The test with this patch (at http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/browser/browser_bug523784.js ) tests that the UI sets the |disable| property when the "Restart Later" button is clicked.
According to Dave, http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/xpcshell/test_bug455906.js tests that the back-end.

Comment 71

[Dave Townsend [:mossop]]

Per comment 70