479384 - Reset button in Library search box is broken

Status: VERIFIED FIXED

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

Description

[Drew Willcoxon :adw]

Clicking the search box's reset button only deletes the search text, and manually deleting the text does nothing.  In either case the scope bar should disappear and we should be returned to the folder we were viewing.

Regwindow: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=65e8380bf4f9&tochange=ddcb3ca67b5e

These are two consecutive revisions (WFM on the older revision), so it looks like http://hg.mozilla.org/mozilla-central/rev/ddcb3ca67b5e is the cause.  Related bug is bug 475273.  From that bug:

>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
>@@ -194,22 +194,29 @@ var PlacesOrganizer = {
>   },
> 
>   /**
>    * 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.
>    */
>+  _cachedLeftPaneSelectedNode: null,
>   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 = this._places.selectedNode;
>+    // When we invalidate a container we use suppressSelectionEvent, when it is
>+    // unset a select event is fired, in many cases the selection did not really
>+    // change, so we should check for it, and return early in such a case.
>+    if (node == this._cachedLeftPaneSelectedNode)
>+      return;
>+    this._cachedLeftPaneSelectedNode = node;

The new return statement is reached, and the logic in the !resetSearchBox conditional below this new code doesn't get called.  That's all I know.

Comment 1

[Marco Bonardo [:mak]]

yes, could be that, even if the selection did not change, we should look for resetSearchBox.

Comment 2

[Marco Bonardo [:mak]]

asking blocking p2 to get preapproval to fix the regression

Comment 3

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

Giving blocking p2 because I think this issue actually blocks the release!

Comment 4

[Drew Willcoxon :adw]

Attachment: v1

Was going to solve this in bug 451151, but since the patch for that bug could potentially introduce regressions and this bug is a blocker, broke out a patch per conversation with Dietrich.

Comment 5

[Dietrich Ayala (:dietrich)]

Comment on attachment 363946 [details] [diff] [review]
v1

r=me

Comment 6

[Mike Connor [:mconnor]]

http://hg.mozilla.org/mozilla-central/rev/d801dcf72ec8

Comment 7

[Marco Bonardo [:mak]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/69549aec001c

Comment 8

[Charles Fenwick]

Verified
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.1b3pre) Gecko/20090301 Shiretoko/3.1b3pre

Comment 9

[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