451151 - Search box In Places Organizer (Library) not behaving correctly

Status: RESOLVED FIXED

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

Description

[Rick]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.1) Gecko/2008070208 Firefox/3.0.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.1) Gecko/2008070208 Firefox/3.0.1

There are a couple of issues here.

1. In the Places Organizer window, if you type some text in the search box and click on "All Bookmarks" in the scopebar, but then backspace back to an empty string, the scope will be reset to "Selected Folder", which will be evident in the scopebar as soon as you start typing in the box again.

2. If you hit Ctrl-F, it will focus the search box and should set the scope to "All Bookmarks", as opposed to "Selected Folder" (which is what Ctrl-Shift-F does).  Instead, when you start typing you will see "Selected Folder" active in the scopebar.  Moreover, the results you get will not be what you expect for "Selected Folder", or even "All Bookmarks".  It seems the results, if any, are what you would expect if the scope was "History".


Reproducible: Always

Steps to Reproduce:
1. Open Places Organizer (Library) window
2. Click on any folder in the left pane except "All Bookmarks"
3. Hit Ctrl-F to do a "Find in Bookmarks" search (should jump to search box)
4. Type something in the search box you know will match a bookmark not in the currently selected folder
5. Notice the scope is "Selected Folder" (incorrect)
6. Notice the results are not what is expected for "Selected Folder", or "All Bookmarks", but perhaps "History"
7. Click on the "All Bookmarks" scope button
8. Notice you get the appropriate results
9. Backspace over your text and retype it
10. Notice that the scope is now "Selected Folder" (incorrect) and the results are consistent with that

Comment 1

[Rick]

In my attempt to debug this:

