444801 - Implement pref support on snav

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Antonio Gomes (tonikitoo)]

Now that bug 436084 has landed, snav needs to add support to its own preferences:

snav.enabled
snav.ignoreTextFields
snav.disableJS
snav.rectFudge
snav.directionBias
snav.keyCode.left
snav.keyCode.right
snav.keyCode.up
snav.keyCode.down
snav.keyCode.modifier

as in old C++ based implementation.

Comment 1

[Antonio Gomes (tonikitoo)]

Attachment: is that the way to go ?

Patch v0 adds 'snav.enabled' support. off course there are a bunch of other prefs to be set, I just wondering if a global 'gPrefenreces' obj to handle them is the way to go.

$cd mozilla-central
$patch -p1 < ../BUG444801_v0.diff

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

The PrefObserver seems like a nice place to hang getters for the snav preferences. You could make the prefService a class member, so it can be reused. You could also make PrefObserver a literal object instead of something that needs to be new'ed.

Comment 3

[Antonio Gomes (tonikitoo)]

Attachment: is that the way to go ? (v2)

addressed mark's comment. what do you think ? can I move for further prefs that way ?

Comment 4

[Doug Turner (:dougt)]

this is the way to go. change the name from PrefObserver to Prefs or SnavPrefs.

Comment 5

[Antonio Gomes (tonikitoo)]

Attachment: WIP preference support for snav (v2)

so, this actually does more than add prefs support in snav:

1) added the following prefs support: 
snav.enabled
snav.ignoreTextFields
snav.rectFudge (fixed value)
snav.directionBias (fixed value)
snav.keyCode.left
snav.keyCode.right
snav.keyCode.up
snav.keyCode.down
snav.keyCode.modifier

2) use "two space" based identation.
3) use the style:

