Status

enhancement
P2
normal
VERIFIED FIXED
11 years ago
10 years ago

People

(Reporter: christian.bugzilla, Assigned: dougt)

Tracking

(Depends on 2 bugs)

Trunk
fennec1.0m5
Other
Maemo
Dependency tree / graph
Bug Flags:
wanted-fennec1.0 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 9 obsolete attachments)

 
Assignee: nobody → doug.turner
Priority: -- → P2
Target Milestone: --- → Fennec M4
Flags: wanted-fennec1.0+
Depends on: 347731
Posted patch patch v.1 (wip) (obsolete) — Splinter Review
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.
Depends on: 441582
Duplicate of this bug: 347731
adding bugs that I would like to fix with this patch.
Depends on: 305657, 373008, 390426, 395704
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.
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)
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.....
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.
"only one way to find out" :-)
If we put this in chrome, will it then work in the embedding case?
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.
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...
> 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 
err ... I meant *v1*
> 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)
Posted file javascript version v.1 (obsolete) —
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.
Attachment #326929 - Flags: review?
Attachment #326929 - Attachment mime type: application/x-javascript → text/plain
Attachment #326929 - Flags: review? → review?(gavin.sharp)
Posted file Simple testcase
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 ;)
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?
romaxa: if you adjust gRectFudge to zero, your testcase works.
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.

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.
Ok, I will try to share list of already known issues after landing initial version.
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.
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.
Attachment #326929 - Attachment is obsolete: true
Attachment #326929 - Flags: review?(gavin.sharp)
Posted file javascript version v.2 (obsolete) —
Attachment #326997 - Flags: review?
(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?
Posted file javascript version v.3 (obsolete) —
more comments from mfinkle and gavin.
Attachment #326997 - Attachment is obsolete: true
Attachment #326997 - Flags: review?
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.
Attachment #327007 - Flags: review+
with absolute positioning, elements can overlap.  that is what the distance == 0 thing is about.
Posted file javascript version v.4 (obsolete) —
incorporates further feedback.
Attachment #327007 - Attachment is obsolete: true
(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?
hmm.  I will remove it.
Posted file javascript version v.5 (obsolete) —
clean up dumps. ensuring everything still works.
Attachment #327009 - Attachment is obsolete: true
Attachment #327037 - Flags: review?
Attachment #327037 - Flags: review? → review+
Posted patch javascript version v.6 (obsolete) — Splinter Review
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!).
Attachment #326422 - Attachment is obsolete: true
Attachment #327037 - Attachment is obsolete: true
Attachment #327154 - Flags: superreview?
Attachment #327154 - Flags: review?
Attachment #327154 - Flags: superreview?(benjamin)
Attachment #327154 - Flags: superreview?
Attachment #327154 - Flags: review?(gavin.sharp)
Attachment #327154 - Flags: review?
Target Milestone: Fennec M4 → Fennec M5
(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

(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?
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 ?
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.
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.
mfinkle also points out that my addEventListener needs to be capturing.
Posted patch javascript version v.7 (obsolete) — Splinter Review
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);
Attachment #327154 - Attachment is obsolete: true
Attachment #327658 - Flags: superreview?(benjamin)
Attachment #327658 - Flags: review?
Attachment #327154 - Flags: superreview?(benjamin)
Attachment #327154 - Flags: review?(gavin.sharp)
Attachment #327658 - Attachment is patch: true
Attachment #327658 - Attachment mime type: application/octet-stream → text/plain
Attachment #327658 - Flags: review? → review?(gavin.sharp)
Comment on attachment 327658 [details] [diff] [review]
javascript version v.7

going to remove attach() and remove() functions.  one sec.
Attachment #327658 - Attachment is obsolete: true
Attachment #327658 - Flags: superreview?(benjamin)
Attachment #327658 - Flags: review?(gavin.sharp)
Posted patch javascript version v.8 (obsolete) — Splinter Review
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.
Attachment #327664 - Flags: review?
Attachment #327664 - Flags: review? → review?(gavin.sharp)
Blocks: 443043
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?
Attachment #327664 - Flags: review?(gavin.sharp) → review+
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/
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.
Attachment #327664 - Attachment is obsolete: true
Attachment #328417 - Flags: review?(gavin.sharp)
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.
Attachment #328417 - Flags: review?(gavin.sharp) → review+
> 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.
(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.
yup.  i changed it locally.
marking fixed.  Please file new bugs.

f514587dd532
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
verfied with beta3
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.