library search should default to Selected Folder

VERIFIED FIXED in Firefox 3

Status

()

Firefox
Bookmarks & History
P2
normal
VERIFIED FIXED
11 years ago
8 years ago

People

(Reporter: aryx, Assigned: mak)

Tracking

({late-l10n})

Trunk
Firefox 3
late-l10n
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3 +
in-litmus +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [strings landed][has patch][has review])

Attachments

(4 attachments, 4 obsolete attachments)

Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.9b2pre) Gecko/2007112805 Minefield/3.0b2pre - Build ID: 2007112805

1. Go to History > Show all history...
2. You have to perform a bookmarks search in the upper right corner to be able to change the search to "history".

Expected result:
1. Search box in upper right corner shows "history" and searches history.
2. In general a way to access the extended search without performing a search.
IMO, this is a bug. Yes, the search field says "Search bookmarks." But I've selected my history window. I see a bunch of history in the window. Why would typing in the search box search an entirely different set of data?

The right way to fix this is probably to explicitly show the History source as being selected in the LHS source bar, and to automatically update the search bar to "Search in <foo>" where <foo> is the currently selected source. 
Severity: enhancement → normal
(Assignee)

Comment 3

10 years ago
this should be really fixed with a unified search in Bug 378798, i suggest duping to that
Depends on: 378798
OS: Windows XP → All
Requesting blocking as this is going to be a glaring mistake if not fixed.  There seems to be bugs pointing back and forth to should be 'fixed' with.. but that in turn points somewhere else. 

If there is no intent to correct the issue of opening 'Show all History' and getting a search box that says 'search in history'  and provide the correct 'View Menu' options related to History, not the bookmarks View Menu as per the mockup here: 
http://people.mozilla.com/~faaborg/files/granParadisoUI/places_OrganizerHistory_i5.png

Then I would suggest perhaps that 'History' be removed/hidden in the Library and in the Application/Browser Window : History->Show All History be changed from opening the Library to merely opening the Side-bar.  

Flags: blocking-firefox3?
Morphing.

The right solution is that the Library search should default to "Selected Folder". This will also result in the grey text in that field updating correctly. When a user selects "Show all History" that folder will be selected and the searchbox will say "Search in 'History'".

