444146 - Migrate SeaMonkey's Keyboard Navigation preferences to new pref window

Status: RESOLVED FIXED

(SeaMonkey :: Preferences, defect)

Description

[Manuel Reimer]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.13) Gecko/20080328 SeaMonkey/1.1.9
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.13) Gecko/20080328 SeaMonkey/1.1.9

No need for much text here...

Reproducible: Always

Comment 1

[Manuel Reimer]

Attachment: First patch

Comment 2

[Ian Neal]

Comment on attachment 328530 [details] [diff] [review]
First patch

>Index: suite/common/pref/pref-keynav.js
>===================================================================

> function setLinksOnlyDisabled()
> {
>   try {
>-    document.getElementById('findAsYouTypeAutoWhat').disabled = 
>-      (!document.getElementById('findAsYouTypeEnableAuto').checked ||
>-       parent.hPrefWindow.getPrefIsLocked('accessibility.typeaheadfind.linksonly'));
>+    document.getElementById('findAsYouTypeAutoWhat').disabled =
>+      (!document.getElementById('accessibility.typeaheadfind.autostart').value ||
>+       document.getElementById('accessibility.typeaheadfind.linksonly').locked);
>   }
>   catch(e) {}
> }
Should not need the try/catch anymore.
Once my pref-popups patch lands you should be able to make use of EnableElementById from preferences.js (was previously called EnableTextbox)

>Index: suite/common/pref/pref-keynav.xul
>===================================================================
>+    <!-- gPrefTabNav var and kTabTo* consts are declared in pref-keynav.js -->
>+    <checkbox id="tabNavigationLinks" label="&tabNavigationLinks.label;"
New style is to have one attribute per line in pref files, so same comment for other parts of this xul.

Comment 3

[Manuel Reimer]

Attachment: Second patch

Comment 4

[[e-mail]]

Comment on attachment 328879 [details] [diff] [review]
Second patch

>+    // Textboxes are always part of the tab order
>+    gPrefTabNav.value = gPrefTabNav.value | 1;
I don't think this is a good idea in the new pref world.

> function setLinksOnlyDisabled()
> {
>+    document.getElementById('findAsYouTypeAutoWhat').disabled =
>+      (!document.getElementById('accessibility.typeaheadfind.autostart').value ||
>+       document.getElementById('accessibility.typeaheadfind.linksonly').locked);
>   }
> }
Extra }

>+      <checkbox id="findAsYouTypeEnableAuto"
>+                label="&findAsYouTypeEnableAuto.label;"
>+                preference="accessibility.typeaheadfind.autostart"/>
Hmm, no access key on a control in the keyboard navigation prefs.
How did that happen ;-) Please fix, thanks!

Comment 5

[Manuel Reimer]

