Doug Turner (:dougt) Assignee
	Comment 1 • 17 years ago

this migrates snav from mozilla cvs to hg, massages the build system to build it into libxul.

next step is to clean up the patch, migrate a few bug fixes, and ask for a full review for the feature.

	Doug Turner (:dougt) Assignee
	Comment 3 • 17 years ago

adding bugs that I would like to fix with this patch.

	Alex Vincent [:WeirdAl]
	Comment 4 • 17 years ago

dougt: can you for convenience's sake give us a diff against existing code in the 1.9 code base?  It's hard to tell just what's changed, since these are all new files for hg.

	Doug Turner (:dougt) Assignee
	Comment 5 • 17 years ago

see comment #1, I am intending his to go through a full review and intend to ignore that it was ever in mozilla cvs.  For what you are doing, you easily can do a hg pull, apply the diff, then compare your hg tree to mozilla using diff (or if you have a real editor, you could M-x ediff-directories)

	Doug Turner (:dougt) Assignee
	Comment 6 • 17 years ago

okay, after talking with roc a bit, we might have everything needed in javascript and his suggestion is to do this in chrome js.  I am going to give that a shot before continuing on this native path.....

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 7 • 17 years ago

JS might not be fast enough, but if it's not fast enough maybe we can figure out some simple stuff that can be done fast in C++ and provided via DOM API, and the rest can be JS.

	Doug Turner (:dougt) Assignee
	Comment 8 • 17 years ago

"only one way to find out" :-)

	Christian Sejersen Reporter
	Comment 9 • 17 years ago

If we put this in chrome, will it then work in the embedding case?

	Doug Turner (:dougt) Assignee
	Comment 10 • 17 years ago

it will be a js component.  it will not be in fennec specific chrome.  Embedders will be able to use it.  we will just need to figure out where something like this would live in our source tree.

	Oleg Romashin (:romaxa)
	Comment 11 • 17 years ago

I think roc approach with C++ and DOM API is much better...

Code presented in "patch v.1" very specific, and cannot solve all problems for all customers of this feature...

I guess "patch v.1" is not including bunch of fixes and updates from here:
https://garage.maemo.org/svn/browser/mozilla/tags/MICROB_2007_BRANCH/microb-engine/microb-engine/1.0.4-72/debian/patches/snav/

There are a lot of changes/improvements and ~6 month of work just to have what we have in Diablo release.

Also from my point of view, spatial navigation and focusing need to be done without real focusing (only some canvas frame for example), and works like hovering mode (see example on N800 first release with opera).

Fix me if I'm wrong somewhere...

	Antonio Gomes (tonikitoo)
	Comment 12 • 17 years ago

> I guess "patch v.1" is not including bunch of fixes and updates from here:
> https://garage.maemo.org/svn/browser/mozilla/tags/MICROB_2007_BRANCH/microb-engine/microb-engine/1.0.4-72/debian/patches/snav

v2 is just to get it built/working w/ fennec ... microb changes are planned to get added into it soon 

	Antonio Gomes (tonikitoo)
	Comment 13 • 17 years ago

err ... I meant *v1*

	Oleg Romashin (:romaxa)
	Comment 14 • 17 years ago

> v2 is just to get it built/working w/ fennec ... microb changes are planned to
> get added into it soon 
> 
I know  that changes not very bad, but they are a bit specific and implemented according to our specification...

I think nsIFrameTraversal API or similar and better need to be exposed for "end-developer" ;).
After that we have to implement something based on that interface...
(It can be implementation based on real focus, or fake focus or something else)

	Doug Turner (:dougt) Assignee
	Comment 15 • 17 years ago

Javascript version of snav.  15% of the size of the existing naive approach.  Performance is not bad.  Still need to test on the device.

Todo:
  Some cases aren't supported (area maps, selects, iframes, framesets).
  Configurable key bindings.
  Performance improvements.
  Disabling when in textfields.

To use, just copy this file into your componens directory, remove compreg.dat in your profile directory, and start firefox.

	Oleg Romashin (:romaxa)
	Comment 16 • 17 years ago

Some problem with snav logic...
The same problem was in previous snav... and it fixed in diablo release...
see testcase in attachment.

