Closed Bug 600505 Opened 14 years ago Closed 13 years ago

App update preference ui favors disabling app update

Categories

(Firefox :: Settings UI, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 10
Tracking Status
blocking2.0 --- -

People

(Reporter: robert.strong.bugs, Assigned: steffen.wilberg)

References

Details

(4 keywords, Whiteboard: [sg:want P3])

Attachments

(3 files, 9 obsolete files)

The preference ui displays

Automatically check for updates to:
[X] Firefox
[X] Add-ons
[X] Search Engines

When updates to Firefox are found:
O Ask me what I want to do
0 Automatically download and install update
  [X] Warn me if this update will disable any of my add-ons


If someone doesn't want updates to be automatically downloaded they might just read the first couple of items on the page and not notice there are other options since they are several lines further down in the ui and instead choose to disable update checks completely.

I don't necessarily like the ui Windows has for these settings but I do think the order of choices is better than what we have.

Firefox has:
1. enable (with individual settings further down the page) or disable entirely
2. N/A for app update
3. N/A for app update
4. individual settings mentioned previously

Windows has:
1. Install updates automatically (recommended)
2. Download update but let me choose whether to install them
3. Check for updates but let me choose whether to download and install them
4. Never check for updates (not recommended)
Rather late in the game but people disabling app update checks is a "bad thing" so requesting blocking to see if we can't get a better ui for Firefox 4.0
blocking2.0: --- → ?
Keywords: uiwanted
This is another one that hurts, but I don't think we can block on this. I don't think it's affecting a significant number of our users.

Good prefs polish to get if/when we re-write the entire dialog, though. Something akin to comment 0 is what we want, which gives clearer options.
blocking2.0: ? → -
If we want to just quickly invert the two sections, so "When update to Firefox are found:" appears first, that's fine.  Otherwise we'll clean this up later when we refactor all of the options.
(In reply to comment #3)
> If we want to just quickly invert the two sections, so "When update to
> Firefox are found:" appears first, that's fine.  Otherwise we'll clean this
> up later when we refactor all of the options.

Yeah, this would be a quick fix, just switch the order of the two sections.
Taking.

(In reply to comment #0)
> Windows has:
> 1. Install updates automatically (recommended)
> 2. Download update but let me choose whether to install them
> 3. Check for updates but let me choose whether to download and install them
> 4. Never check for updates (not recommended)

Should we support case #2 (download but not install) at all?
#1 would be app.update.enabled and app.update.auto set to true (default values).
#3 would be app.update.enabled == true and app.update.auto == false.
#4 would be app.update.enabled == false and app.update.auto == false.

What about the rest of the current options:
  Automatically check for updates to:
  [X] Add-ons
  [X] Search Engines
Check for those updates in case #1 and #3, and don't check in #4?
Assignee: nobody → steffen.wilberg
Blocks: 600500
In comment #0 I wasn't stating that we should have the exact same options as Windows. It was only an example for UX.
Attached image screenshot (obsolete) —
Robert: too late, you already had me hooked :)
Attachment #541216 - Flags: ui-review?(faaborg)
Attached patch patch (obsolete) — Splinter Review
In 'Check for updates, but let me choose whether to download and install them', should there not be a 'Warn me if this will disable any of my add-ons' checkbox?

Also it would be handy if you threw in a 'Check for updates, and say Firefox is up to date even if it has failed to reach the update server' checkbox. Bug 648967
(In reply to comment #9)
> In 'Check for updates, but let me choose whether to download and install
> them', should there not be a 'Warn me if this will disable any of my
> add-ons' checkbox?
There shouldn't though it would be fine to inform the user that when manually choosing whether to download you will always be informed if there are incompatible add-ons.

> Also it would be handy if you threw in a 'Check for updates, and say Firefox
> is up to date even if it has failed to reach the update server' checkbox.
> Bug 648967
That should not be a preference and is already covered by Bug 648967. Please don't try to get your bugs fixed in other bugs
Comment on attachment 541216 [details]
screenshot

yeah this is clearer than what we currently have, sorry about the delay.

small nit, mentioning if the update is downloaded seems kind of peripheral to the overall choice, and increases the complexity (one would assume that an update must be downloaded before it is installed).

Also it seems like we should mention why we are making our recommendations.  

How about:

(*) Automatically install updates (recommended: improved security)
     [x] warm me if this will disable nay of my add-ons
( ) Check for updates, but let me choose whether to install them
( ) Never check for updates (not recommend: security risk)
Attachment #541216 - Flags: ui-review?(faaborg) → ui-review+
Attached patch patch v1 (obsolete) — Splinter Review
With strings from comment 11, and more comments.
Attachment #541219 - Attachment is obsolete: true
Attachment #549507 - Flags: review?(gavin.sharp)
Attached patch patch v1.1 (obsolete) — Splinter Review
Unbitrotted, and fixed double accesskey.
Attachment #549507 - Attachment is obsolete: true
Attachment #551108 - Flags: review?(gavin.sharp)
Attachment #549507 - Flags: review?(gavin.sharp)
updateManual.label has a typo ("not recommend").
Attached patch patch v1.2 (obsolete) — Splinter Review
Fixed that typo.
Gavin, should I pick another reviewer?
Attachment #551108 - Attachment is obsolete: true
Attachment #555833 - Flags: review?(gavin.sharp)
Attachment #551108 - Flags: review?(gavin.sharp)
Attached patch patch v1.3 (obsolete) — Splinter Review
Faaborg suggested a further tweak in bug 600500 comment 14.
So now we have:
(*) Automatically install updates (recommended: improved security)
    [x] Warn me if this will disable any of my add-ons
( ) Check for updates, but let me choose whether to download and install them
( ) Never check for updates (not recommended: security risk)
Attachment #555833 - Attachment is obsolete: true
Attachment #555843 - Flags: review?(gavin.sharp)
Attachment #555833 - Flags: review?(gavin.sharp)
Keywords: uiwanted
Comment on attachment 555843 [details] [diff] [review]
patch v1.3

Robert, could you review this, please? Your queue is much shorter than Gavin's, and you reported the bug...
Attachment #555843 - Flags: review?(gavin.sharp) → review?(robert.bugzilla)
Oops, patch v1.2 and 1.3 are identical.
Will update updateCheck.label per bug 600500 comment 14 in the next iteration.
Comment on attachment 555843 [details] [diff] [review]
patch v1.3

Took a quick look and see that you have changed the updateHistory.accesskey entity value without changing the name. Locales like en-GB at the very least need to see this change. Please keep updateHistory.label in sync with the entity name change when you fix the above. Please verify this isn't the case else where as well.

The following text seems rather awkward:
Automatically check for &brandShortName; updates:
   Automatically install updates (recommended: improved security)

I think it would read better as
&brandShortName; updates:
   Automatically install updates (recommended: improved security)

Please run that suggested text by faaborg and if he doesn't agree that's fine.

minusing mainly due to the entity name not being changed and to check with faaborg regarding the text.
Attachment #555843 - Flags: review?(robert.bugzilla) → review-
(In reply to Robert Strong [:rstrong] (do not email) from comment #20)
> I think it would read better as
> &brandShortName; updates:
>    Automatically install updates (recommended: improved security)
> 
> Please run that suggested text by faaborg and if he doesn't agree that's
> fine.

I agree that this reads better too. Just in case you need an opinion from the UX team and Faaborg is busy. :)
Attached patch patch v1.4 (obsolete) — Splinter Review
1. Dropped the unnecessary change of updateHistory.accesskey.
2. Bumped enableAddonsUpdate2.label and .accesskey to 3 because I have to change the accesskey. Other entities are either new or unchanged.
3. Used "&brandShortName; updates:" as suggested by comment 20.
4. Renamed the second heading from "Automatically check for updates to" (Add-ons, Search Engines) to "Automatically update:" to make it fit better, and more precise (it not only checks, but also updates silently).
5. Wrapped the two sections into proper groupboxes like almost everywhere else in the pref window, and removed the separators.
Attachment #541216 - Attachment is obsolete: true
Attachment #555843 - Attachment is obsolete: true
Sadly there's no border around the groupboxes on Linux, but the captions are bold, and the contents are indented.
Attachment #562898 - Flags: ui-review?(faaborg)
Attachment #562898 - Attachment description: screenshot of patch v1.4 → Linux screenshot of patch v1.4
Attached file Windows screenshot of patch v1.5 (obsolete) —
Attachment #563946 - Flags: ui-review?(faaborg)
Attached patch patch v1.5 (obsolete) — Splinter Review
Had to remove the align="start" attribute of the tabpanel element to make the width of the groupboxes extend to the width of the tabpanel. The other tabpanels don't have this attribute either.
Attachment #562894 - Attachment is obsolete: true
Comment on attachment 563946 [details]
Windows screenshot of patch v1.5

This is the patch and not the Windows screenshot
Oops.
Attachment #563946 - Attachment is obsolete: true
Attachment #563948 - Flags: ui-review?(faaborg)
Attachment #563946 - Flags: ui-review?(faaborg)
Comment on attachment 563948 [details]
Windows screenshot of patch v1.5

Looks good!
Comment on attachment 562898 [details]
Linux screenshot of patch v1.4

looks good
Attachment #562898 - Flags: ui-review?(faaborg) → ui-review+
Comment on attachment 563948 [details]
Windows screenshot of patch v1.5

looks good
Attachment #563948 - Flags: ui-review?(faaborg) → ui-review+
The Windows screenshot looks very good.  But, shouldn't the line that says "not recommend: security risk" actually say "not recommended: security risk"?  It would flow better with the line above that says "recommended: improved security".
(In reply to Jeff Grossman from comment #32)
That's a typo, thanks for catching that!
Attached patch patch v1.6Splinter Review
Typo fixed.
Attachment #563947 - Attachment is obsolete: true
Attachment #564662 - Flags: review?(robert.bugzilla)
Steffen, patch looks good but before I r+ this I am going to run through the different configurations to verify everything works as expected. I will hopefully have this reviewed by tonight but it may be a day or so. Thanks
Sure, take all the time you need, and thanks for the update.
Note that I (re-)landed this on the UX branch, so there are plenty of "-ux" nightlies to test.
(In reply to myself from comment #36)
Things not to tell your reviewer:
#1: "take all the time you need" ;-)
I promise, I'm close to being done though I was close last week as well. ;)
Comment on attachment 564662 [details] [diff] [review]
patch v1.6

Looks good and everything worked as expected with a disable updater build. It might be a good thing to have finer control over what controls are disabled when locking prefs now that there is an additional control but I'm not too worried about that and think we should wait to see if providing that would be of any value.

># HG changeset patch
># Parent 70e4de45a0d0f7b54e4dbc22c177e56a9c717a42
># User Steffen Wilberg <steffen.wilberg@web.de>
>Bug 600505: Reorganize app update preference ui to not favor disabling app update. ui-review=faaborg
>
>diff --git a/browser/components/preferences/advanced.js b/browser/components/preferences/advanced.js
>--- a/browser/components/preferences/advanced.js
>+++ b/browser/components/preferences/advanced.js
>...
>   /**
>-   * Enables/disables UI for "when updates are found" based on the values,
>-   * and "locked" states of associated preferences.
>+   * Sets the pref values based on the selected item of the radiogroup,
>+   * and sets the disabled state of the warnIncompatible checkbox accordingly.
>    */
>-  updateAutoItems: function () 
>+  updateWritePrefs: function ()
>   {
>     var enabledPref = document.getElementById("app.update.enabled");
>     var autoPref = document.getElementById("app.update.auto");
>-    
>-    var updateModeLabel = document.getElementById("updateModeLabel");
>-    var updateMode = document.getElementById("updateMode");
>-    
>-    var disable = enabledPref.locked || !enabledPref.value ||
>-                  autoPref.locked;
>-    updateModeLabel.disabled = updateMode.disabled = disable;
>-  },
>+    var radiogroup = document.getElementById("updateRadioGroup");
>+    switch (radiogroup.value) {
>+      case "auto":      // 1. Automatically install updates
>+        enabledPref.value = true;
>+        autoPref.value = true;
>+        break;
>+      case "checkOnly": // 2. Check, but but let me choose
s/but but/but/

r=me with that
Attachment #564662 - Flags: review?(robert.bugzilla) → review+
https://hg.mozilla.org/mozilla-central/rev/2d9076dbcd15
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 10
Whiteboard: [inbound] → [inbound][sg:want P3]
Whiteboard: [inbound][sg:want P3] → [sg:want P3]
The second radio button should read "Check for updates, but let me choose whether to download and install them" like in Windows (7) update settings.
(In reply to sdrocking from comment #42)
> The second radio button should read "Check for updates, but let me choose
> whether to download and install them" like in Windows (7) update settings.

Would seem the comment 16 attachment didn't include the wording change that the attachment comment mentioned.
Indeed. Please file a followup-bug if you think that needs to be changed.
Depends on: 697385
Depends on: 701987
You need to log in before you can comment on or make changes to this bug.