Closed Bug 436084 Opened 13 years ago Closed 13 years ago
Assignee: nobody → doug.turner
Priority: -- → P2
Target Milestone: --- → Fennec M4
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.
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)
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)
Attachment #326929 - Flags: review?
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?
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
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.
(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?
filters: http://mxr.mozilla.org/mozilla-central/source/editor/ui/composer/content/StructBarContextMenu.js#75 Okay, i will change that too.
more comments from mfinkle and gavin.
Attachment #327007 - Flags: review+
with absolute positioning, elements can overlap. that is what the distance == 0 thing is about.
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.
clean up dumps. ensuring everything still works.
Attachment #327037 - Flags: review? → review+
(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.
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);
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)
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)
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§ion=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§ion=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: 13 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.