Try it, behavior very strange ;)

	Doug Turner (:dougt) Assignee
	Comment 17 • 17 years ago

yeah, i see it too w/ the js version.  We should fix it.  Where in:

https://garage.maemo.org/svn/browser/mozilla/tags/MICROB_2007_BRANCH/microb-engine/microb-engine/1.0.4-72/debian/patches/snav/

Do you fix this issue?

	Oleg Romashin (:romaxa)
	Comment 18 • 17 years ago

Yep, that problem was fixed by Pete Collins:
Some where here:
https://garage.maemo.org/svn/browser/mozilla/branches/MICROB_2007_BRANCH/microb-engine/microb-engine/debian/patches/snav/2008-02-08-SNAV.patch

	Doug Turner (:dougt) Assignee
	Comment 19 • 17 years ago

romaxa: if you adjust gRectFudge to zero, your testcase works.

	Oleg Romashin (:romaxa)
	Comment 20 • 17 years ago

yahoo.com: "Today's Top Searches" box - that was original testcase...
try press right from "Euro 2008" link and get "Carrot Cake Recipe"

I can fill new bug about it if it required.

There are many cases and if it required I or our tester can check and move some of them in mozilla bugzilla.

	Doug Turner (:dougt) Assignee
	Comment 21 • 17 years ago

I really don't want this to be the bug that has a zillion edge cases in it.  Lets file separate bugs for each case, but lets do this after we get something landed.  In the mean time, if you have a list, please email it to me and I can work on the issues.

	Oleg Romashin (:romaxa)
	Comment 22 • 17 years ago

Ok, I will try to share list of already known issues after landing initial version.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 23 • 17 years ago

Comment on attachment 326929 [details]
javascript version v.1

> * The Initial Developer of the Original Code is
> * Doug Turner <dougt@meer.net>

Shouldn't this actually be MoCo, with you listed in contributors as "original author"?

>var kReallyBigNumber = 9999999999;

You can just use the "Infinity" keyword.

