Last Comment Bug 503370 - Library doesn't handle CTRL+F correctly
: Library doesn't handle CTRL+F correctly
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Bookmarks & History (show other bugs)
: 3.5 Branch
: x86 All
: -- normal (vote)
: Firefox 15
Assigned To: Johan C
:
:
Mentors:
: 749249 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-07-09 12:42 PDT by NesteaZen
Modified: 2013-11-13 02:19 PST (History)
6 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch 0.1 (1.13 KB, patch)
2012-04-26 11:58 PDT, Johan C
fryn: review-
Details | Diff | Splinter Review
patch 0.2 (4.24 KB, patch)
2012-04-26 16:05 PDT, Johan C
dao+bmo: feedback-
Details | Diff | Splinter Review
patch 0.3 (7.87 KB, patch)
2012-04-30 13:50 PDT, Johan C
no flags Details | Diff | Splinter Review
patch 0.3 + documentation. (8.07 KB, patch)
2012-04-30 16:17 PDT, Johan C
fryn: feedback+
Details | Diff | Splinter Review
patch 0.3 + fixed commit message (8.07 KB, patch)
2012-05-01 14:01 PDT, Johan C
dao+bmo: review+
Details | Diff | Splinter Review

Description NesteaZen 2009-07-09 12:42:43 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1) Gecko/20090624 Firefox/3.5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1) Gecko/20090624 Firefox/3.5

Library doesn't handle CTRL+F correctly.

Reproducible: Always

Steps to Reproduce:
1. CTRL+SHIFT+H
2. CTRL+F
3. populate the search field with text
Actual Results:  
search defaults to the Bookmarks folder

Expected Results:  
the search should be defaulted to the History folder
Comment 1 Marco Bonardo [::mak] 2009-07-09 12:48:20 PDT
confirmed
Mozilla/5.0 (Windows; U; Windows NT 6.0; it; rv:1.9.1) Gecko/20090624 Firefox/3.5
Comment 2 Drew Willcoxon :adw 2009-07-10 20:04:39 PDT
Ctrl+F maps to the OrganizerCommand_find:all command, which boils down to:

http://mxr.mozilla.org/mozilla1.9.1/source/browser/components/places/content/places.js#940

which sets the search scope to bookmarks.
Comment 3 NesteaZen 2009-07-11 01:26:07 PDT
So what does this mean? No bug?

Can't you differentiate between the two selections?
If Bookmarks is selected search that one and vice versa.
Comment 4 Drew Willcoxon :adw 2009-07-11 13:17:04 PDT
Well, we should decide whether Ctrl+F and OrganizerCommand_find:all mean "find in all bookmarks" or "find in all bookmarks if you're viewing bookmarks or all history if you're viewing history."  I agree that the latter makes more sense.  In comment 2 I was just noting where the problem exists in the code.
Comment 5 NesteaZen 2009-07-11 13:43:38 PDT
> I agree that the latter makes more sense.
Yea well XD

> In comment 2 I was just noting where the problem exists in the code.
Oh don't worry. I was just curious and wanted to clarify, but yeah whatever you feel to do.
I'm for the 2nd. It's easier to use the shortcut since you most probably will be hitting H with your right hand and if not then you do everything with your left hand even type in the search term with the left hand and leave the mouse alone until you get the results and it's a lot faster.
Apart from the fact that in Fx 3.0.11 it was that way. So it feels weird 3.5 is behaving differently.

But take your time :).
Comment 6 Gervase Markham [:gerv] 2009-11-26 07:21:26 PST
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
Comment 7 Johan C 2012-04-26 11:00:28 PDT
*** Bug 749249 has been marked as a duplicate of this bug. ***
Comment 8 Johan C 2012-04-26 11:58:43 PDT
Created attachment 618745 [details] [diff] [review]
patch 0.1

Initial patch.

No breakages.
Comment 9 Frank Yan (:fryn) 2012-04-26 12:11:10 PDT
Comment on attachment 618745 [details] [diff] [review]
patch 0.1

