621823 - Zoom levels in toolkit.zoomManager.zoomValues are always reset to 0.5,0.75,0.9,1,1.2,1.5,2

Status: VERIFIED FIXED

(SeaMonkey :: Preferences, enhancement)

Description

[Zane U. Ji]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b9pre) Gecko/20101228 Firefox/4.0b9pre SeaMonkey/2.1b2pre
Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b9pre) Gecko/20101228 Firefox/4.0b9pre SeaMonkey/2.1b2pre

When restart SM, the user specified value is changed.

Reproducible: Always

Steps to Reproduce:
1. Open about:config
2. Change toolkit.zoomManager.zoomValues
3. Restart SM and check the value of toolkit.zoomManager.zoomValues
Actual Results:  
The inputted value is not preserved.

Expected Results:  
Shouldn't reset that value.

A change in this December causes this problem.

Comment 1

[Robert Kaiser]

That's intentional in this version of our zoom feature, changing that would probably require deleting all accesskeys for zoom values or finding a complicated locale-aware automatism for them, which is also quite complicated.

I wouldn't see this as a bug, but as an enhancement request to make it actually configurable. I will not work on it, but if anyone can provide a patch that also fixes accesskeys without problems, I'm all for that.

Comment 2

[Tony Mechelynck [:tonymec]]

Setting Importance to RFE according to comment #1. Also some attempt at triageing.

Comment 3

[rsx11m]

It's a property setting that can be changed in a custom-strings.txt override.
See http://forums.mozillazine.org/viewtopic.php?p=10928171#p10928171 for details if you need different zoom steps.

Comment 4

[rsx11m]

So, I was thinking that it might be possible to solve this in a similar way as it is done for the localized mailnews.reply_header_* preferences where the defaults are provided in a .properties definition but can be overridden in the Config Editor. However, Toolkit uses a simple getCharPref() in their backend to read this value whereas a getComplexValue(Ci.nsIPrefLocalizedString) should be needed, so probably just the chrome: URI would be returned as the "value".

> deleting all accesskeys for zoom values or ... locale-aware automatism

I'm not sure about that. Why is it necessary to adjust the View > Zoom menu with the changed set of zoom levels and not just leave the list as it is? Allowing the backend to use different levels for Ctrl+/- while retaining the locale-specific menu levels should be feasible, anything inbetween would just show up as "other", or not? In general, if someone changes a hidden pref he or she better knows what the consequences are, thus I wouldn't bother too much about matching the menu UI with the pref as long as the default is matched.

http://mxr.mozilla.org/l10n-central/search?string=values=&find=viewZoomOverlay.properties shows that no locale thus far has changed any of the zoom levels to something else. Thus, a "cheap" alternative might be to just set an identical toolkit.zoomManager.zoomValues list in pref/defaults and let the user override it in this way. A localization note probably should be added in this case that localizers have to maintain correspondence with the preference default and are supposed to only change the access keys if necessary.

Comment 5

[Robert Kaiser]

Nothing of this is about being able to adapt the zoom levels per locale, that's a stupid idea. All this is about is about having access keys defined for every entry in the menu, and those access keys need to be set in the locale files, and that's what's causing the whole problem in the first place.

And as I said, there might be good solutions if you think about them long enough and carve out the details, I just went with what was the easiest way to keep the menu structure we had while switching to the toolkit backend and supporting per-site remembered zoom levels.

Once again, it's all about the accesskeys, not about the zoom level entries themselves.

Comment 6

[rsx11m]

Attachment: Uncouple pref and menu

Ok, so this seems to be the easiest solution and just replaces copying of the localized string into the preferences introduced by bug 386363 with an explicit definition of the default list in browser-prefs.js mirroring that entry.

In this way, one can change the pref to any sequence of steps, but only those which have corresponding values in the menu will show selected in View > Zoom. Changing the menu list as described in the thread pointed to in comment #3 is still possible, but in turn won't affect the Ctrl+/- zoom levels.

Nothing changes for the user compared to current behavior with the defaults.

Comment 7

[Robert Kaiser]

Comment on attachment 541065 [details] [diff] [review]
Uncouple pref and menu

Have actually _tested_ this with different sets of arbitrarily defined levels? This simply CANNOT work reasonably as the menu not fitting the levels is outside of the measure for usability and the "must match" L10n comment is BS.
I'd favor a solution that either removes the levels from the menu altogether or removes the accesskeys, or does something incredibly intelligent to set the accesskeys without needing L10n settings, but this harms more than it helps.