>    _onInputKeyPress: function(event) {

>	function snavfilter(node) {
>	    switch (node.nodeName.toLowerCase()) {

Should use instanceof checks instead of relying on nodeName (e.g. HTMLInputElement, (HTMLAnchorElement && elem.href), etc. Could steal stuff from nsContextMenu.js).

>	var bestElementToFocus = null;
>	var distanceToBestElement = kReallyBigNumber;
>
>	var focusedRect = inflateRect(event.target.getBoundingClientRect(),
>				      - gRectFudge);
>
>	var doc = event.target.ownerDocument;
>
>	var treeWalker = doc.createTreeWalker(doc, Ci.nsIDOMNodeFilter.SHOW_ELEMENT, snavfilter, false);
>	var nextNode = treeWalker.nextNode();
>
>	while (nextNode) {

I'd use while ((nextNode = treeWalker.nextNode()));
(could also add (&& nextNode != event.target) to avoid calculating the distance of the element from itself)

>	    var distance = distanceBetweenRectWithDirectionalBias(focusedRect, nextRect, event);

>	    if (distance <= distanceToBestElement && distance > 0) {

<= means we'll prefer the last found element in document order, assuming there are multiple elements the same distance away... is this intentional? Guess it probably doesn't matter.

>	if (bestElementToFocus != null) {
>	    dump("focusing element  " + bestElementToFocus.nodeName);

>	} else {
>	    // couldn't find anything.  just advance and hope.
>	    dump("advancing focus");

Shame there isn't a regressFocus() :)

Probably want to remove the dump()s before landing.

>function inflateRect(rect, value)
>{
>    var newRect = new Object();
>    newRect.left   = rect.left - value;
>    newRect.top    = rect.top - value;
>    newRect.right  = rect.right  + value;
>    newRect.bottom = rect.bottom + value;
>    return newRect;

style nit: I'd write this as:
return {
  left  : rect.left - value,
  top   : rect.top - value,
  right : rect.right  + value,
  bottom: rect.bottom + value,
};

>function Intersects(a, b)

This is really "Contains", right? Might be clearer to use a rect() object that has these methods on it's prototype.

>function spatialDistance(event, a, b)

>    if (event.keyCode == event.DOM_VK_LEFT) {
...
>    } else if (event.keyCode == event.DOM_VK_RIGHT) {
...
>    } else if (event.keyCode == event.DOM_VK_UP) {
...
>    } else if (event.keyCode == event.DOM_VK_DOWN) {
...
>    }

You can simplify all of this down to just calculating the delta between coordinates, right? You're only concerned about absolute distance.

>    var scopedRect = inflateRect(a, gRectFudge);

>    if (event.keyCode == event.DOM_VK_LEFT) {
>    else if (event.keyCode == event.DOM_VK_RIGHT) {
>    else if (event.keyCode == event.DOM_VK_UP) {
>    else if (event.keyCode == event.DOM_VK_DOWN) {

This doesn't look quite right... shouldn't each of these branches just extend their associated property on scopedRect (i.e. VK_LEFT sets left = 0, VK_DOWN sets bottom = Infinity, etc.)? Though really you could just extend .left/.right for both LEFT and RIGHT and .top/.bottom for both UP and DOWN, since isInDirection has already ensured that the element is in the right direction.

>    var d = ((mx-nx)*(mx-nx)) + ((my-ny)*(my-ny));

Math.pow(deltax, 2) + Math.pow(deltay, 2).

>    if (d<0)
>	d=d*(-1);

Math.abs(d), but d is a sum of squares so can't be negative.

>    if (inlineNavigation)
>	d /= gDirectionalBias;

Could use a comment... "prefer elements directly aligned with the focused element" or something like that. Might also want to add a comment that this function returns the distance squared, not the actual distance.

>function distanceBetweenRectWithDirectionalBias(recta, rectb, event) {
>
>    if (! isRectInDirection(event, recta, rectb))
>	return 0;
>
>    return spatialDistance(event, recta, rectb);

Combining these two checks and relying on a magic "0" value for "is not in that direction" is kinda confusing... would be best to just inline this function in _onInputKeyPress I think.

	Doug Turner (:dougt) Assignee
	Comment 24 • 17 years ago

Comment on attachment 326929 [details]
javascript version v.1

> Should use instanceof checks instead of relying on nodeName (e.g.
HTMLInputElement, (HTMLAnchorElement && elem.href), etc. Could steal stuff from nsContextMenu.js).

Take a look at other filers, they do what I am doing.

> (could also add (&& nextNode != event.target) to avoid calculating the distance
of the element from itself)

That would break the while loop, and we want to continue for all elements that pass our filter.  I suppose I can do this check inside the loop.

> prefer the last found element in document order

Yes.

> Probably want to remove the dump()s before landing. 

I will comment them out, for sure.

> This is really "Contains", right? Might be clearer to use a rect() object that
has these methods on it's prototype.

Well, i was hoping that the rect objects return from getBoundingClientRect would be more than they are (maybe having method on it).  I could create a new object that has inflateRect() and Intersects().  Do you think that is required -- i am not using these functions much.

> You can simplify all of this down to just calculating the delta between
coordinates, right? You're only concerned about absolute distance.

No.  The shortest distance algorithm does no work as well as the one I implemented.  If you would like to see the difference, i coded up what you are talking about and it is pretty bad -- it works, but it isn't what you would "expect" to have happen.

> This doesn't look quite right

I am trying to figure out if the next element is directly inline with the currently focused element.  There are not too many ways to do this, and your approach is clearer.  

> Combining these two checks 

I left them separate so that you could change out the spatialDistance function easier (which I did -- I have a use the center-of-the-rect implementation).  i would be happy to put everything in distanceBeweenRect*.  But I do not think putting stuff in _onInputKeyPress is a good idea.

New patch coming up.

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 26 • 17 years ago

(In reply to comment #24)
> (From update of attachment 326929 [details])
> > Should use instanceof checks instead of relying on nodeName (e.g.
> HTMLInputElement, (HTMLAnchorElement && elem.href), etc. Could steal stuff from
> nsContextMenu.js).
> 
> Take a look at other filers, they do what I am doing.

I agree with gavin. What other filers?

	Doug Turner (:dougt) Assignee
	Comment 27 • 17 years ago

filters:
http://mxr.mozilla.org/mozilla-central/source/editor/ui/composer/content/StructBarContextMenu.js#75


Okay, i will change that too. 

	Doug Turner (:dougt) Assignee
	Comment 28 • 17 years ago

more comments from mfinkle and gavin.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 29 • 17 years ago

Comment on attachment 327007 [details]
javascript version v.3

>    _onInputKeyPress: function(event) {

>	while ((nextNode = treeWalker.nextNode())) {

>	    if (distance <= distanceToBestElement && distance > 0) {

" && distance > 0" is unneeded now that isRectInDirection is separate, right?

>function spatialDistance(event, a, b)

>    if (event.keyCode == event.DOM_VK_LEFT) {
>    } else if (event.keyCode == event.DOM_VK_RIGHT) {
>    } else if (event.keyCode == event.DOM_VK_UP) {
>    } else if (event.keyCode == event.DOM_VK_DOWN) {

could still collapse some of these cases, but we can do that later I guess.

	Doug Turner (:dougt) Assignee
	Comment 30 • 17 years ago

with absolute positioning, elements can overlap.  that is what the distance == 0 thing is about.

	Doug Turner (:dougt) Assignee
	Comment 31 • 17 years ago

incorporates further feedback.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 32 • 17 years ago

(In reply to comment #30)
> with absolute positioning, elements can overlap.  that is what the distance ==
> 0 thing is about.

Right, but isInDirection will return false then, right?

	Doug Turner (:dougt) Assignee
	Comment 33 • 17 years ago

hmm.  I will remove it.

	Doug Turner (:dougt) Assignee
	Comment 34 • 17 years ago

clean up dumps. ensuring everything still works.

	Doug Turner (:dougt) Assignee
	Comment 35 • 17 years ago

this is basically javascript version v.5 patch + a preference check in our app-startup.  This patch also include build stuff (mozilla/toolkit/spatial-navigation), and it includes mochitests (which will grow as we start tackling edge cases!).

	Antonio Gomes (tonikitoo)
	Comment 36 • 16 years ago

(In reply to comment #35)
> Created an attachment (id=327154) [details]
> javascript version v.6
> 
> this is basically javascript version v.5 patch + a preference check in our
> app-startup.  

dougt, it seems like pref setup at startup is broken. it throws an exception for me:

[Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getBoolPref]"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"  location: "JS frame :: file:///home/agomes/prism/xulrunner-hg/mozilla-central/objdir/browser/dist/bin/components/spatial-navigation.js :: anonymous :: line 71"  data: no]

and snav does not work at all

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 37 • 16 years ago

(In reply to comment #36)
> dougt, it seems like pref setup at startup is broken. it throws an exception
> for me:
> 
> [Exception... "Component returned failure code: 0x8000ffff
> (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getBoolPref]"  nsresult: "0x8000ffff
> (NS_ERROR_UNEXPECTED)"  location: "JS frame ::
> file:///home/agomes/prism/xulrunner-hg/mozilla-central/objdir/browser/dist/bin/components/spatial-navigation.js
> :: anonymous :: line 71"  data: no]
> 
> and snav does not work at all
> 

Hmm, there is a try/catch around the getBoolPref call. So, if the pref doesn't exist, the thrwo will be caught and "enabled" should stay false.

Antonio - have you added the pref?

	Antonio Gomes (tonikitoo)
	Comment 38 • 16 years ago

I have added 

user_pref("snav.enabled", true);

in prefs.js (profile dir) before testing. I am able to see it in about:config

mark: does snav work for you w patch v6 ?

	Doug Turner (:dougt) Assignee
	Comment 39 • 16 years ago

are you testing in FF3?  That is the only thing this will work in.  I need to adjust how we hook up things to fennec.

	Doug Turner (:dougt) Assignee
	Comment 40 • 16 years ago

in order to make fennec and other applications work, it has been suggested that applications pass their browser element to snav, and not have snav try to figure out which browser to use.

	Doug Turner (:dougt) Assignee
	Comment 41 • 16 years ago

mfinkle also points out that my addEventListener needs to be capturing.

	Doug Turner (:dougt) Assignee
	Comment 42 • 16 years ago

This is the same as v.6, but instead of being a xpcom component, it is a js module.

This does require that any application wanting snav just create a snav object, and attach to it.

 Components.utils.import("resource://gre/modules/spatial-navigation.jsm");
...

 var snav = new SpatialNavigation();
 snav.attach(browser_element);


I also modified how we focus an element.  Instead of directly calling .focus() on the element we want to focus, I had to go through the DOM Utils so that my window would also be focused (required by current versions of fennec):

doc.defaultView.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowUtils).focus(bestElementToFoc\
us);

	Doug Turner (:dougt) Assignee
	Comment 43 • 16 years ago

Comment on attachment 327658 [details] [diff] [review]
javascript version v.7

going to remove attach() and remove() functions.  one sec.

	Doug Turner (:dougt) Assignee
	Comment 44 • 16 years ago

changed the usage a bit:

var snav = new SpatialNavigation(browser_element, optional_callback);

optional_callback will be called if/when spatial navigation does focus something.  This will aide things like fennec that might need to do something (like update its canvas) when focus changes.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 45 • 16 years ago

Comment on attachment 327664 [details] [diff] [review]
javascript version v.8

>diff --git a/toolkit/Makefile.in b/toolkit/Makefile.in

> DIRS	= \
> 		content \
> 		locales \
> 		obsolete \
> 		profile \
> 		themes \
>+		spatial-navigation \

Do we really want this right in the root? Seems like it would be best under components/, or maybe under a new modules/ directory? Ask bsmedberg?

	Benjamin Smedberg
	Comment 46 • 16 years ago

Yes, I want to flatten the hierarchy, and move as much of this as practical into toolkit/featurename rather than hide it behind components/ and mozapps/

	Doug Turner (:dougt) Assignee
	Comment 47 • 16 years ago

same as before, but I changed the mochitest from being a html test, to a chrome test so that I can attach the spatial navigation module directly to an element.  This allows me to test spatial navigation even if the application we are testing doesn't include spatial navigation.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 48 • 16 years ago

Comment on attachment 328417 [details] [diff] [review]
javascript version v.9

>+++ b/toolkit/spatial-navigation/tests/Makefile.in

>+relativesrcdir	= toolkit/spatial-navigation/chrome

This should be toolkit/spatial-navigation/tests

>+MOCHI_CONTENT = $(NULL)

Could just omit this since there are no content files, but...

>diff --git a/toolkit/spatial-navigation/tests/chrome/test_snav.xul b/toolkit/spatial-navigation/tests/chrome/test_snav.xul

>+<body id="some-content" xmlns="http://www.w3.org/1999/xhtml">

>+function onLoad()
>+{
>+    var x = document.getElementById("some-content");
>+    var snav = new SpatialNavigation(x);

Seems like it would probably be better to use an actual <xul:browser> that loads a packaged .html file, so that we're testing something closer to what we're actually using, but I guess if this works it's probably OK.

	Doug Turner (:dougt) Assignee
	Comment 49 • 16 years ago

> This should be toolkit/spatial-navigation/tests

This is a chrome test.  See: http://developer.mozilla.org/en/docs/index.php?title=Mochitest&section=15#Makefile_changes


I can follow up with more tests, and they can test using a xul:browser.  There are plenty more test cases for edge cases we will need.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 50 • 16 years ago

(In reply to comment #49)
> > This should be toolkit/spatial-navigation/tests
> 
> This is a chrome test.  See:
> http://developer.mozilla.org/en/docs/index.php?title=Mochitest&section=15#Makefile_changes

Yeah, but that refers to the "/testing/mochitest/chrome" part of the $(INSTALL) line, not relativesrcdir. The relativesrcdir needs to point to the actual location of the test file for tinderbox error log links to work.

	Doug Turner (:dougt) Assignee
	Comment 51 • 16 years ago

yup.  i changed it locally.

	Doug Turner (:dougt) Assignee
	Comment 52 • 16 years ago

marking fixed.  Please file new bugs.

f514587dd532

	Joel Maher ( :jmaher ) (UTC -8)
	Comment 53 • 15 years ago

verfied with beta3