(In reply to comment #4)
> (From update of attachment 328879 [details] [diff] [review])
> >+    // Textboxes are always part of the tab order
> >+    gPrefTabNav.value = gPrefTabNav.value | 1;
> I don't think this is a good idea in the new pref world.

I think this is to get sure some bit isn't set by the user via about:config. Sure we can just drop this line?

Comment 6

[Ian Neal]

Comment on attachment 328879 [details] [diff] [review]
Second patch

>Index: suite/common/pref/pref-keynav.js
>===================================================================
>   if (!/Mac/.test(navigator.platform)) {
>+    gPrefTabNav = document.getElementById('accessibility.tabfocus');
>+    // Textboxes are always part of the tab order
>+    gPrefTabNav.value = gPrefTabNav.value | 1;
>     var tabNavigationLinks = document.getElementById('tabNavigationLinks');
>+    tabNavigationLinks.checked = ((gPrefTabNav.value & kTabToLinks) != 0);
>+    tabNavigationLinks.disabled = gPrefTabNav.locked;
>     var tabNavigationForms = document.getElementById('tabNavigationForms');
>+    tabNavigationForms.checked = ((gPrefTabNav.value & kTabToForms) != 0);
>+    tabNavigationForms.disabled = gPrefTabNav.locked;

You should look at using onsynctopreference / onsyncfrompreference for how you relate the checkboxes to the preference then you can put the same preference attribute on both checkboxes and then disabled status will automatically reflect the locked status and you can drop those bits from here.

>   }
>   else
>     document.getElementById('tabNavigationPrefs').setAttribute("hidden", true);
> 
Retrieve the accessibility.typeaheadfind.autostart value here and pass it into the function.
>   setLinksOnlyDisabled();
> }
> 
> function setLinksOnlyDisabled()
> {
If you take an argument of the pref value in this function it will simplify the function
>-  try {
>-    document.getElementById('findAsYouTypeAutoWhat').disabled = 
>-      (!document.getElementById('findAsYouTypeEnableAuto').checked ||
>-       parent.hPrefWindow.getPrefIsLocked('accessibility.typeaheadfind.linksonly'));
>+    document.getElementById('findAsYouTypeAutoWhat').disabled =
>+      (!document.getElementById('accessibility.typeaheadfind.autostart').value ||
>+       document.getElementById('accessibility.typeaheadfind.linksonly').locked);
Use EnableTextbox("findAsYouTypeAutoWhat", aEnable, false) here where aEnable is the pref value you have passed into the function - note name of this function is changing in Bug 441403 to EnableElementById

>Index: suite/common/pref/pref-keynav.xul
>===================================================================
>+<overlay xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
>+  <prefpane id="keynav_pane"
>+            label="&pref.keyNav.title;"
>+            script="chrome://communicator/content/pref/pref-keynav.js">
>+
>+    <preferences id="keynav_preferences">
>+      <preference id="accessibility.typeaheadfind.autostart"
>+                  name="accessibility.typeaheadfind.autostart"
>+                  onchange="setLinksOnlyDisabled();"
Pass this.value within the function.
>+                  type="bool"/>
>+      <preference id="accessibility.typeaheadfind.linksonly"
>+                  name="accessibility.typeaheadfind.linksonly"
>+                  type="bool"/>
>+      <preference id="accessibility.typeaheadfind.enablesound"
>+                  name="accessibility.typeaheadfind.enablesound"
>+                  type="bool"/>
>+      <preference id="accessibility.typeaheadfind.enabletimeout"
>+                  name="accessibility.typeaheadfind.enabletimeout"
>+                  type="bool"/>
>+      <preference id="accessibility.tabfocus"
>+                  name="accessibility.tabfocus"
>+                  type="int"/>
>+    </preferences>
>+
>+    <groupbox id="tabNavigationPrefs"
>+              align="start">
>+      <caption label="&tabNavigationBehavior.label;"/>
>+      <description>&tabNavigationDesc.label;</description>
>+
>+      <!-- gPrefTabNav var and kTabTo* consts are declared in pref-keynav.js-->
>+      <checkbox id="tabNavigationLinks"
>+                label="&tabNavigationLinks.label;"
>+                accesskey="&tabNavigationLinks.accesskey;"
>+                oncommand="gPrefTabNav.value ^= kTabToLinks;"/>
Add preference, onsyncfrompreference and onsynctopreference attributes here and remove the oncommand
>+      <checkbox id="tabNavigationForms"
>+                label="&tabNavigationForms.label;"
>+                accesskey="&tabNavigationForms.accesskey;"
>+                oncommand="gPrefTabNav.value ^= kTabToForms;"/>
Add preference, onsyncfrompreference and onsynctopreference attributes here and remove the oncommand

Note: for onsyncfrompreference and onsyncfrompreference you have to use return document.getElementById('keynav_pane').<function_name> and it will not know about any globals you have set but once in the <function_name> it will know about the globals.

Comment 7

[Manuel Reimer]

Attachment: Third patch

Try to fix all the above stuff.

Comment 8

[[e-mail]]

Comment on attachment 330581 [details] [diff] [review]
Third patch

This looks good to me barring a few nits but I'll give IanN another chance ;-)

>+  return ((pref.value & kTabToLinks) != 0)
Nit: missing semicolon, also I don't think you need the outer brackets.


>+  return ((pref.value & kTabToForms) != 0)
Ditto.

>+      <!-- gPrefTabNav var and kTabTo* consts are declared in pref-keynav.js-->
Obsolete?

>+<!-- Todo: Add Accesskey for "findAsYouTypeEnableAuto" -->
Obsolete?

>+      <checkbox id="findAsYouTypeEnableAuto"
>+                label="&findAsYouTypeEnableAuto.label;"
>+                accesskey="&findAsYouTypeEnableAuto.accesskey;"
Not your fault, but pref-keynav.dtd duplicates the "A" access key :-(

Comment 9

[Ian Neal]

Comment on attachment 330581 [details] [diff] [review]
Third patch

>Index: suite/common/pref/pref-keynav.js
>===================================================================
> function Startup()
>+  if (/Mac/.test(navigator.platform))
>     document.getElementById('tabNavigationPrefs').setAttribute("hidden", true);
Nit: We should try and be consistent about whether we are using ' or ", my preference is the latter but your call.
You can set the global mentioned below in an else to this if statement.

> 
>+function ReadTabNavLinks()
>+{
>+  var pref = document.getElementById('accessibility.tabfocus');
rather than doing this everywhere you could just have a global variable (gPrefTabNav)

>+  return ((pref.value & kTabToLinks) != 0)
As Neil said missing ; and try without 2nd set of brackets.
You could also have a more generic function (ReadTabToPref) and just pass the 2 or 4 as the argument when calling.

>+}
>+function WriteTabNavLinks(field)
> {
>+  var curval = document.getElementById('accessibility.tabfocus').value;
>+  // Textboxes are always part of the tab order
>+  curval |= kTabToTextboxes;
>+  if (field.checked)
>+    return curval | kTabToLinks;
>+  else
>+    return curval & ~kTabToLinks;
>+}
This is probably overcomplicated, again have a more generic function (WriteTabToPref) and just pass the 2 or 4 as the argument when calling. You should just be able to then XOR the bit against the pref value. so no need to touch the 1st bit.

>+
>+function ReadTabNavForms()
>+{
>+  var pref = document.getElementById('accessibility.tabfocus');
>+  return ((pref.value & kTabToForms) != 0)
See above comments.

>+}
>+function WriteTabNavForms(field)
>+{
>+  var curval = document.getElementById('accessibility.tabfocus').value;
>+  // Textboxes are always part of the tab order
>+  curval |= kTabToTextboxes;
>+  if (field.checked)
>+    return curval | kTabToForms;
>+  else
>+    return curval & ~kTabToForms;
See above comments.

>Index: suite/common/pref/pref-keynav.xul
>===================================================================
>+      <!-- gPrefTabNav var and kTabTo* consts are declared in pref-keynav.js-->
As Neil said, now redundant

>+      <checkbox id="tabNavigationLinks"
>+                label="&tabNavigationLinks.label;"
>+                accesskey="&tabNavigationLinks.accesskey;"
>+                preference="accessibility.tabfocus"
>+                onsyncfrompreference="return document.getElementById('keynav_pane').ReadTabNavLinks();"
See comments about js

>+                onsynctopreference="return document.getElementById('keynav_pane').WriteTabNavLinks(this);"/>
See comments about js

>+      <checkbox id="tabNavigationForms"
>+                label="&tabNavigationForms.label;"
>+                accesskey="&tabNavigationForms.accesskey;"
>+                preference="accessibility.tabfocus"
>+                onsyncfrompreference="return document.getElementById('keynav_pane').ReadTabNavForms();"
See comments about js

>+                onsynctopreference="return document.getElementById('keynav_pane').WriteTabNavForms(this);"/>
See comments about js

>+      <description>&tabNavigationTextboxes.label;</description>
>+    </groupbox>
>+
>+    <groupbox align="start">
>+      <caption label="&findAsYouTypeBehavior.label;"/>
>+<!-- Todo: Add Accesskey for "findAsYouTypeEnableAuto" -->
As per Neil's comment

>+      <checkbox id="findAsYouTypeEnableAuto"
>+                label="&findAsYouTypeEnableAuto.label;"
>+                accesskey="&findAsYouTypeEnableAuto.accesskey;"
"F" looks like a good accesskey to me.

r- for the moment

Comment 10

[Manuel Reimer]

Attachment: Just another patch

Comment 11

[Manuel Reimer]

... and another patch.

IMHO it would be silly to just pass a "4" or a "2" to a function. Noone will ever know what those silly numbers mean.

As I'm unable to access my globals, I tried to switch based on the ID of the calling field.

IMHO the whole "bitswapping"-stuff *SUCKS*. Why don't we have separate boolean prefs for this?

Comment 12

[[e-mail]]

(In reply to comment #9)
>(From update of attachment 330581 [details] [diff] [review])
>>+  if (field.checked)
>>+    return curval | kTabToLinks;
>>+  else
>>+    return curval & ~kTabToLinks;
>>+}
>This is probably overcomplicated, again have a more generic function
>(WriteTabToPref) and just pass the 2 or 4 as the argument when calling. You
>should just be able to then XOR the bit against the pref value.
I think you're making an unreasonable assumption about prefwindow behaviour.

Comment 13

[Ian Neal]

Comment on attachment 330638 [details] [diff] [review]
Just another patch

>Index: suite/common/pref/pref-keynav.js
>===================================================================
>+function ReadTabNav(field)
If you are doing it this way you might as well pass the field's id rather than the field. An alternative would be to pass a boolean.
Nit: passed variables should start with an "a".
>+{
>+  var curval = document.getElementById("accessibility.tabfocus").value;
>+  // Return the right bit based on the id of "field"
>+  if (field.id == "tabNavigationLinks")
>+    return (pref.value & kTabToLinks) != 0;
Hmmm, where's pref come from?

>+  else
>+    return (pref.value & kTabToForms) != 0;
Hmmm, where's pref come from? This second return doesn't need to be part of an else statement.

>+}
>+function WriteTabNav(field)
An alternative would be to pass a boolean and the field's checked as arguments.
Nit: passed variables should start with an "a".
> {
>+  var curval = document.getElementById("accessibility.tabfocus").value;
>+  // Textboxes are always part of the tab order
>+  curval |= kTabToTextboxes;
>+  // Toggle the right bit based on the id of "field"
>+  if (field.id == "tabNavigationLinks")
>+    return curval ^ kTabToLinks;
>+  else
>+    return curval ^ kTabToForms;
This second return doesn't need to be part of an else statement. Looks like you should go back to using the checked status to determine whether to use | or & ~ (see comment#12).

Almost there!

Comment 14

[Manuel Reimer]

Attachment: Another patch

Comment 15

[Ian Neal]

Comment on attachment 330771 [details] [diff] [review]
Another patch

>Index: suite/common/pref/pref-keynav.js
>===================================================================
>+function ReadTabNav(aField)
>+{
>+  var curval = document.getElementById("accessibility.tabfocus").value;
>+  // Return the right bit based on the id of "aField"
>+  if (aField.id == "tabNavigationLinks")
>+    return (curval & kTabToLinks) != 0;
>+
>+  return (curval & kTabToForms) != 0;
>+}
Nit: You need a blank line between this function and the next
>+function WriteTabNav(aField)
> {

r=me with above nit sorted

Comment 16

[Justin Wood (:Callek)]

$ hg push
pushing to ssh://hg.mozilla.org/comm-central/
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 1 changesets with 5 changes to 5 files

$ hg tip -v
changeset:   196:303365143db1
tag:         tip
user:        Manuel Reimer <[e-mail]>
date:        Tue Aug 26 08:37:54 2008 -0400
files:       suite/common/pref/pref-keynav.js suite/common/pref/pref-keynav.xul suite/common/pref/preferences.xul suite/common/pref/preftree.xul suite/locales/en-US/chrome/common/pref/pref-keynav.dtd
description:
Bug 444146, Migrate SeaMonkey's Keyboard Navigation preferences to new pref window
r= IanN

Just pushed the fix, with newline nit addressed

Comment 17

[[e-mail]]

Sadly in the month between the patch and the commit, bug 448107 renamed EnableTextbox to EnableElementById, thus causing bug 524886.