* if (..) {

instead of

* if (..)
{

4) removed some dump code.

note: snav.disableJS is still missing.

dougt ?

Comment 6

[Doug Turner (:dougt)]

Comment on attachment 331689 [details] [diff] [review]
WIP preference support for snav (v2)

the spacing is weird in this patch.  in some places it is 2 and others it is 4.

why is this QI needed:
+  event.QueryInterface(Ci.nsIDOMKeyEvent);

why do you need to save the modifierKey


You can clean up the modifierKey stuff, something like:

var modkey = PrefObserver['keyCodeModifier'];

if (modkey & 0x00100000 && !event.shiftKey)
  return;

I do not think we need to support |ignoreTextFields|.

remove these lines:
+    // user_pref("snav.keyCode.modifier", 1048594);
+    // user_pref("snav.keyCode.modifier", 0x00000012 | 0x00100000);


Don't do stuff like:
+    try {
+      this.enabled = this._branch.getBoolPref("enabled");
+    } catch (e){
+      this.enabled = false;
+    }


Instead, just call 'observe' with the aTopic of "nsPref:changed" and the aData of what you are interested in.

Comment 7

[Antonio Gomes (tonikitoo)]

Attachment: WIP preference support for snav (v3)

addressed dougt's comments

Comment 8

[Antonio Gomes (tonikitoo)]

(In reply to comment #6)
> (From update of attachment 331689 [details] [diff] [review])
> the spacing is weird in this patch.  in some places it is 2 and others it is 4.

odd, I tried to use 2 throughout the code. 

> why is this QI needed:
> +  event.QueryInterface(Ci.nsIDOMKeyEvent);

not needed

> why do you need to save the modifierKey
> You can clean up the modifierKey stuff, something like:
> var modkey = PrefObserver['keyCodeModifier'];
> if (modkey & 0x00100000 && !event.shiftKey)
>   return;

done

> I do not think we need to support |ignoreTextFields|.

k

> remove these lines:
> +    // user_pref("snav.keyCode.modifier", 1048594);
> +    // user_pref("snav.keyCode.modifier", 0x00000012 | 0x00100000);

k

> Don't do stuff like:
> +    try {
> +      this.enabled = this._branch.getBoolPref("enabled");
> +    } catch (e){
> +      this.enabled = false;
> +    }
> Instead, just call 'observe' with the aTopic of "nsPref:changed" and the aData
> of what you are interested in.

done.

so, what about:

snav.directionalBias;
snav.rectFudge;

should they be settable?

we are only missing snav.disableJS. Investigating how to do this ...

Comment 9

[Antonio Gomes (tonikitoo)]

after talking to doug, implemented snav prefs that interest to us, at this level:

snav.enabled
snav.keyCode.left
snav.keyCode.right
snav.keyCode.up
snav.keyCode.down
snav.keyCode.modifier

mobile devices will usually set 'snav.keyCode.modifier' to 0, and use d-pad for directional, than keyCode.(left|right|up|down) can use default values. for celphones or any not-so-keyboard-friend device, they might have to map directions to their keys ...

Comment 10

[Antonio Gomes (tonikitoo)]

Attachment: basic pref support to snav.

patch ready for review.

Comment 11

[Doug Turner (:dougt)]

maybe we should just make keyCodeModifier a string?  And have valid values include "shift", "alt", "ctrl".

you can get away with only calling "this._branch.addObserver" once if you pass a first argument of snav., right?

do we need to unregister anything?

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

add "accel" like the <key> modifiers?

Comment 13

[Antonio Gomes (tonikitoo)]

Attachment: addressed doug't comments.

> maybe we should just make keyCodeModifier a string?  And have valid values
> include "shift", "alt", "ctrl".

done. '+' works as a the separator, and any value different from "shift", "alt", "ctrl" sets snav.modifier to "none" , previously '0'.

> you can get away with only calling "this._branch.addObserver" once if you pass
> a first argument of snav., right?

we can not , actually. it is needed for the observer to work.

> do we need to unregister anything?

hum, we could do it add unregister in PrefObserver and call it from SpatialNavigation.uninit ? not sure if it is needed ...

Comment 14

[Doug Turner (:dougt)]

Comment on attachment 335970 [details] [diff] [review]
addressed doug't comments.

if the preference for the modifier is not set, but the user has the ALT key held down as they navigate, we aren't going to prevent them?

looks good, but we should handle that case.

Comment 15

[Antonio Gomes (tonikitoo)]

doug, I thought it was desired.

use case: user does not set any modifier ("none"), and presses LEFT arrow holding ALT. Snav should not work, once it is pressing a valid modifier that is not set as snav modifier. So, the default action is called: Back()...
make sense ?

Comment 16

[Antonio Gomes (tonikitoo)]

Attachment: basic pref support to snav (v2)

Fix dougt's point in comment #14.

Added two mochitests for "snav.enabled" and "snav.keyCode.up|down|right|left" prefs. 

probably SpatialNavUtils.js has to be modified to support modifiers (alt, crtl) tests ?

Comment 17

[Antonio Gomes (tonikitoo)]

Attachment: basic pref support to snav (v3)

highlights:

* adds pref support to snav.
* adds two mochitest (test_snav_prefDisabled.xul and test_snav_prefKeyCode.xul  ). The former tests just if snav.enabled works fine and the later tests "keyCode.right", "keyCode.left", "keyCode.down" and "keyCode.up".
* Some changes in SpatialNavUtils.js were done: methods prepareTest and completeTest make sure all prefs have the right value before and after each test.
* desired prefs have to be in each mochitest file.

Comment 19

[Doug Turner (:dougt)]

this looks fine.  Clint, is there a better way to sent prefs in a mochitest?

Comment 20

[Doug Turner (:dougt)]

Comment on attachment 340795 [details] [diff] [review]
basic pref support to snav (v3)

this is fine.  one more review.

Comment 21

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 340795 [details] [diff] [review]
basic pref support to snav (v3)

>diff -r 3f7ffb18fd55 toolkit/spatial-navigation/SpatialNavigation.js

>-      if (event.keyCode == event.DOM_VK_RIGHT || event.keyCode == event.DOM_VK_DOWN  ) {
>+        if (event.keyCode == PrefObserver['keyCodeRight'] ||
>+            event.keyCode == PrefObserver['keyCodeDown'] ) {
>         // we are moving forward into the document
>         if (target.textLength != target.selectionEnd)
>           return;

will this leave the wrong indent?

Comment 22

[Antonio Gomes (tonikitoo)]

Attachment: basic pref support to snav (final patch)

fixed indentation.

Comment 23

[Antonio Gomes (tonikitoo)]

checkin-needed added.

follow-up for fennec : bug 458508

Comment 24

[Antonio Gomes (tonikitoo)]

Attachment: basic pref support to snav (final patch - 2) [Checkin: Comment 27]

> if there is no modifier than it default to nothing (from bug 458508 #2).

done.

-        } catch(e) {
-            this.keyCodeModifier = kAlt+separator+kShift;
-        }
+        } catch(e) {
+            this.keyCodeModifier = kNone;
+        }

Comment 25

[cmtalbert]

(In reply to comment #19)
> this looks fine.  Clint, is there a better way to sent prefs in a mochitest?

Sorry for missing this comment.  I looked over the "final patch -2" and it looks good to me.  I don't know of any other way to do it.

Comment 26

[Antonio Gomes (tonikitoo)]

checkin-needed added, final patch - 2 is the last one, it addresses some small dougt and mfinkle requests

thanks

Comment 27

[Serge Gautherie (:sgautherie)]

Comment on attachment 341793 [details] [diff] [review]
basic pref support to snav (final patch - 2)
[Checkin: Comment 27]

http://hg.mozilla.org/mozilla-central/rev/733d4a419520