405774 - Using the Arrows to navigate 'Back' - folder does not follow along

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P3)

Description

[Jim Jeffery not reading bug-mail 1/2/11]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b2pre) Gecko/2007112802 Minefield/3.0b2pre Firefox/3.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b2pre) Gecko/2007112802 Minefield/3.0b2pre Firefox/3.0

1. Open the Organizer (Show all Bookmarks)
2. Click any Folder in the left pane
3. Click the Green 'Back Arrow'
4. Note that the info in the right panel goes 'back' to the previous folder but the highlighted folder in the left pane does not follow along.



Reproducible: Always

Steps to Reproduce:
1.
2.
3.
Actual Results:  
Highlighted Folder does not follow the back/forward command to match with contents in the right-pane. 

Expected Results:  
Highlighted Folder should follow the back/forward command to match with contents in the right-pane. 

Also if you have a mouse with forward/back buttons, they are ignored and do not function. 

Vista HP 
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b2pre) Gecko/2007112802 Minefield/3.0b2pre Firefox/3.0 ID:2007112802

Comment 1

[Jim Jeffery not reading bug-mail 1/2/11]

It was also noted in the MZ Builds Forum, that this does not work in an 1119 build either (before new Places Organizer) 

Comment 2

[(mostly gone) XtC4UaLL [:xtc4uall]]

confirmed with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2pre) Gecko/2007112805

Comment 3

[Asaf Romano (gone)]

Attachment: patch

Comment 4

[Dietrich Ayala (:dietrich)]

Comment on attachment 292332 [details] [diff] [review]
patch



>-            // If the specified child could not be located, just select the first
>-            // item in the list. 
>-            if (this.view.rowCount)
>-              this.view.selection.select(0);
>+            // If the specified child could not be located, clear the selection
>+            var selection = this.view.selection;
>+            selection.clearSelection();
>           }

does this break how the organizer selects the first row by default when opened? do we care now that the default view is no longer a folder's contents?