(In reply to comment #4)
> Then I would suggest perhaps that 'History' be removed/hidden in the Library
> and in the Application/Browser Window : History->Show All History be changed
> from opening the Library to merely opening the Side-bar.  

Absolutely not.
Flags: blocking-firefox3? → blocking-firefox3+
Priority: -- → P2
Summary: No direct way to search history if opened from history menu → library search should default to Selected Folder
Whiteboard: [morph in comment #6]

Updated

10 years ago
Assignee: nobody → mak77

Updated

10 years ago
Whiteboard: [morph in comment #6] → [morph in comment #6][needs status update]
(Assignee)

Updated

10 years ago
Whiteboard: [morph in comment #6][needs status update] → [morph in comment #6][swag: 1d]
(Assignee)

Updated

10 years ago
Blocks: 426148
(Assignee)

Updated

10 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

10 years ago
Blocks: 425854
(Assignee)

Comment 8

10 years ago
Created attachment 313062 [details]
screenshot, fixed style on winstripe

this fixes the scopeBar style, i've removed bookmarks menu and bookmarks toolbar scopes, you can see the three states (selected, hover, unselected), so could potentially fix Bug 425854 and Bug 426148
Attachment #313062 - Flags: ui-review?(beltzner)
(Assignee)

Comment 9

10 years ago
Created attachment 313063 [details] [diff] [review]
patch
Attachment #312915 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
Whiteboard: [morph in comment #6][swag: 1d] → [has patch] [needs UI review beltzner]
Duplicate of this bug: 422076
Hardware: PC → All
The Save button is still styled oddly, otherwise I think it looks good.  Waiting on beltzner though.
Attachment #313062 - Flags: ui-review?(beltzner) → ui-review+
(Assignee)

Comment 12

10 years ago
yes the save button is not so cool, if someone has a suggestion for that, hwv could be addressed later
Does this default to the selected top level folder (bookmarks, tags, history) or literally the selected folder?  If you are in a folder with then bookmarks, doing a search just over those ten bookmarks is kind of silly since you can see all of them.  The screen shot of searching just the bookmarks toolbar folder has me concerned.
Created attachment 313281 [details] [diff] [review]
Add searchHistory string

The text in the search box currently uses the searchBookmarks string ("Search Bookmarks"), so it makes sense to have a searchHistory string ("Search History") to display when the history folder is open.
Attachment #313281 - Flags: ui-review?(beltzner)
Attachment #313281 - Flags: review?(beltzner)
Keywords: late-l10n
Whiteboard: [has patch] [needs UI review beltzner] → [has patch] [needs UI review beltzner] [1 string]
(Assignee)

Comment 15

10 years ago
(In reply to comment #13)
> Does this default to the selected top level folder (bookmarks, tags, history)
> or literally the selected folder?  

if you're in a history container searches into full history. in Tags or all Bookmarks search full bookmarks (toolbar, menu, unfiled). in a valid Bookmarks folder searches in that folder, the user can then click "All Bookmarks" in the scope bar to move to full bookmarks search.

(In reply to comment #14)
> Created an attachment (id=313281) [details]
> Add searchHistory string
> 
> The text in the search box currently uses the searchBookmarks string ("Search
> Bookmarks"), so it makes sense to have a searchHistory string ("Search
> History") to display when the history folder is open.

It does already "Search in 'History' using the title of the top history container, so actually a new string is not needed, It's not the same as Search Bookmarks but it's quite similar imo

(In reply to comment #15)
> It does already "Search in 'History' using the title of the top history
> container, so actually a new string is not needed, It's not the same as Search
> Bookmarks but it's quite similar imo

I think "Search History" looks much better than "Search in 'History'", so I do think it is needed and matters.
Comment on attachment 313281 [details] [diff] [review]
Add searchHistory string

Yeah, I agree, we might as well polish this up.
Attachment #313281 - Flags: ui-review?(beltzner)
Attachment #313281 - Flags: ui-review+
Attachment #313281 - Flags: review?(beltzner)
Attachment #313281 - Flags: review+
Attachment #313281 - Flags: approval1.9?
(Assignee)

Comment 18

10 years ago
ok, i'll merge Reed string change in my patch
(Assignee)

Updated

10 years ago
Whiteboard: [has patch] [needs UI review beltzner] [1 string] → [has patch][1 string needs explicit approval]

Updated

10 years ago
Attachment #313281 - Flags: approval1.9? → approval1.9+
Keywords: checkin-needed
(Assignee)

Comment 19

10 years ago
Created attachment 313395 [details] [diff] [review]
patch

we can check-in reed string change. But if this patch is ok and mano has enough time to review we could check in the full patch. I'll be able to address comments in 3 hours if needed
Attachment #313063 - Attachment is obsolete: true
Attachment #313395 - Flags: review?(mano)
Whiteboard: [has patch][1 string needs explicit approval] → [has patch][1 string needs explicit approval][needs review mano]
Target Milestone: --- → Firefox 3
Whiteboard: [has patch][1 string needs explicit approval][needs review mano] → [has patch][1 string][needs review mano]
Aren't some files included twice in your patch?
string:

Checking in browser/locales/en-US/chrome/browser/places/places.properties;
/cvsroot/mozilla/browser/locales/en-US/chrome/browser/places/places.properties,v  <--  places.properties
new revision: 1.40; previous revision: 1.39
done
Keywords: checkin-needed
Whiteboard: [has patch][1 string][needs review mano] → [strings landed]
Comment on attachment 313395 [details] [diff] [review]
patch

And on the next patch, please avoid calling asQuery multiple times
Attachment #313395 - Flags: review?(mano)
(Assignee)

Comment 23

10 years ago
(In reply to comment #20)
> Aren't some files included twice in your patch?

oh damn, i included the path twice when generating :/ sorry

(Assignee)

Updated

10 years ago
Attachment #313395 - Attachment is obsolete: true
(Assignee)

Comment 24

10 years ago
Created attachment 313501 [details] [diff] [review]
patch
Attachment #313501 - Flags: review?(mano)
Whiteboard: [strings landed] → [strings landed][needs review mano]
Looks good, what about updating winstripe & pinstripe though?
(In reply to comment #25)
> Looks good, what about updating winstripe & pinstripe though?

gnomestripe and pinstripe, you mean?
(Assignee)

Comment 27

10 years ago
pinstripe has been already fixed some time ago, has a round style (i don't have a mac to check but stylesheet appear good)... winstripe has the oldest Library style between the three (has not been updated in a long time). will take a look in ubuntu for gnomestripe
(Assignee)

Comment 28

10 years ago
Created attachment 313560 [details]
screenshot on gnomestripe

gnomestripe looks already good to me
Keywords: late-l10n
Keywords: late-l10n
Comment on attachment 313501 [details] [diff] [review]
patch

>Index: toolkit/components/places/src/utils.js
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/components/places/src/utils.js,v
>retrieving revision 1.12
>diff -u -8 -p -r1.12 utils.js
>--- toolkit/components/places/src/utils.js	3 Apr 2008 21:20:39 -0000	1.12
>+++ toolkit/components/places/src/utils.js	4 Apr 2008 00:36:48 -0000
>@@ -298,16 +298,33 @@ var PlacesUtils = {
>                    Ci.nsINavHistoryResultNode.RESULT_TYPE_FOLDER_SHORTCUT,
>                    Ci.nsINavHistoryResultNode.RESULT_TYPE_QUERY,
>                    Ci.nsINavHistoryResultNode.RESULT_TYPE_DYNAMIC_CONTAINER],
>   nodeIsContainer: function PU_nodeIsContainer(aNode) {
>     return this.containerTypes.indexOf(aNode.type) != -1;
>   },
> 
>   /**
>+   * Determines whether or not a ResultNode is an history related container.

nit: a result-node

>Index: browser/components/places/content/places.js
>===================================================================
>RCS file: /cvsroot/mozilla/browser/components/places/content/places.js,v
>retrieving revision 1.152
>diff -u -8 -p -r1.152 places.js
>--- browser/components/places/content/places.js	3 Apr 2008 20:51:54 -0000	1.152
>+++ browser/components/places/content/places.js	4 Apr 2008 00:36:50 -0000
>@@ -218,24 +218,71 @@ var PlacesOrganizer = {
> 
>     // Make sure the search UI is hidden.
>     PlacesSearchBox.hideSearchUI();
>     if (resetSearchBox) {
>       var searchFilter = document.getElementById("searchFilter");
>       searchFilter.reset();
>     }
> 
>-    // Update the "Find in <current collection>" command and the gray text in
>-    // the search box in the toolbar if the active collection is the current
>-    // collection.
>+    this._setSearchScopeForNode(node);
>+  },
>+
>+  /**
>+   * set the search scope based on node properties

mega-nit: Sets...the node's properties

>+   * @param   aNode
>+   *          the node to set up scope for

hrm, s/for/from?

>+   */
>+  _setSearchScopeForNode: function PO__setScopeForNode(aNode) {

Pur aNode.itemId in a variable.

Plus the pinstripe changes we discussed over irc, which is the only reason for denying. ;)
Attachment #313501 - Flags: review?(mano) → review-
(Assignee)

Comment 30

10 years ago
Created attachment 313803 [details] [diff] [review]
patch

fixed review comments
Attachment #313501 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
Attachment #313803 - Flags: review?(mano)
Comment on attachment 313803 [details] [diff] [review]
patch

I'm just curious, why do we disable this button rather than hiding it? This is supposed to work like a context menu IMO.

Updated

10 years ago
Whiteboard: [strings landed][needs review mano] → [strings landed][has patch][needs review mano]
(Assignee)

Comment 32

10 years ago
maybe a user could blind-click the space taken by a button, knowing its position... we disable options in context menus for the same kind of pointed node, instead of hiding them...
Comment on attachment 313803 [details] [diff] [review]
patch

r=mano, but pinstripe my need futher tweaks for disabled appearance (use graytext for the label, we might just do that already).
Attachment #313803 - Flags: review?(mano) → review+
(Assignee)

Comment 34

10 years ago
disabled appearance should be already set up by default on a toolbarbutton on pinstripe, still i don't have a Mac to tweak that further and to be sure of that... 
So please, if after this check-in something is wrong in the disabled state on pinstripe, file a followup blocking this.
Keywords: checkin-needed
Whiteboard: [strings landed][has patch][needs review mano] → [strings landed][has patch][has review]
Checking in browser/components/places/content/places.js;
/cvsroot/mozilla/browser/components/places/content/places.js,v  <--  places.js
new revision: 1.153; previous revision: 1.152
done
Checking in browser/components/places/content/places.xul;
/cvsroot/mozilla/browser/components/places/content/places.xul,v  <--  places.xul
new revision: 1.128; previous revision: 1.127
done
Checking in browser/themes/pinstripe/browser/places/organizer.css;
/cvsroot/mozilla/browser/themes/pinstripe/browser/places/organizer.css,v  <--  organizer.css
new revision: 1.9; previous revision: 1.8
done
Checking in browser/themes/winstripe/browser/places/organizer.css;
/cvsroot/mozilla/browser/themes/winstripe/browser/places/organizer.css,v  <--  organizer.css
new revision: 1.7; previous revision: 1.6
done
Checking in toolkit/components/places/src/utils.js;
/cvsroot/mozilla/toolkit/components/places/src/utils.js,v  <--  utils.js
new revision: 1.13; previous revision: 1.12
done
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
I tested the fix with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9pre) Gecko Minefield/3.0pre.

Seems to work fine. But one question I have: Should we use "Search Tags" as emtpy text for the search field if the user selects the Tag container inside the left pane?
(Assignee)

Comment 37

10 years ago
no, not for now at least, searching into tags is quite complex and will become more complex when we will change how tag works... so for now we should search in all bookmarks when Tags is selected
(Assignee)

Comment 38

10 years ago
PS: however file a followup on search in Tags, for future reference
(Assignee)

Updated

10 years ago
Duplicate of this bug: 407416
verified with: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9pre) Gecko/2008050704 Minefield/3.0pre
Status: RESOLVED → VERIFIED
(In reply to comment #37)
> more complex when we will change how tag works... so for now we should search
> in all bookmarks when Tags is selected

That doesn't work at the moment. I've filed bug 432778 therefor.

(In reply to comment #38)
> PS: however file a followup on search in Tags, for future reference

This is bug 432779 now.
Flags: in-litmus?
(Assignee)

Updated

10 years ago
Depends on: 432778
Test cases were created for 3.1 and 3.0 test runs on litmus for regression testing.

For 3.0,
https://litmus.mozilla.org/show_test.cgi?id=7528

For 3.1,
https://litmus.mozilla.org/show_test.cgi?id=7529
Flags: in-litmus? → in-litmus+
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.