User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.3b) Gecko/20030223 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.3b) Gecko/20030223 Sidebar search panel in advanced mode: When selecting "All Engines" and checking multiple search engines, search results for all engines are aggregated in the main window. Right-click on one of the results in the main window produces a small, empty oblong aka contentless context menu. Reproducible: Always Steps to Reproduce: Expected Results: Show context menu as if just checking a single search engine and right-clicking in the results panel in the sidebar. This bug is a spinoff of bug 57654 to keep track of the missing context menu bug! The current implementation of the XUL 'tree' element has *no* 'selectedItems' attribute anymore, which is used in /xpfe/components/search/resources/shared.js to generate the context menu. Hopefully, my patch for #57654 will resolve this bug, but we better keep track of this resolution here.

taking

Karsten, is there any relation to bug 134604 or bug 195213?

> is there any relation to bug 134604 or bug 195213? Not directly. shared.js is not involved there. But I get errors on the javascript console I'll report there.

-> karsten, per his request

Since this diff -u is about twice the size of the new file and many portions are just reformatted, I'll comment the "real" major differences here: 1. license block added 2. function SelectedItemsCount(oNode): New function to simplify the use of tree and listitem 3. function IterateOverItems(oNode, fCallback, oUserData): New function to iterate over all the /selected/ items of a tree or listbox and call the fCallback function for each one with a common oUserData 4. function ProcessSelectedItemFill(oItem, oUserData): New function for use with (3.), containing the core loop functionality of the old fillContextMenu function 5. function fillContextMenu(name, treeName): Minor modifications in popup-cleaning, call to (3.) to get rid of listbox problems. 6. function setSearchMode(): Same functionality, but more elegantly. ;-) 7. function ProcessSelectedItemDo(oItem, oUserData): New function for use with (3.), containing the core loop functionality of the old doContextCmd function 8. function doContextCmd(cmdName, treeName): Mainly the old function, but with listbox problems extracted to (7.) 9. function doSort(sortColName, naturalOrderResource): Only cosmetic changes. 10.function setInitialSort(node, sortDirection): Only cosmetic changes.

Karsten: you should try to stick to the naming convention a file (or block of code) has. In this file it seems that function arguments have no prefix ("arg1", typically it's "aArg1", a for argument), you should do the same (it makes reading a file easier if you don't have to make a mental switch to different naming conventions while reading through it). Sorry to make you go through fixing this patch up. Read http://www.mozilla.org/hacking/mozilla-style-guide.html for other things you might want/need to know about Mozilla coding style.

> Read http://www.mozilla.org/hacking/mozilla-style-guide.html for other > things you might want/need to know about Mozilla coding style. I've read that one some while ago, but very obviously missed the part about prefixes and such. My apologies; I'll update my patch accordingly, of course.

First attack after all this time. ;-)

I'm glad someone is taking care of this and hope this receives a review soon... I'll go ahead and obselete the other bug's patch and clear the reviews. I have one initial comment after a quick review of the code. The following could be written: var selectLength = select_list.length; - if (selectLength == 0) selectLength = 1; - for (var nodeIndex=0; nodeIndex < selectLength; nodeIndex++) + if (!selectLength) + selectLength = 1; + for (var nodeIndex = 0; nodeIndex < selectLength; ++nodeIndex) { <snip> } simpler as: var nodeIndex = select_list.length ? select_list.length - 1 : 0; do { <snip> } while (--nodeIndex);

Yeah, that's right. But to keep the patch simple, I tried to refrain from unrelated clean-up (you may find many such clean-up /inside/ the patch's scope, though).

Comment on attachment 171938 [details] [diff] [review] New start V1 This looks closer down the road to what it should be, but I'll let Neil look at it and r/sr it if he likes it.

Comment on attachment 171938 [details] [diff] [review] New start V1 Neil's pretty overloaded, and jag already reviewed my first patch-attempts in this area in 2003 *g*

Comment on attachment 171938 [details] [diff] [review] New start V1 Just trying to "revive" this stalled bug/patch.

Comment on attachment 171938 [details] [diff] [review] New start V1 I don't think we have sidebar search results any more...

(In reply to neil@parkwaycc.co.uk from comment #14) > Comment on attachment 171938 [details] [diff] [review] > New start V1 > > I don't think we have sidebar search results any more... Sooo, this is WONTFIX then?