1. For the first issue, in places.js:
PlacesOrganizer.onPlaceSelected(false) is called whenever search box becomes "", eg by backspacing, on line 794.  This naturally resets the scope to whichever folder is selected, which is a problem if the scope is supposed to be something else.

  search: function PSB_search(filterString) {
    var PO = PlacesOrganizer;
    // If the user empties the search box manually, reset it and load all
    // contents of the current scope.
    // XXX this might be to jumpy, maybe should search for "", so results
    // are ungrouped, and search box not reset
    if ((filterString == "" || this.searchFilter.hasAttribute("empty"))) {
-->   PO.onPlaceSelected(false);
      return;
    }
    ...

A workaround hack to fix this is to modify onPlaceSelected() to not do _setSearchScopeForNode() if called with the parameter resetSearchBox to false:

  onPlaceSelected: function PO_onPlaceSelected(resetSearchBox) {  <-- line 180
  ...
    // Make sure the search UI is hidden.
    PlacesSearchBox.hideSearchUI();
    if (resetSearchBox) {
      var searchFilter = document.getElementById("searchFilter");
      searchFilter.reset();
      this._setSearchScopeForNode(node); //RC<-- ...TO HERE (from below)
    }

    //RC this._setSearchScopeForNode(node);  <-- MOVE THIS LINE UP...
    ...

Comment 2

[Rick]

In debugging the second problem:

2. Ctrl-F is mapped to PlacesSearchBox.findAll() in places.js
First, filterCollection should be more appropriately set to "bookmarks" instead of "all".
Second, need to update the scopebar so it will not be stuck on "Selected Folder"
  /**
   * Finds across all bookmarks
   */
  findAll: function PSB_findAll() {  <-- line 839
    //RC this.filterCollection = "all";       <-- SHOULD BE "bookmarks"
    this.filterCollection = "bookmarks"; //RC <-- HERE

    //RC Now update the scopebar              <-- INSERT THIS CODE FROM HERE...
    var scopeButton = "scopeBarAll";
    var scopeBar = document.getElementById("organizerScopeBar");
    var child = scopeBar.firstChild;
    while (child) {
      if (child.getAttribute("id") != scopeButton)
        child.removeAttribute("checked");
      else
        child.setAttribute("checked", "true");
      child = child.nextSibling;
    }
    //RC                                      <-- ...TO HERE

    this.focus();
  },


This seems to work.  But if you decide to leave filterCollection to "all", it points out the weird results you get (HISTORY instead of ALL or SELECTED) in the switch statement in PlacesSearchBox.search():

  search: function PSB_search(filterString) { <-- line 786
    ...
    switch (PlacesSearchBox.filterCollection) {
    ...
    case "all":
      content.applyFilter(filterString);      <-- LOOK HERE
      break;
    }

The function applyFilter() has two parameters, the filter string, and an array of folders to filter on.  It seems like it should work if you leave out the 2nd argument, as done here, but you get the weird history results.  If you instead pass it an empty array, it seems to work as expected.
      content.applyFilter(filterString, []);  <-- LOOK HERE

Hope you don't mind me posting code snippets here, but I just wanted to share what I knew about it more specifically.

Comment 3

[Paul J. Weiss]

I can confirm the problem, and that fixing it would be quite helpful.

Comment 4

[Paul J. Weiss]

This in part seems to be a duplicate of bug 434704.

Comment 5

[Dietrich Ayala (:dietrich)]

Not going to hold the 3.1 release for this, but would definitely take a patch. Rick, can you make your change into a patch?

http://developer.mozilla.org/En/Creating_a_patch

Comment 6

[Drew Willcoxon :adw]

Attachment: v1 (fixes several bugs)

This patch fixes several related bugs that touch the same code: this bug, Bug 469437, Bug 434704, and Bug 479532.  mak, it also fixes Bug 479384, which was right there, so I fixed it, but you should make sure it doesn't reopen bug 475273.  The summary for this bug is pretty general yet accurate, so I've attached the patch here.

Re: Bug 479532, the Find menuitem on OS X is now bound to the OrganizerCommand_find:all command instead of OrganizerCommand_find:current.  Their access keys should be the same (at least they are in en-US) plus with Bug 469437, searching by all bookmarks now makes more sense.

I took the opportunity to clean up the code a little and eliminate some duplicate code and paths.

Comment 7

[Drew Willcoxon :adw]

Attachment: v2

Comment 8

[Drew Willcoxon :adw]

Attachment: v3

Noticed a small problem with updating the OrganizerCommand_find:current label in PO__setScopeForNode().  v2 fixed a problem with updating PlacesSearchBox.filterCollection in PQB_setScope().  Sorry!

Comment 9

[Marco Bonardo [:mak]]

Comment on attachment 363559 [details] [diff] [review]
v3

>diff --git a/browser/components/places/content/places.js b/browser/components/places/content/places.js
> 
>     window.addEventListener("AppCommand", this, true);
> #ifdef XP_MACOSX
>-    // 1. Map Edit->Find command to the organizer's command
>+    // 1. Map Edit->Find command to OrganizerCommand_find:all
>     var findMenuItem = document.getElementById("menu_find");
>-    findMenuItem.setAttribute("command", "OrganizerCommand_find:current");
>-    var findKey = document.getElementById("key_find");
>-    findKey.setAttribute("command", "OrganizerCommand_find:current");
>+    findMenuItem.setAttribute("command", "OrganizerCommand_find:all");

i'm unsure how this works on osX, but looks like key_find is the key binding in editMenuOverlay.xul and we should set command on both key_find and menu_find

>    * Called when a place folder is selected in the left pane.
>-   * @param   resetSearchBox
>+   * @param   shouldResetSearchBox

why this param name change? adding "should" looks like it should do that, but we will ignore it sometimes. Please revert to the old imperative verb.

>    *          true if the search box should also be reset, false if it should
>-   *          be left alone.
>+   *          be left alone.  If user manually reset the search box by either
>+   *          clicking its reset button or deleting its text, this is false.

imho would be more clear as "should be reset, false otherwise.  Notice that if the user manually resets the search box, ..., this will be false.". And we should explain why the search box should be reset, so the fact the selection changed and so we must update the scope.


> 
>   /**
>-   * Sets the search scope based on node's properties
>+   * Sets the search scope based on aNode's properties.
>    * @param   aNode
>    *          the node to set up scope from
>    */
>   _setSearchScopeForNode: function PO__setScopeForNode(aNode) {
>-    var scopeBarFolder = document.getElementById("scopeBarFolder");
>     var itemId = aNode.itemId;
>+    var isHistory = false;
>     if (PlacesUtils.nodeIsHistoryContainer(aNode) ||
>         itemId == PlacesUIUtils.leftPaneQueries["History"]) {
>-      scopeBarFolder.disabled = true;
>-      var folders = [];
>-      var filterCollection = "history";
>-      var scopeButton = "scopeBarHistory";
>+      PlacesQueryBuilder.setScope("history");
>+      isHistory = true;
>     }
>-    else if (PlacesUtils.nodeIsFolder(aNode) &&
>-             itemId != PlacesUIUtils.leftPaneQueries["AllBookmarks"] &&
>-             itemId != PlacesUIUtils.leftPaneQueries["Tags"] &&
>-             aNode.parent.itemId != PlacesUIUtils.leftPaneQueries["Tags"]) {
>-      // enable folder scope
>-      scopeBarFolder.disabled = false;
>-      var folders = [PlacesUtils.getConcreteItemId(aNode)];
>-      var filterCollection = "collection";
>-      var scopeButton = "scopeBarFolder";
>+    // Default to All Bookmarks for all other nodes, per bug 469437.
>+    else
>+      PlacesQueryBuilder.setScope("bookmarks");

mh wait, when we are on a root the default search scope should be the root folder. does this still work with your changes? Looks like you're always defaulting to all bookmarks.

>+
>+    // Enable or disable the folder scope button.
>+    var folderButton = document.getElementById("scopeBarFolder");
>+    folderButton.disabled =
>+      isHistory ||
>+      !PlacesUtils.nodeIsFolder(aNode) ||
>+      itemId == PlacesUIUtils.leftPaneQueries["AllBookmarks"] ||
>+      itemId == PlacesUIUtils.leftPaneQueries["Tags"] ||
>+      aNode.parent.itemId == PlacesUIUtils.leftPaneQueries["Tags"];

once you state !PlacesUtils.nodeIsFolder(aNode) and excluded AllBookmarks, check for tags or history is quite useless, both Tags, History and single tags containers are queries (not folders). This is based on an old assumption when tags were folders.

>   /**
>    * Updates the display with the title of the current collection.
>    * @param   title
>    *          The title of the current collection.
>    */
>@@ -963,18 +952,16 @@ var PlacesSearchBox = {
>   get filterCollection() {
>     return this.searchFilter.getAttribute("collection");
>   },
>   set filterCollection(collectionName) {
>     if (collectionName == this.filterCollection)
>       return collectionName;
> 
>     this.searchFilter.setAttribute("collection", collectionName);
>-    if (this.searchFilter.value)
>-      return collectionName; // don't overwrite pre-existing search terms

why this removal? iirc was added as a sanity check to avoid search box "jumpiness"


>+    // Check the appropriate scope button in the scope bar.
>     var bar = document.getElementById("organizerScopeBar");
>     var child = bar.firstChild;
>     while (child) {
>-      if (child.getAttribute("id") != id)
>+      if (child.getAttribute("id") != scopeButtonId)
>         child.removeAttribute("checked");
>       else
>         child.setAttribute("checked", "true");
>       child = child.nextSibling;
>     }

i'm probably OT, but i don't understand why these are not <button type="radio"> in a radiogroup

Comment 10

[Drew Willcoxon :adw]

(In reply to comment #9)
> >    * Called when a place folder is selected in the left pane.
> >-   * @param   resetSearchBox
> >+   * @param   shouldResetSearchBox
> 
> why this param name change? adding "should" looks like it should do that, but
> we will ignore it sometimes. Please revert to the old imperative verb.

OK, it's just that it often confused me when working with this function -- does it mean "should reset" or "did reset"?  So I changed the name to more accurately reflect the comment.  I'll change it back.

> mh wait, when we are on a root the default search scope should be the root
> folder. does this still work with your changes? Looks like you're always
> defaulting to all bookmarks.

If you re-read bug 469437 comment 0, Alex clearly states that All Bookmarks should be the default for root folders, too.  In fact that appears to be the main point of that bug.

> >   /**
> >    * Updates the display with the title of the current collection.
> >    * @param   title
> >    *          The title of the current collection.
> >    */
> >@@ -963,18 +952,16 @@ var PlacesSearchBox = {
> >   get filterCollection() {
> >     return this.searchFilter.getAttribute("collection");
> >   },
> >   set filterCollection(collectionName) {
> >     if (collectionName == this.filterCollection)
> >       return collectionName;
> > 
> >     this.searchFilter.setAttribute("collection", collectionName);
> >-    if (this.searchFilter.value)
> >-      return collectionName; // don't overwrite pre-existing search terms
> 
> why this removal? iirc was added as a sanity check to avoid search box
> "jumpiness"

Hmm, by returning early the only thing that's avoided is updating the search box's emptytext.  Without updating it, the emptytext can become out of sync with the current scope.  Is it possible this early return was guarding against something old in the code that is no longer there?  Or does setting emptytext cause jumpiness?

> i'm probably OT, but i don't understand why these are not <button type="radio">
> in a radiogroup

Looks like they are actually <toolbarbutton type="radio">.  I'll check it out.

Comment 11

[Marco Bonardo [:mak]]

(In reply to comment #10)
> If you re-read bug 469437 comment 0, Alex clearly states that All Bookmarks
> should be the default for root folders, too.  In fact that appears to be the
> main point of that bug.

dunno why but i was reading that backwards, you're right 

> > >   /**
> > >    * Updates the display with the title of the current collection.
> > >    * @param   title
> > >    *          The title of the current collection.
> > >    */
> > >@@ -963,18 +952,16 @@ var PlacesSearchBox = {
> > >   get filterCollection() {
> > >     return this.searchFilter.getAttribute("collection");
> > >   },
> > >   set filterCollection(collectionName) {
> > >     if (collectionName == this.filterCollection)
> > >       return collectionName;
> > > 
> > >     this.searchFilter.setAttribute("collection", collectionName);
> > >-    if (this.searchFilter.value)
> > >-      return collectionName; // don't overwrite pre-existing search terms
> > 
> > why this removal? iirc was added as a sanity check to avoid search box
> > "jumpiness"
> 
> Hmm, by returning early the only thing that's avoided is updating the search
> box's emptytext.  Without updating it, the emptytext can become out of sync
> with the current scope.  Is it possible this early return was guarding against
> something old in the code that is no longer there?  Or does setting emptytext
> cause jumpiness?

i was making confusion with bug 475331. So this is now using emptytext, while before we were using actual search box content (it was not a real search textbox), and in that vision had a sense not overwriting search terms... So, ok, your removal looks fine now.

> 
> > i'm probably OT, but i don't understand why these are not <button type="radio">
> > in a radiogroup
> 
> Looks like they are actually <toolbarbutton type="radio">.  I'll check it out.

yes thanks, why do we need to set them nanually, are we missing a radiogroup?

Comment 12

[Drew Willcoxon :adw]

Attachment: v4

Changes re: comment 9, comment 11, thanks.

Comment 13

[Drew Willcoxon :adw]

Attachment: v5

I started working on bug 469436 and noticed a small existing problem with the code:  when the selected folder is renamed and there is an active search, the scope bar and search string go away but the content tree is not updated.  It's confusing.  This version of the patch fixes that by keeping the search UI in sync.

Also, I was able to get rid of the _cachedLeftPaneSelectedNode member, because we can reduce the check for changed selection to comparing the old and new content tree place URIs (right?), which we were already doing anyway.  (If old URI == new URI, selection not changed.)

All changes in this version (as diffed with v4) are in PO_onPlaceSelected().

Comment 14

[Marco Bonardo [:mak]]

Comment on attachment 363783 [details] [diff] [review]
v5

>+    // Update the right-pane contents.  We must update also if the user clears
>+    // the search box; in that case we are called with resetSearchBox == false.
>+    var oldContentURI = this._content.place;
>+    if (this._content.place != newContentURI || !resetSearchBox) {

create a var selectionChanged = this._content.place != newContentURI; and use it here and below
it should be more readable. But see below first.

> 
>+    // At this point, resetSearchBox == true: if !resetSearchBox, the selection
>+    // has not changed, and we returned early.

This is a bit confusing, we can be called with these cases:
1. left pane selection has changed (Select event) -> selectionChanged && resetSearchBox
2. left pane selection has not changed (Select event due to suppressSelection) -> !selectionChanged && resetSearchBox
3. left pane selection has not changed, we are searching or changing scope (right pane uri changed) -> every time we search the right pane uri changes but the left pane selection does not change, and we are always called with !resetSearchBox
4. left pane selection has not changed, user cleared the search box -> we are called with !resetSearchBox

So looking at 3rd and 4th cases the use of content uri to check selection is probably not working as intended, since the right pane will change its uri without the need to change the left pane selection.

>+    folderButton.disabled =
>+      !PlacesUtils.nodeIsFolder(aNode) ||
>+      itemId == PlacesUIUtils.leftPaneQueries["AllBookmarks"];

Directly use PlacesUIUtils.AllBookmarksFolderId and if possible align on the right of .disabled = (should not go over 80 chars now)

Comment 15

[Drew Willcoxon :adw]

Attachment: v6

(In reply to comment #14)
> So looking at 3rd and 4th cases the use of content uri to check selection is
> probably not working as intended, since the right pane will change its uri
> without the need to change the left pane selection.

Ah, completely right, thanks.  (The only reason it worked is because bug 476952 isn't fixed.)

But, I noticed that if you rename the selected folder, node != this._cachedLeftPaneSelectedNode even though the selection does not change.  (Rename causes an entirely new results node object to take the place of the old?  I guess that's not surprising.)  So instead this patch checks node.uri != this._cachedLeftPaneSelectedURI.

Comment 16

[Drew Willcoxon :adw]

Attachment: v7

(In reply to comment #14)
> Directly use PlacesUIUtils.AllBookmarksFolderId and if possible align on the
> right of .disabled = (should not go over 80 chars now)

PlacesUIUtils.AllBookmarksFolderId should be PlacesUIUtils.allBookmarksFolderId.  Sorry, I should have checked myself.

Comment 17

[Marco Bonardo [:mak]]

Comment on attachment 363968 [details] [diff] [review]
v7

>diff --git a/browser/components/places/content/places.js b/browser/components/places/content/places.js
>--- a/browser/components/places/content/places.js
>+++ b/browser/components/places/content/places.js
>+    // If either the place of the content tree in the right pane has changed or
>+    // the user cleared the search box, update the place, hide the search UI,
>+    // and update the back/forward buttons by setting location.
>+    var contentPlaceChanged = this._content.place != placeURI;

this had a sense before, when we had the need to do the same compare multiple times, now it's not much useful to pull the compare out of the if and in a var. sorry, i wasn't clear probably.

>+    this._cachedLeftPaneSelectedURI = node.uri;

add a newline here

>+    // At this point, resetSearchBox == true, because if resetSearchBox == false,
>+    // the user reset the search box, and therefore the left pane selection has
>+    // not changed and we returned early.

it's still a bit confuse, let's simply say "At this point, resetSearchBox is true, because the left pane selection has changed, otherwise we would have returned earlier."

Apart that looks good, only 2 things left:
- the patch does not apply cleanly to current trunk here and will need an unbitrot after bug 479384, so take care of unbitrotting this when needed
- i think we could have a basic test that puts text in the Library search box, checks content pane is correct (is filtering on that term), then resets the search box and checks again the content pane. Would that be hard to do?

Comment 18

[Drew Willcoxon :adw]

Attachment: v8

1. Changes re: comment 17
2. Based this patch off of latest m-c + the patch in bug 479384.  I'm assuming that patch will land first, since it's a blocker and ready to go.
3. Added a browser chrome test, requesting review.

Comment 19

[Drew Willcoxon :adw]

Attachment: v9

Forgot that SimpleTest.executeSoon() is async, so win.close() and finish() should be inside it, not outside.  Someday I will get this right.

Comment 20

[Marco Bonardo [:mak]]

Comment on attachment 364066 [details] [diff] [review]
v9

>diff --git a/browser/components/places/tests/browser/browser_library_search.js b/browser/components/places/tests/browser/browser_library_search.js
>+function search(aFolderId, aSearchStr, aExpectedScopeButtonId) {
>+  var doc = libraryWin.document;
>+  var folderTree = doc.getElementById("placesList");
>+  var contentTree = doc.getElementById("placeContent");
>+
>+  // First, ensure that selecting the folder in the left pane updates the
>+  // content tree properly.
>+  if (aFolderId) {
>+    folderTree.selectItems([aFolderId]);
>+    isnot(folderTree.selectedNode, null,
>+       "Sanity check: left pane tree should have selection after selecting!");
>+
>+    // History node in left pane does not have an ID.

not much clear, History node has an id, it's a child of leftPaneFolderId.
So, i can't glue this comment with the compare below:
>+    if (aFolderId !== PlacesUIUtils.leftPaneQueries["History"]) {

Comment 21

[Drew Willcoxon :adw]

Attachment: v10

Comment clarified.

Marco, I noticed that the test hangs just before closing Minefield about half the time I run it.  I'm not sure if it's a problem with the test or my machine.  If you have time could you try it?

Comment 22

[Marco Bonardo [:mak]]

i've tried that and it was working fine, i'll do more runs

Comment 23

[Marco Bonardo [:mak]]

Comment on attachment 364121 [details] [diff] [review]
v10

ok, i've run browser chrome tests about 20 times, without seeing any hang, nor i see any timeout issue that could arise from the test.
Usually on my VM i see hangs when something is wrong, but tinderboxes are strange beasts.

Comment 24

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/3e87c90e48ca

Comment 25

[Marco Bonardo [:mak]]

Comment on attachment 364121 [details] [diff] [review]
v10

medium risk, fixes some common issue when using the search box in the Library, making the behavior consistent with Faaborg's requests.
Has a browser-chrome test.

Comment 26

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 364121 [details] [diff] [review]
v10

a191=beltzner

Comment 27

[Shawn Wilsher :sdwilsh]

This patch does not apply cleanly to mozilla-1.9.1

Comment 28

[Drew Willcoxon :adw]

(In reply to comment #27)
Marco's bug 469436 comment 29 applies here too (excluding bug 469436 itself of course).

Comment 29

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/defb09f7b933

Comment 30

[Aakash Desai [:aakashd]]

verified FIXED on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3) Gecko/20090305 Firefox/3.1b3 ID:20090305133223 for 1.9.1

Comment 31

[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