Change wording for manual cache size override

RESOLVED FIXED

Status

()

Firefox
Preferences
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: jduell, Assigned: jduell)

Tracking

(Blocks: 1 bug)

unspecified
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 beta7+)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Created attachment 476163 [details]
Current network panel

Observe attached screenshot of the Network setting panel when "user-set cache size" is in effect.

I'm not schooled in UI, but I think the general idiom for "input field with checkbox controlling if it's greyed out" is that you check the box in order to enable the field, rather than uncheck it, as we currently have here.  

I.e., this UI would be better if the checkbox were selected instead of empty, and the text said "Manually control the size of my cache".

(My eye at least is trained to associate an unchecked box to the upper left of an indented field as "that setting is off")
Yes, agreed — sorry I didn't catch this the first time around.

If we're string-frozen, a workable solution would be to make sure the input box is grayed out too — or just hide the entire line, which is even clearer.

If we can still change strings, something like:

"Manually override cache settings (not recommended)" would work. I'm sure there's a better phrasing for this, but I'm tired right now. :)

The main point is to reinforce that in most cases, you want Firefox to be managing this for you.
(Assignee)

Comment 2

7 years ago
Requesting blocking-beta7, as this needs a string change.  I'll have a patch by end of day, assuming limi and I can work out an agreed-upon wording.  

I think we want to change both the wording for both the checkbox and the indented value.

"Override automatic cache management"
  -- "Limit cache to XX MB of space"

My current favorite:  it conveys the idea that Firefox is managing the size of the cache for you, and that you're "limiting" things if you tweak it.  Seems likely to scare away all but those who actually need it (i.e. those who are have a disk space crunch).

"Manually limit the size of my cache" 
  -- "Limit cache to XX MB of space"
      
Suitably scary, but doesn't make it clear that there still is a limit on cache size, even if you don't set this.
Blocks: 559729
blocking2.0: --- → ?
Depends on: 559942
(Assignee)

Updated

7 years ago
Summary: Smart cache UI still needs work? → Change wording for manual cache size override
(In reply to comment #2)
> I think we want to change both the wording for both the checkbox and the
> indented value.
> 
> "Override automatic cache management"
>   -- "Limit cache to XX MB of space"
> 
> My current favorite:  it conveys the idea that Firefox is managing the size of
> the cache for you, and that you're "limiting" things if you tweak it.  Seems
> likely to scare away all but those who actually need it (i.e. those who are
> have a disk space crunch).

Agreed, this is the best option so far. We could even go further and replace "automatic" with "optimized" if we really want to push the idea that the setting is doing the right thing for you, but I'm fine with either.
(Assignee)

Comment 4

7 years ago
Created attachment 477635 [details] [diff] [review]
Change wording of cache override UI elements

OK, here's the patch.  Wording already approved by limi.

Gavin: At first I thought I needed to set "disabled" for useCacheBefore/cacheSize/useCacheAfter (since they need to be greyed out initially), but then discovered that the function that greys them out gets called when the prefs panel loads.
Assignee: nobody → jduell.mcbugs
Attachment #477635 - Flags: ui-review+
Attachment #477635 - Flags: review?(gavin.sharp)
blocking2.0: ? → beta7+
Component: Networking: Cache → Preferences
Product: Core → Firefox
QA Contact: networking.cache → preferences
(Assignee)

Comment 5

7 years ago
Created attachment 477641 [details] [diff] [review]
Changed names of altered string variables
Attachment #477635 - Attachment is obsolete: true
Attachment #477641 - Flags: ui-review+
Attachment #477641 - Flags: review?(gavin.sharp)
Attachment #477635 - Flags: review?(gavin.sharp)
Comment on attachment 477641 [details] [diff] [review]
Changed names of altered string variables

>diff --git a/browser/components/preferences/advanced.js b/browser/components/preferences/advanced.js

>-  updateCacheSizeUI: function (smartSizeEnabled)
>+  updateCacheSizeUI: function (smartSizeDisabled)
>   {
>-    document.getElementById("useCacheBefore").disabled = smartSizeEnabled;
>-    document.getElementById("cacheSize").disabled = smartSizeEnabled;
>-    document.getElementById("useCacheAfter").disabled = smartSizeEnabled;
>+    document.getElementById("LimitCacheSizeBefore").disabled = smartSizeDisabled;
>+    document.getElementById("cacheSize").disabled = smartSizeDisabled;
>+    document.getElementById("LimitCacheSizeAfter").disabled = smartSizeDisabled;

I don't think you want this change - the disabled state of these elements depends on whether "smart sizing" is enabled, so .disabled = smartSizeEnabled is the correct semantic.

>   readSmartSizeEnabled: function ()
>   {
>     var enabled = document.getElementById("browser.cache.disk.smart_size.enabled").value;
>-    this.updateCacheSizeUI(enabled);
>+    this.updateCacheSizeUI(!enabled);

This could use a comment - "The smart_size.enabled preference element is inverted='true', so it's value is the opposite of the actual pref value.". Also just name the variable "disabled".

>diff --git a/browser/components/preferences/advanced.xul b/browser/components/preferences/advanced.xul

>+              <label id="LimitCacheSizeBefore" control="cacheSize"

>+              <label id="LimitCacheSizeAfter" flex="1">&LimitCacheSizeAfter.label;</label>

No need to change the IDs for these.

>diff --git a/browser/locales/en-US/chrome/browser/preferences/advanced.dtd b/browser/locales/en-US/chrome/browser/preferences/advanced.dtd

>+  The entities LimitCacheSizeBefore.label and LimitCacheSizeAfter.label appear on a single

nit: use "limitCacheSizeBefore" capitalization to be consistent with all the others (and with string names in general)
(Assignee)

Comment 7

7 years ago
Created attachment 477681 [details] [diff] [review]
updated with changes from comment 6

Updated with gavin's changes.

> I don't think you want this change

The code worked (I do check my patches! :), but yes, the variable name implied the inverse.  Much clearer now.
Attachment #477641 - Attachment is obsolete: true
Attachment #477681 - Flags: ui-review+
Attachment #477681 - Flags: review?(gavin.sharp)
Attachment #477641 - Flags: review?(gavin.sharp)
Comment on attachment 477681 [details] [diff] [review]
updated with changes from comment 6

>diff --git a/browser/components/preferences/advanced.xul b/browser/components/preferences/advanced.xul

>               <textbox id="cacheSize" type="number" size="4" max="1024"

>-                       aria-labelledby="useCacheBefore cacheSize useCacheAfter"/>
>+                       aria-labelledby="limitCacheSizeBefore cacheSize limitCacheSizeAfter"/>

aria-labelledby references IDs, not entities, so this change needs to be reverted too.

r=me with that.
Attachment #477681 - Flags: review?(gavin.sharp) → review+
(Assignee)

Comment 9

7 years ago
http://hg.mozilla.org/mozilla-central/rev/0e43f7a9d7a8
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.