Closed Bug 503370 Opened 15 years ago Closed 12 years ago

Library doesn't handle CTRL+F correctly

Categories

(Firefox :: Bookmarks & History, defect)

3.5 Branch
x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 15

People

(Reporter: n4lph4-mozilla, Assigned: johan.charlez)

References

Details

Attachments

(1 file, 4 obsolete files)

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
confirmed
Mozilla/5.0 (Windows; U; Windows NT 6.0; it; rv:1.9.1) Gecko/20090624 Firefox/3.5
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
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.
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.
> 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 :).
OS: Windows XP → All
Version: unspecified → 3.5 Branch
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
Assignee: nobody → johan.charlez
Attached patch patch 0.1 (obsolete) — Splinter Review
Initial patch.

No breakages.
Attachment #618745 - Flags: feedback?(fryn)
Comment on attachment 618745 [details] [diff] [review]
patch 0.1

It looks good to me.
Thanks for writing this. :)
Attachment #618745 - Flags: review?(mak77)
Attachment #618745 - Flags: feedback?(fryn)
Attachment #618745 - Flags: feedback+
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. :)
Attachment #618745 - Flags: review?(mak77)
Attachment #618745 - Flags: review+
Attachment #618745 - Flags: feedback+
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 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.
Attachment #618745 - Flags: review+ → review-
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.
Thank you mak, dao and fryn.

I'll keep the patch simple, limited to just setting the correct scope for each selection.
Attached patch patch 0.2 (obsolete) — Splinter Review
patch 0.2
Attachment #618745 - Attachment is obsolete: true
Attachment #618839 - Flags: feedback?(fryn)
Attachment #618839 - Flags: feedback?(dao)
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?
Attachment #618839 - Flags: feedback?(dao) → feedback-
(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 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.
Attachment #618839 - Flags: feedback?(fryn)
(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.
(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.
(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.
Attached patch patch 0.3 (obsolete) — Splinter Review
Dao, fryn, could you take another look?

I have removed code that should no longer be required.

Still no breakages in '..places/tests/browser'.
Attachment #618839 - Attachment is obsolete: true
Attachment #619674 - Flags: feedback?(fryn)
Attachment #619674 - Flags: feedback?(dao)
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?
(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
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.
Attached patch patch 0.3 + documentation. (obsolete) — Splinter Review
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]);
>     }
Attachment #619674 - Attachment is obsolete: true
Attachment #619743 - Flags: review?(dao)
Attachment #619743 - Flags: feedback?(fryn)
Attachment #619674 - Flags: feedback?(fryn)
Attachment #619674 - Flags: feedback?(dao)
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.
Attachment #619743 - Flags: feedback?(fryn) → feedback+
fixed the commit message
Attachment #619743 - Attachment is obsolete: true
Attachment #620055 - Flags: review?(dao)
Attachment #619743 - Flags: review?(dao)
Comment on attachment 620055 [details] [diff] [review]
patch 0.3 + fixed commit message

Thanks!
Attachment #620055 - Flags: review?(dao) → review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/d69f1f58581a
Flags: in-testsuite-
Keywords: checkin-needed
Target Milestone: --- → Firefox 15
Great. Thanks for changing it.
Better late than never.
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! :-)
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: