240760 - cmd-F (find dialog) should focus quicksearch when organizing bookmarks

Status: RESOLVED FIXED

(Camino Graveyard :: Bookmarks, defect)

Description

[Dave Aton]

User-Agent:       Mozilla/5.0 (000000000; 0; 000 000 00 0 000000; 00000; 0000000) 00000000000000 00000000000
Build Identifier: 2004041608 (v0.7+)

Downloaded and installed Camino nightly build. Started up. Opened Bookmarks.
Opened bookmark group Mac Software using triangle, and saw VersionTracker.
Pressed Command-F and entered "version" in dialog box (without quotes), and
clicked Find Next button. Did not display the VersionTracker bookmark.

Reproducible: Always
Steps to Reproduce:
1. See Details section above.
2.
3.

Actual Results:  
No match displayed.

Expected Results:  
Showed match on "VersionTracker" bookmark.

Comment 1

[Greg K.]

Confirmed using Camino/2004040108. Find command operates on the page beneath the
Bookmarks editor, not the Bookmarks themselves.

Comment 2

[Jasper]

Yup we shouldn't allwo suers to do a find in the bookmarks manager.

Comment 3

[Dave Aton]

(In reply to comment #2)
> Yup we shouldn't allwo suers to do a find in the bookmarks manager.

I disagree. Camino should behave like Mozilla. Many users have hundreds of
bookmarks acquired over many years, or bookmarks in different folders that have
something in common. Being able to find and display those in a separate window
is essential. For me, it's a deal-breaker when considering a switch from Mozilla
to Camino.

Comment 4

[Mike Pinkerton (not reading bugmail)]

we should disable find when the manager is present, search is done with the
search field at the bottom of the manager.

Comment 5

[Dave Aton]

(In reply to comment #4)
> we should disable find when the manager is present, search is done with the
> search field at the bottom of the manager.

IMO, a more intuitive would be to make Find open the search field and move the
cursor there, but most would be just to let the user use the Find command the
way s/he probably thinks is normal. (I don't think the average user distingushes
between "Find" and "Search.")

Comment 6

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

*** Bug 258973 has been marked as a duplicate of this bug. ***

Comment 7

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

In bug 258973 comment 4, Simon says: Yeah, I vote for just focussing the
quicksearch field.

Tidying summary, too.

Comment 8

[Simon Fraser [no longer active]]

(In reply to comment #7)
> In bug 258973 comment 4, Simon says: Yeah, I vote for just focussing the
> quicksearch field.

But what do you do if the Find panel is already showing? Should we hide it when
switching to bookmarks, and reshow when we leave?

Comment 9

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

*** Bug 301652 has been marked as a duplicate of this bug. ***

Comment 10

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #8)
> But what do you do if the Find panel is already showing? Should we hide it when
> switching to bookmarks, and reshow when we leave?

At first thought that seems wacky, but there's precedent of sorts with the View
Options inspector in the Finder and the Keyboard Viewer, Character Palette, and
Font panel elsewhere in the OS (they hide when switching to apps that have not
called them and reappear when switching back to the original apps).

Comment 11

[Mike Pinkerton (not reading bugmail)]

this is a good idea, but the code would require a lot of changes to do it right (firstResponder, etc) and we still don't have a good answer for what happens when the find dialog is already present.

Comment 12

[Martin Girschick]

If the find window is replaced by an "in window" search bar  with the same functionality the described issue won't be a problem anymore. See #258211.

Comment 13

[froodian (Ian Leue)]

Attachment: Patch

This patch may be a bit controversial... but I like it.  The find panel is useless in the bm manager anyway.

Comment 14

[froodian (Ian Leue)]

Attachment: Better

This just gets rid of the find dialog when the bookmark manager comes up, not when we call the IBActions for opening it.  It's the Right Way, and works if you type "about:bookmarks" (which the previous patch didn't).

Comment 15

[Stuart Morgan]

Comment on attachment 245228 [details] [diff] [review]
Better

>+  if (mFindDialog != nil)
>+    [mFindDialog close];

The |if| check here doesn't do anything useful.

>-// XXX this is only used to show bm after an import

I think the reason this comment was here is that it's kind of a hack. Exposing internals like this isn't generally a good idea, and it's something I want to clean up in Camino's code, so adding more would make me sad.

Instead, how about having MC call a method in the frontmost BWC (if any) to give it a chance to handle Find if it wants to, which returns a BOOL for whether or not it did.  That gives an extensible way for BWC to do content-specific things with Find without MC having to know the details.

Comment 16

[froodian (Ian Leue)]

Attachment: Patch

Addresses review comments.

Comment 17

[Stuart Morgan]

Comment on attachment 248480 [details] [diff] [review]
Patch

r=me, this is much cleaner, thanks. Please document performFindCommand to explain that the return value is whether or not it was handled though.

Also, we need a follow-up bug or two on making the find panel behavior better:
- if you switch windows or tabs to an existing bookmark manager, it stays showing
- if you toggle on the bookmark manager, causing the find panel to vanish, then toggle it off again, the find panel should come back.

Despite that, I still think this behavior is still an improvement over where we are now, so I'm in favor of landing this now and addressing those issues later.

Comment 18

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 248480 [details] [diff] [review]
Patch

sr=pink

regarding performFindCommand, it seems like it should have a better name, but i can't think of one. Definitely needs to be documented.

Comment 19

[froodian (Ian Leue)]

Checked in on trunk and MOZILLA_1_8_BRANCH.  Filed followup work as a single bug (bug 364255).