Closed
Bug 600505
Opened 14 years ago
Closed 13 years ago
App update preference ui favors disabling app update
Categories
(Firefox :: Settings UI, defect)
Firefox
Settings UI
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)
69.54 KB,
image/png
|
faaborg
:
ui-review+
|
Details |
41.03 KB,
image/png
|
faaborg
:
ui-review+
|
Details |
22.35 KB,
patch
|
robert.strong.bugs
:
review+
|
Details | Diff | Splinter Review |
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)
Reporter | ||
Comment 1•14 years ago
|
||
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
Comment 2•14 years ago
|
||
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.
Comment 3•14 years ago
|
||
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.
Comment 4•13 years ago
|
||
(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.
Assignee | ||
Comment 5•13 years ago
|
||
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
Reporter | ||
Comment 6•13 years ago
|
||
In comment #0 I wasn't stating that we should have the exact same options as Windows. It was only an example for UX.
Assignee | ||
Comment 7•13 years ago
|
||
Robert: too late, you already had me hooked :)
Attachment #541216 -
Flags: ui-review?(faaborg)
Assignee | ||
Comment 8•13 years ago
|
||
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
Reporter | ||
Comment 10•13 years ago
|
||
(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 11•13 years ago
|
||
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+
Assignee | ||
Comment 12•13 years ago
|
||
With strings from comment 11, and more comments.
Attachment #541219 -
Attachment is obsolete: true
Attachment #549507 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 13•13 years ago
|
||
Unbitrotted, and fixed double accesskey.
Attachment #549507 -
Attachment is obsolete: true
Attachment #551108 -
Flags: review?(gavin.sharp)
Attachment #549507 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 14•13 years ago
|
||
updateManual.label has a typo ("not recommend").
Assignee | ||
Comment 15•13 years ago
|
||
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)
Assignee | ||
Comment 16•13 years ago
|
||
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)
Assignee | ||
Comment 17•13 years ago
|
||
https://hg.mozilla.org/projects/ux/rev/74008670f6ff
Assignee | ||
Comment 18•13 years ago
|
||
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)
Assignee | ||
Comment 19•13 years ago
|
||
Oops, patch v1.2 and 1.3 are identical. Will update updateCheck.label per bug 600500 comment 14 in the next iteration.
Reporter | ||
Comment 20•13 years ago
|
||
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-
Comment 21•13 years ago
|
||
(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. :)
Assignee | ||
Comment 22•13 years ago
|
||
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
Assignee | ||
Comment 23•13 years ago
|
||
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)
Assignee | ||
Comment 24•13 years ago
|
||
https://hg.mozilla.org/projects/ux/rev/dce8c0b89e94
Assignee | ||
Updated•13 years ago
|
Attachment #562898 -
Attachment description: screenshot of patch v1.4 → Linux screenshot of patch v1.4
Assignee | ||
Comment 25•13 years ago
|
||
Attachment #563946 -
Flags: ui-review?(faaborg)
Assignee | ||
Comment 26•13 years ago
|
||
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
Reporter | ||
Comment 27•13 years ago
|
||
Comment on attachment 563946 [details]
Windows screenshot of patch v1.5
This is the patch and not the Windows screenshot
Assignee | ||
Comment 28•13 years ago
|
||
Oops.
Attachment #563946 -
Attachment is obsolete: true
Attachment #563948 -
Flags: ui-review?(faaborg)
Attachment #563946 -
Flags: ui-review?(faaborg)
Reporter | ||
Comment 29•13 years ago
|
||
Comment on attachment 563948 [details]
Windows screenshot of patch v1.5
Looks good!
Comment 30•13 years ago
|
||
Comment on attachment 562898 [details]
Linux screenshot of patch v1.4
looks good
Attachment #562898 -
Flags: ui-review?(faaborg) → ui-review+
Comment 31•13 years ago
|
||
Comment on attachment 563948 [details]
Windows screenshot of patch v1.5
looks good
Attachment #563948 -
Flags: ui-review?(faaborg) → ui-review+
Comment 32•13 years ago
|
||
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".
Assignee | ||
Comment 33•13 years ago
|
||
(In reply to Jeff Grossman from comment #32) That's a typo, thanks for catching that!
Assignee | ||
Comment 34•13 years ago
|
||
Typo fixed.
Attachment #563947 -
Attachment is obsolete: true
Attachment #564662 -
Flags: review?(robert.bugzilla)
Reporter | ||
Comment 35•13 years ago
|
||
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
Assignee | ||
Comment 36•13 years ago
|
||
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.
Assignee | ||
Comment 37•13 years ago
|
||
(In reply to myself from comment #36) Things not to tell your reviewer: #1: "take all the time you need" ;-)
Reporter | ||
Comment 38•13 years ago
|
||
I promise, I'm close to being done though I was close last week as well. ;)
Reporter | ||
Comment 39•13 years ago
|
||
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+
Assignee | ||
Comment 40•13 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/2d9076dbcd15
Keywords: user-doc-needed
Whiteboard: [inbound]
Comment 41•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2d9076dbcd15
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 10
Updated•13 years ago
|
Whiteboard: [inbound] → [inbound][sg:want P3]
Assignee | ||
Updated•13 years ago
|
Whiteboard: [inbound][sg:want P3] → [sg:want P3]
Comment 42•13 years ago
|
||
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.
Comment 43•13 years ago
|
||
(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.
Assignee | ||
Comment 44•13 years ago
|
||
Indeed. Please file a followup-bug if you think that needs to be changed.
Comment 45•13 years ago
|
||
Created Bug 697385
You need to log in
before you can comment on or make changes to this bug.
Description
•