It looks good to me.
Thanks for writing this. :)
Comment 10 Frank Yan (:fryn) 2012-04-26 12:38:49 PDT
Comment on attachment 618745 [details] [diff] [review]
patch 0.1

I think my understanding of this patch and the affected code is reasonably good, so r=me!
I'll push this to the fx-team repo later today.

Thanks for the patch, Johan. :)
Comment 11 Dão Gottwald [:dao] 2012-04-26 12:44:58 PDT
Comment on attachment 618745 [details] [diff] [review]
patch 0.1

>+   * Finds across all bookmarks or all history depending on scope.

This comment isn't correct (or your code is wrong, depending on what you really want). this.filterCollection can be something besides "bookmarks" or "history", in which case you're not setting a scope.

>+    switch(this.filterCollection) {

switch (
Comment 12 Frank Yan (:fryn) 2012-04-26 13:15:37 PDT
Comment on attachment 618745 [details] [diff] [review]
patch 0.1

(In reply to Dão Gottwald [:dao] from comment #11)
> Comment on attachment 618745 [details] [diff] [review]
> patch 0.1
> 
> >+   * Finds across all bookmarks or all history depending on scope.
> 
> This comment isn't correct (or your code is wrong, depending on what you
> really want). this.filterCollection can be something besides "bookmarks" or
> "history", in which case you're not setting a scope.

Right. The problem here is that the scope isn't set immediately when the library window opens. AIUI, there are only 3 filterCollection values that are possible at that point: "history", "downloads", and "bookmarks".
I think the code should probably be:

        case "history":
          PlacesQueryBuilder.setScope("history");
          break;
        case "downloads":
          PlacesQueryBuilder.setScope("downloads");
          break;
        default:
          PlacesQueryBuilder.setScope("bookmarks");         
          break;

Also, the label and accesskey entity names need to be updated here:
https://mxr.mozilla.org/mozilla-central/source/browser/components/places/content/places.xul#102
I recommend something like cmd.find.label, etc., though Dão is better at variable naming than I am.
The entity values need to be updated here:
https://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/places/places.dtd#26
I suggest simply "Find".

There are a lot of strange things going on in our UI for this. The search box says "Search History" when the user clicks the History item in the sidebar, but when the user press ctrl/cmd+f, it seems that we intentionally search bookmarks, since the entity name and value currently reflect that. The user is not informed of this, since the placeholder attribute of a textbox currently is not displayed when the textbox has focus. (I'm fixing that in bug 673873.)

Also, we display the "Find in Bookmarks..." menu item in the menu bar on OS X, but we don't display the "Find in Current Collection..." menu item, which makes is completely undiscoverable that ctrl/cmd+shift+f enables the user to search within a collection. Cmd+shift+f will conflict with fullscreen mode, so I think we should remove "find in collection" when we move the library window to a tab.
Comment 13 Marco Bonardo [::mak] 2012-04-26 14:29:10 PDT
Find in collection is intended as a sort of an additional filter, we default to searching all bookmarks and allow to restrict to the current folder.  Honestly I'm not sure it's so much used, we may even decide that it's not worth having it at all.

After discussing briefly with Mano, we may completely remove the scope bar UI stuff, just search in downloads, bookmarks or history based on the left pane selection as we do today.  May be handy at the same time to show "somewhere" the path to the bookmark (separate bug clearly). We are going to lose the save search button, but it's so dumb it's not worth keeping it as it is, there are add-ons doing better work already in this area.
Comment 14 Johan C 2012-04-26 15:32:55 PDT
Thank you mak, dao and fryn.

I'll keep the patch simple, limited to just setting the correct scope for each selection.
Comment 15 Johan C 2012-04-26 16:05:51 PDT
Created attachment 618839 [details] [diff] [review]
patch 0.2

patch 0.2
Comment 16 Dão Gottwald [:dao] 2012-04-27 00:37:19 PDT
Comment on attachment 618839 [details] [diff] [review]
patch 0.2

>   <commandset id="organizerCommandSet">
>     <command id="OrganizerCommand_find:all"
>-             label="&cmd.findInBookmarks.label;"
>-             accesskey="&cmd.findInBookmarks.accesskey;"
>+             label="&cmd.findInLibrary.label;"
>+             accesskey="&cmd.findInLibrary.accesskey;"
>              oncommand="PlacesSearchBox.findAll();"/>

>-<!ENTITY cmd.findInBookmarks.label      "Find in Bookmarks…">
>-<!ENTITY cmd.findInBookmarks.accesskey  "F">
>+<!ENTITY cmd.findInLibrary.label        "Find in Library…">
>+<!ENTITY cmd.findInLibrary.accesskey    "F">

I don't see the point of this label and access key. It's not displayed anywhere, as far as I can tell.
Just remove it?
Comment 17 Frank Yan (:fryn) 2012-04-27 00:56:42 PDT
(In reply to Dão Gottwald [:dao] from comment #16)
> I don't see the point of this label and access key. It's not displayed
> anywhere, as far as I can tell.

It's displayed in the menu bar on OS X.
I have no idea where &cmd.findCurrent.label; is displayed though.
I'm okay with removing both the findInBookmarks and findCurrent labels and accesskeys, since we won't have them when the Library is in a tab anyway.
Comment 18 Frank Yan (:fryn) 2012-04-27 01:01:59 PDT
Comment on attachment 618839 [details] [diff] [review]
patch 0.2

I defer to Dão for what to do here, though I do recommend that we either remove neither or both pairs of entities.
Comment 19 Dão Gottwald [:dao] 2012-04-27 01:13:02 PDT
(In reply to Frank Yan (:fryn) from comment #17)
> (In reply to Dão Gottwald [:dao] from comment #16)
> > I don't see the point of this label and access key. It's not displayed
> > anywhere, as far as I can tell.
> 
> It's displayed in the menu bar on OS X.

So we're overriding menu_find's default label on OS X. Seems like we can stop doing that.
Comment 20 Frank Yan (:fryn) 2012-04-27 01:28:30 PDT
(In reply to Dão Gottwald [:dao] from comment #19)
> (In reply to Frank Yan (:fryn) from comment #17)
> > (In reply to Dão Gottwald [:dao] from comment #16)
> > > I don't see the point of this label and access key. It's not displayed
> > > anywhere, as far as I can tell.
> > 
> > It's displayed in the menu bar on OS X.
> 
> So we're overriding menu_find's default label on OS X. Seems like we can
> stop doing that.

Sounds good to me.
Johan, please remove cmd.findInBookmarks.label, cmd.findInBookmarks.accesskey, cmd.findCurrent.label, and cmd.findInBookmarks.accesskey from the dtd files and their uses in places.js.

I recommend that we also remove placesKey_find:current and findCurrent() while we're at it. They are completely undicoverable and will conflict with the fullscreen shortcut on OS X later.
Comment 21 Dão Gottwald [:dao] 2012-04-27 01:39:18 PDT
(In reply to Frank Yan (:fryn) from comment #20)
> Johan, please remove cmd.findInBookmarks.label,
> cmd.findInBookmarks.accesskey, cmd.findCurrent.label, and
> cmd.findInBookmarks.accesskey from the dtd files and their uses in places.js.

They're not used in places.js, only the command elements are.
Comment 22 Johan C 2012-04-30 13:50:18 PDT
Created attachment 619674 [details] [diff] [review]
patch 0.3

Dao, fryn, could you take another look?

I have removed code that should no longer be required.

Still no breakages in '..places/tests/browser'.
Comment 23 Dão Gottwald [:dao] 2012-04-30 13:56:50 PDT
Comment on attachment 619674 [details] [diff] [review]
patch 0.3

>+    switch (this.filterCollection) {
>+      case "history":
>+        PlacesQueryBuilder.setScope("history");
>+        break;
>+      case "downloads":
>+        PlacesQueryBuilder.setScope("downloads");
>+        break;
>+      default:
>+        PlacesQueryBuilder.setScope("bookmarks");
>+        break;

Not sure why bookmarks should be the default. I imagine history to be more useful for searching.

>   updateCollectionTitle: function PSB_updateCollectionTitle(aTitle) {
>     let title = "";
>     if (aTitle) {
>       title = PlacesUIUtils.getFormattedString("searchCurrentDefault",
>                                                [aTitle]);
>     }

>+++ b/browser/locales/en-US/chrome/browser/places/places.properties
> searchCurrentDefault=Search in '%S'

Is this still useful?
Comment 24 Johan C 2012-04-30 14:01:26 PDT
(In reply to Dão Gottwald [:dao] from comment #23)
> Comment on attachment 619674 [details] [diff] [review]
> patch 0.3
> 
> >   updateCollectionTitle: function PSB_updateCollectionTitle(aTitle) {
> >     let title = "";
> >     if (aTitle) {
> >       title = PlacesUIUtils.getFormattedString("searchCurrentDefault",
> >                                                [aTitle]);
> >     }
> 
> >+++ b/browser/locales/en-US/chrome/browser/places/places.properties
> > searchCurrentDefault=Search in '%S'
> 
> Is this still useful?

I believe it sets the label/text in the search-box.
e.g: Search History, search Bookmarks
Comment 25 Johan C 2012-04-30 14:25:33 PDT
I was mistaken, this bit sets the "folder_scope label" e.g: "Seach in 'folder'".

I've spoken to fryn on I.R.C where he proposed that I file a "follow-up" bug for dealing with the removal of the scopebar.

I'll upload a new patch with some documentation explaining why we're leaving this bit in for now.

This bug fixes the ctrl+f issue and frees up the mac os fullscreen shortcut. Search a folder is still possible.
Comment 26 Johan C 2012-04-30 16:17:26 PDT
Created attachment 619743 [details] [diff] [review]
patch 0.3 + documentation.

Dao: Regarding bookmarks being the default, is bug 469437 relevant?

Added some documentation regarding:
>   updateCollectionTitle: function PSB_updateCollectionTitle(aTitle) {
>     let title = "";
>     if (aTitle) {
>       title = PlacesUIUtils.getFormattedString("searchCurrentDefault",
>                                                [aTitle]);
>     }
Comment 27 Frank Yan (:fryn) 2012-04-30 16:46:44 PDT
Comment on attachment 619743 [details] [diff] [review]
patch 0.3 + documentation.

Review of attachment 619743 [details] [diff] [review]:
-----------------------------------------------------------------

As Johan noted, we should not regress bug 469437.
I haven't done a full analysis of all the possible states of the filterCollection property, but as I understand it, it can be one of four values: "history", "downloads", "bookmarks", and "collection". I'm not sure it's possible for filterCollection to be "collection" when ctrl/cmd+f is pressed, but if it is, we should search in all bookmarks. If not, it doesn't really matter what the "default" case of the switch block is. If this understanding is correct, then the patch's switch block should be fine.
Comment 28 Johan C 2012-05-01 14:01:03 PDT
Created attachment 620055 [details] [diff] [review]
patch 0.3 + fixed commit message

fixed the commit message
Comment 29 Dão Gottwald [:dao] 2012-05-02 10:51:39 PDT
Comment on attachment 620055 [details] [diff] [review]
patch 0.3 + fixed commit message

Thanks!
Comment 30 Ryan VanderMeulen [:RyanVM] 2012-05-03 03:54:37 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/d69f1f58581a
Comment 31 NesteaZen 2012-05-03 20:02:53 PDT
Great. Thanks for changing it.
Better late than never.
Comment 32 Ed Morley [:emorley] 2012-05-04 03:42:42 PDT
NesteaZen, sorry this took so long to get fixed. The change has just merged to mozilla-central so will be in the 2012-05-05 Nightly, or in Firefox 15 when it hits aurora/beta/release (https://wiki.mozilla.org/RapidRelease/Calendar).

https://hg.mozilla.org/mozilla-central/rev/d69f1f58581a

Thank you again for your bug report! :-)

Note You need to log in before you can comment on or make changes to this bug.