Comment 8

[rsx11m]

> Have actually _tested_ this with different sets of arbitrarily defined levels?

Yes, the Ctrl +/- keys followed the settings in the pref, and when looking at the menu items in View > Zoom there was simply nothing checked but it worked anyway as expected. Possibly not fully understanding the connections between the menu and the pref, I may have overlooked some other dependency, but didn't see it during the testing of modifying a couple of zoom levels in the pref.

> "must match" L10n comment

That relates to the default list which "must match" obviously to get an initial list which is identical between the SeaMonkey world and the Toolkit world. Again, it's a hidden preference so "know what you are doing" when changing it.

Comment 9

[rsx11m]

Attachment: Create menu from pref

(In reply to comment #7)
> I'd favor a solution that either removes the levels from the menu altogether
> or removes the accesskeys,

This is an alternative patch implementing the second suggestion to remove the access keys (there is still Ctrl+0 for the 100% reset) and to re-align the menu list with the preferences when creating it. The advantage is that there is no longer a duplication of the zoom levels in the localized part (thus we can get rid of the "BS" as so friendly criticized). I don't see an easy way to get the accesskeys back in this way, especially as it would introduce an undesired mix again between pref and localized properties.

Comment 10

[rsx11m]

(making the bug title a bit more search friendly...)

Related to the bug here but essentially independent issues I've filed bug 667525 and bug 667529 on the handling of the View > Zoom > Other menu item.

Comment 11

[rsx11m]

Neil, ping for the reviews?

Comment 12

[rsx11m]

Attachment: Unbitrotted patch

Since bug 667525 checked in first it bitrotted attachment 541143 [details] [diff] [review].
This is the same version with respective updates in viewZoomOverlay.properties.

Given that KaiRo f-'ed it and that we are going into the direction of removing any dependency to the properties with the patch for bug 667525 already, I'm canceling attachment 541065 [details] [diff] [review] as well.

Comment 13

[rsx11m]

BTW: I didn't request f? from KaiRo as the patch follows his comment #7 by implementing the 2nd proposed option, thus assuming that it's ok with him.

Comment 14

[[e-mail]]

Hmm, so this loses the access keys, particular the one for original size...

Comment 15

[rsx11m]

Well, I could retain the 'z' access key for that purpose, but then there is also the Ctrl+0 keyboard shortcut for the original size. After Alt+V and Alt+Z to get into the zoom menu, one can still use the cursor keys to move to the respective zoom level, so in general navigation using the keyboard only isn't lost.

Comment 16

[rsx11m]

Comment on attachment 545414 [details] [diff] [review]
Unbitrotted patch

Adding an accesskey attribute for the "Original" menuItem should do the trick:

>+    if (thisFactor == 100) {
>       label = zoomBundle.getString("labelOriginal");
>+      menuItem.setAttribute("accesskey", zoomBundle.getString("accessKeyOriginal");
>       menuItem.setAttribute("key", "key_zoomReset");
>     }

along with an "accessKeyOriginal=z" definition in viewZoomOverlay.properties.

Comment 17

[rsx11m]

(In reply to comment #16)
>+      menuItem.setAttribute("accesskey", zoomBundle.getString("accessKeyOriginal"));

Oops, copy-paste error when hacking the patch...

Comment 18

[[e-mail]]

I have a cunning plan.

1. Define strings:
zoom.100.label=100% (Original Size)
zoom.100.accesskey=z
zoom.200.label=200% (Double Size)
zoom.200.accesskey=D
2. Set the preference to .5,.67,.8,.9,1,1.1,1.2,1.33,1.5,1.7,2,2.4
3. When building the menu:
* If a zoom string exists, use it and its access key
* Otherwise, try to find an unused digit and use that as the access key
This will result in the following menu:

_5_0%
_6_7%
_8_0%
_9_0%
100% (Original si_z_e)
_1_10%
1_2_0%
1_3_3%
15_0_%
1_7_0%
200% (_D_ouble size)
2_4_0%

Comment 19

[[e-mail]]

Comment on attachment 545414 [details] [diff] [review]
Unbitrotted patch

>+  var zoomFactors = Services.prefs
>+                            .getCharPref("toolkit.zoomManager.zoomValues")
>+                            .split(",");
On a side note, this should use ZoomManager.zoomValues

Comment 20

[rsx11m]

(In reply to comment #18)
> * If a zoom string exists, use it and its access key
> * Otherwise, try to find an unused digit and use that as the access key
> This will result in the following menu:

So, catching the 200% case would work just the same as the 100% case, no problem. Finding access keys in this way assures that the defaults are all assigned, they may change for the same value if the user modifies the pref, but that sure looks like a feasible approach. I'll need at least another loop and some array to keep track of assigned keys (or search by attribute), I'll give it a try over the next few days.

Comment 21

[rsx11m]

Needs some more testing, but the following loop implements comment #18 while keeping track of assigned accessKeys in a "usedKeys" string:

>   for (var j = 0; j < label.length && accessKey.length == 0; ++j) {
>     var testKey = label[j];
>     if (testKey>='0' && testKey<='9' && usedKeys.indexOf(testKey)<0) {
>       accessKey = testKey;
>       usedKeys += testKey;
>     }
>   }

Comment 22

[rsx11m]

Attachment: Assign accesskeys dynamically

This was actually much less painful than I initially thought, so here now the implementation of Neil's idea in line with KaiRo's 3rd proposed solution type.

Accesskeys are defined for 100% and 200% only along with the labels (and I'm not sure if you wrote the new properties like that on purpose, but I also removed the redundant "%zoom%" parts in those as they are always 100 or 200 by definition).

For each zoom value, the accesskeys are determined per comment #21, and only based on the numbers of that label. I was thinking if I could terminate the loop already when reaching the first non-numerical character, but that would not work with RTL locales or other regions putting something before the number.

I've also addressed your comment #19 to get the array of zoom levels directly.

Comment 23

[[e-mail]]

Comment on attachment 545698 [details] [diff] [review]
Assign accesskeys dynamically

Thanks for getting to this so quickly!

>+      for (var j = 0; j < label.length && accessKey.length == 0; ++j) {
Don't need to check accessKey, just break out of the loop.

>+        if (testKey>='0' && testKey<='9' && usedKeys.indexOf(testKey)<0) {
Nit: spaces around operators.
If we reverse the process and keep track of available keys this avoids having to test that the access key is a digit. There are at least three options:
1. var freeKeys = "0123456789";
   Delete characters as you use them, presumably using replace().
2. var freeKeys = { 0: true, 1: true, ... 9: true };
   Probably use hasOwnProperty to check and delete to remove a key.
3. var freeKeys = [ true, true, ... true ];
   Just set the values to false as you use them.

>+    if (accessKey.length > 0)
Nit: if (accessKey) suffices.

>+# the "Other" accesskey is defined in viewZoomOverlay.dtd
FYI the preferred L10N note format is something like this:
# LOCALIZATION NOTE (labelOther): The access key is in viewZoomOverlay.dtd

Comment 24

[rsx11m]

(In reply to comment #23)
> 1. var freeKeys = "0123456789";
>    Delete characters as you use them, presumably using replace().

This would be closest to the current string-based version, but unfortunately just using freeKeys.replace(testKey, ""); didn't work for me and the index is searched for twice anyway in this variant.

> 2. var freeKeys = { 0: true, 1: true, ... 9: true };
>    Probably use hasOwnProperty to check and delete to remove a key.

Looks interesting but somewhat intimidating, so not quite my favorite. ;-)

> 3. var freeKeys = [ true, true, ... true ];
>    Just set the values to false as you use them.

That version would still require boundary checks against '0' and '9' to avoid using an index outside that range. However, combined with variant #1 I get:

>   var freeKeys = [ '0', '1', '2', '3', '4', '5', '6', '7', '8', '9' ];
>   for (var j = 0; j < label.length; ++j) {
>     var testKey = label[j];
>     var indexKey = freeKeys.indexOf(testKey);
>     if (indexKey >= 0) {
>       accessKey = testKey;
>       freeKeys.splice(indexKey, 1);
>       break;
>     }
>   } 

This creates an array of available keys, and when a matching key is found it is spliced off the array until it's empty. This doesn't need any boundary checks but I need to keep track of the array index in order to remove it later.

> # LOCALIZATION NOTE (labelOther): The access key is in viewZoomOverlay.dtd

I'll add/modify those where necessary, there are more occurrences of l10n notes which aren't labeled in this way. New patch coming up tomorrow.

Comment 25

[rsx11m]

(In reply to comment #24)
> That version would still require boundary checks against '0' and '9' to
> avoid using an index outside that range.

Interesting, apparently I was thinking too much in C++ terms with char-to-int conversion. This works in JS as array['x'] = undefined and treated like false:

>   var freeKeys = [ true, true, true, true, true, true, true, true, true, true ];
>   for (var j = 0; j < label.length; ++j) {
>     var testKey = label[j];
>     if (freeKeys[testKey]) {
>       accessKey = testKey;
>       freeKeys[testKey] = false;
>       break;
>     }
>   } 

So, that would be the shortest variant, but is less intuitive than comment #24.

Comment 26

[[e-mail]]

(In reply to comment #24)
> (In reply to comment #23)
> > 1. var freeKeys = "0123456789";
> >    Delete characters as you use them, presumably using replace().
> This would be closest to the current string-based version, but unfortunately
> just using freeKeys.replace(testKey, ""); didn't work for me
freeKeys = freeKeys.replace(testKey, "");

Comment 27

[[e-mail]]

(In reply to comment #25)
> that would be the shortest variant, but is less intuitive than comment #24.
I tried to think of as many variants as I could just so that you had the flexibility to decide which was the most intuitive :-)

Comment 28

[rsx11m]

Attachment: Use array for free accesskeys

Thanks for the hints, and I figured that I had screwed up the string assignment when implementing option #1 later myself. I ended up with comment #24 being the most explicit, thus least "hackish" and easiest to read version.

BTW: As a side effect of comment #19, any zoom levels outside the min/max range specified in the zoom.*Percent prefs aren't filled and also won't get an access key assigned.

As a drive-by fix, all l10n notes are made proper LOCALIZATION NOTEs in both viewZoomOverlay.properties and viewZoomOverlay.dtd files. Two accesskeys were lower case there despite the label being upper case, corrected without any changes in entity names to maintain consistency.

Comment 29

[[e-mail]]

(In reply to comment #28)
> BTW: As a side effect of comment #19, any zoom levels outside the min/max
> range specified in the zoom.*Percent prefs aren't filled and also won't get
> an access key assigned.
I don't think they would have worked anyway, so that's an extra fix :-)

Comment 30

[rsx11m]

Yes, the current implementation doesn't have any visible effect when one of those "forbidden" values is used from the menu, but toolkit throws an exception, thus it's certainly beneficial to get this fixed in the process.

Comment 31

[Robert Kaiser]

Comment on attachment 545897 [details] [diff] [review]
Use array for free accesskeys

>+// Zoom levels for View > Zoom and Ctrl +/- keyboard shortcuts
>+pref("toolkit.zoomManager.zoomValues", "0.5,0.67,0.8,0.9,1,1.1,1.2,1.33,1.5,1.7,2,2.4");

Do we need that at all? Is all.js not setting that already?

>+<!-- LOCALIZATION NOTE (zoomMenu,zoomOtherCmd) labels in viewZoomOverlay.properties -->
>+<!ENTITY zoomMenu.accesskey         "Z">
>+<!ENTITY zoomOtherCmd.accesskey     "O">

If possible, please move those to be in the .properties as well and set along the same lines. It's easier for localizers to have the connected entries in the same files.

Still, thanks about the L10n notes and the caring about actually having accesskeys - I haven't looked at all code in detail as I'm short on time this week, but the approach you take is very much appreciated and sure better than the workaround I hacked in for the current version!

Comment 32

[rsx11m]

(In reply to comment #31)
> Do we need that at all? Is all.js not setting that already?

This is actually a subset of what's defined in all.js, which defines a version with additional values that wouldn't allow to assign accesskeys to all of them.

> If possible, please move those to be in the .properties as well

I left the zoomMenu and zoomOtherCmd accesskeys where they are as they are indeed a constant and set in the XUL code as entities. Moving them to properties would require to remove them there and to set them as attributes in the JS parts, parallel to setting the label in updateViewMenu() and updateZoomMenu(). To that extent, it's possible but appears redundant from the code perspective, thus it's a trade off. Neil's call.

Comment 33

[rsx11m]

(also keep in mind that these are entities which aren't new and have been there before, thus the localizers have already had to cope with that cross reference,  and moving them now would imply they have to work on those again...)

Comment 34

[rsx11m]

Attachment: Alternative patch

Following KaiRo's comment #31, this is a version of attachment 545897 [details] [diff] [review] which moves the accesskeys to their corresponding labels in the properties. As an added bonus, this now allows different accesskeys for fullZoom vs. textZoom. :-)

I've also made the property names more consistent. So, while in the short run localizers may have to double-check their assignments and match them with the new property names, on the long run this version is definitely more l10n-friendly.

Neil, please r+ one and r- the other of these patches, whatever you decide.

Comment 35

[[e-mail]]

Comment on attachment 545897 [details] [diff] [review]
Use array for free accesskeys

Seeing as you were so kind as to address KaiRo's comments, marking obsolete.

Comment 36

[[e-mail]]

Comment on attachment 546040 [details] [diff] [review]
Alternative patch

> 
>-<!-- see zoomOtherLabel in viewZoomOverlay.properties -->
>-<!ENTITY zoomMenu.accesskey         "z">
>-<!ENTITY zoomOtherCmd.accesskey     "o">
[Could have possibly removed the blank line too]

>+zoom.value.label=%zoom% %
> 
>+# labels and accesskeys to emphasize the 100 % and 200 % entries
> 
>+zoom.100.label=100 % (Original Size)
>+zoom.100.accesskey=z
>+zoom.200.label=200 % (Double Size)
>+zoom.200.accesskey=D
>+
>+other.label=Other (%zoom% %) â¦
>+other.accesskey=O
Should be zoom.other.* throughout surely? r=me with that fixed.

I considered bikeshedding the order of the entries to match code order...

Comment 37

[rsx11m]

Attachment: Patch for checkin

(In reply to comment #36)
> [Could have possibly removed the blank line too]
> Should be zoom.other.* throughout surely? r=me with that fixed.

Both done.

> I considered bikeshedding the order of the entries to match code order...

I've left the zoom-type properties on top as they are labels for the View menu, but moved zoom.value.label underneath the zoom.{100,200}.* properties and before the zoom.other.* entries, thus matching now the order. Also added back the comment that accesskeys for those are assigned dynamically to avoid confusion.

r=Neil from attachment 546040 [details] [diff] [review].

Comment 38

[rsx11m]

Push for trunk, please.

Comment 39

[Philip Chee]

Pushed.
http://hg.mozilla.org/comm-central/rev/a534791838e8

Comment 40

[Justin Wood (:Callek)]

Wow, happy to see this fixed! Thank You!

Comment 41

[rsx11m]

My thanks sure go back to Neil and KaiRo for their feedback and suggestions!

As another minor spin-off (and the last one from my side, I promise), bug 672146 follows up on comment #30 and fixes a couple of other cases where toolkit's zoom manager may receive invalid arguments and thus throws exceptions.

Comment 42

[rsx11m]

I've seen that Callek already set the "relnote" keyword, otherwise I would have set it based on the following observation when migrating a profile from 2.4a2 to 2.5a2:

Given that a user_pref is set for toolkit.zoomManager.zoomValues already the first time SM 2.5+ is invoked (coming from running 2.[1-4] before), the preference and menu will stick with 0.5,0.75,0.9,1,1.2,1.5,2 rather than adopting the new menu defaults from the browser-prefs.js as defined in the patch.

Possible wording: "SeaMonkey 2.5 defines new zoom stepping defaults in View > Zoom and makes them configurable with a hidden toolkit.zoomManager.zoomValues preference (bug 621823). Profiles migrated from earlier versions retain the old stepping. Look up the preference in about:config and reset it from the right-click menu to switch to the new default stepping for the page zoom."

Comment 43

[Jens Hatlak (:InvisibleSmiley)]

Relnoted (cf. bug 688953 comment 5).

Comment 44

[Tony Mechelynck [:tonymec]]

Mozilla/5.0 (X11; Linux x86_64; rv:12.0a1) Gecko/20111223 Firefox/12.0a1 SeaMonkey/2.9a1 ID:20111223003001

Some time ago I have set my zoom values to "0.33,0.4,0.5,0.6,0.67,0.75,0.8,0.9,1,1.1,1.25,1.33,1.5,1.7,2,2.5,3" and it wasn't reset on startup, nor even when upgrading 2.8a1 → 2.9a1. No ill effects AFAICT.

=> VERIFIED.

Comment 45

[Tony Mechelynck [:tonymec]]

P.S. …also, the zoom menu holds "my" values, with accesskeys where possible.