>Index: browser/components/places/content/places.js
>===================================================================
>RCS file: /cvsroot/mozilla/browser/components/places/content/places.js,v
>retrieving revision 1.116
>diff -u -p -8 -r1.116 places.js
>--- browser/components/places/content/places.js	2 Dec 2007 20:08:24 -0000	1.116
>+++ browser/components/places/content/places.js	9 Dec 2007 21:32:07 -0000
>@@ -59,16 +59,19 @@ var PlacesOrganizer = {
>     this._content = document.getElementById("placeContent");
>     this._initFolderTree();
> 
>     var leftPaneSelection = "AllBookmarks"; // default to all-bookmarks
>     if ("arguments" in window && window.arguments.length > 0)
>       leftPaneSelection = window.arguments[0];
> 
>     this.selectLeftPaneQuery(leftPaneSelection);
>+    // clear the back-stack
>+    this._backHistory.splice(0);
>+    document.getElementById("OrganizerCommand:Back").setAttribute("disabled", true);

why? if the organizer is already open, navigating back seems a valid action.


>   forward: function PO_forward() {
>+    this._backHistory.unshift(this.location);
>     var historyEntry = this._forwardHistory.shift();
>+    this._location = null;
>     this.location = historyEntry;
>   },

why this change? from looking at the location setter, seems that you could remove both of these changes and the code would function the same. i must be missing something.

> 
>   /**
>    * Called when a place folder is selected in the left pane.
>    * @param   resetSearchBox
>    *          true if the search box should also be reset, false if it should
>    *          be left alone.
>    */
>   onPlaceSelected: function PO_onPlaceSelected(resetSearchBox) {
>+    // Don't change the right-hand pane contents when there's no selection
>     if (!this._places.hasSelection)
>       return;
> 
>-    var node = asQuery(this._places.selectedNode);
>-
>-    var queries = node.getQueries({});
>+    var node = this._places.selectedNode;
>+    var queries = asQuery(node).getQueries({});
> 

nit: can you remove the whitespace in the previous couple of empty lines while you're here

Comment 5

[Asaf Romano (gone)]

(In reply to comment #4)
> (From update of attachment 292332 [details] [diff] [review])
> 
> 
> >-            // If the specified child could not be located, just select the first
> >-            // item in the list. 
> >-            if (this.view.rowCount)
> >-              this.view.selection.select(0);
> >+            // If the specified child could not be located, clear the selection
> >+            var selection = this.view.selection;
> >+            selection.clearSelection();
> >           }
> 
> does this break how the organizer selects the first row by default when opened?
> do we care now that the default view is no longer a folder's contents?
> 

which first row is selected by default? selectPlaceURI is not called for the left pane AFAICT.

> >Index: browser/components/places/content/places.js
> >===================================================================
> >RCS file: /cvsroot/mozilla/browser/components/places/content/places.js,v
> >retrieving revision 1.116
> >diff -u -p -8 -r1.116 places.js
> >--- browser/components/places/content/places.js	2 Dec 2007 20:08:24 -0000	1.116
> >+++ browser/components/places/content/places.js	9 Dec 2007 21:32:07 -0000
> >@@ -59,16 +59,19 @@ var PlacesOrganizer = {
> >     this._content = document.getElementById("placeContent");
> >     this._initFolderTree();
> > 
> >     var leftPaneSelection = "AllBookmarks"; // default to all-bookmarks
> >     if ("arguments" in window && window.arguments.length > 0)
> >       leftPaneSelection = window.arguments[0];
> > 
> >     this.selectLeftPaneQuery(leftPaneSelection);
> >+    // clear the back-stack
> >+    this._backHistory.splice(0);
> >+    document.getElementById("OrganizerCommand:Back").setAttribute("disabled", true);
> 
> why? if the organizer is already open, navigating back seems a valid action.
> 
> 
Init is only called on winodw-open, there's no much point in allowing to go back from the initial selection to "".

> >   forward: function PO_forward() {
> >+    this._backHistory.unshift(this.location);
> >     var historyEntry = this._forwardHistory.shift();
> >+    this._location = null;
> >     this.location = historyEntry;
> >   },
> 
> why this change? from looking at the location setter, seems that you could
> remove both of these changes and the code would function the same. i must be
> missing something.
> 

the location setter now clears the forward stack if this._location is still set.

Comment 6

[Asaf Romano (gone)]

mozilla/browser/components/places/content/places.js 1.118
mozilla/browser/components/places/content/tree.xml 1.88

Comment 7

[Jim Jeffery not reading bug-mail 1/2/11]

This is fixed, however - the forward/back buttons on the mouse are still non-functional.  I feel they should work as well..  

comments ?

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b3pre) Gecko/2007121307 Minefield/3.0b3pre Firefox/3.0 ID:2007121307

Comment 8

[Asaf Romano (gone)]

File a new bug please.

Comment 9

[Jim Jeffery not reading bug-mail 1/2/11]

Filed: https://bugzilla.mozilla.org/show_bug.cgi?id=408243

Comment 10

[Tracy Walker [:tracy]]

verified with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre)
Gecko/2007121805 Minefield/3.0b3pre and the equivalent Mac nightly build. 

Comment 11

[Aakash Desai [:aakashd]]

Test case https://litmus.mozilla.org/show_test.cgi?searchType=by_id&id=7459 has been added for regression testing.

Comment 12

[Aakash Desai [:aakashd]]

After rummaging through the library subgroup on 3.1 and 3.0 test runs, test case https://litmus.mozilla.org/show_test.cgi?id=6934 just had to be updated. I've deleted the test case 7459 and have updated the aforementioned test case to the test runs.

for 3.0,
https://litmus.mozilla.org/show_test.cgi?id=5322

for 3.1,
https://litmus.mozilla.org/show_test.cgi?id=6934

Comment 13

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv