Bug 16313 - text search (find) keybindings should work in the Web Inspector
Summary: text search (find) keybindings should work in the Web Inspector
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2007-12-05 15:13 PST by Rachael Worthington (cheers)
Modified: 2008-08-15 10:03 PDT (History)
4 users (show)

See Also:


Attachments
Patch for Cmd-F or Ctrl-F (1.42 KB, patch)
2008-07-22 01:03 PDT, Anthony Ricaud
no flags Details | Formatted Diff | Diff
Same as original but endeavors to avoid false-positive keystrokes... (1.78 KB, patch)
2008-07-27 00:44 PDT, Daniel Jalkut
no flags Details | Formatted Diff | Diff
Same as original + avoid false positives + style edits via bdash (1.73 KB, patch)
2008-07-27 01:43 PDT, Daniel Jalkut
no flags Details | Formatted Diff | Diff
Add credit/blame line in case there are questions or requests for fixes from review. (1.85 KB, patch)
2008-07-27 10:39 PDT, Daniel Jalkut
no flags Details | Formatted Diff | Diff
Ugh - learn to spell my email addr. (1.85 KB, patch)
2008-07-27 10:43 PDT, Daniel Jalkut
timothy: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rachael Worthington (cheers) 2007-12-05 15:13:19 PST
Current behavior:
hitting cmd-F gives an error bonk, cmd-G does nothing

desired behavior:
hitting cmd-F should either bring up a find panel, for searching just the file you're focused on, or at least  move your cursor to the find field in the upper right so that you can enter search terms.
cmd-G and cmd-shift-G should cycle forwards and backwards through search results like it does elsewhere.

(This is something of a pet peeve of mine)
Comment 1 Adam Roben (:aroben) 2008-01-29 11:07:18 PST
<rdar://problem/5712878>
Comment 2 Anthony Ricaud 2008-07-22 01:03:34 PDT
Created attachment 22419 [details]
Patch for Cmd-F or Ctrl-F

I'm not sure the early break; is a good idea for code readability.

I'll post a patch for Cmd+G and Cmd-Shift-G later.
Comment 3 Daniel Jalkut 2008-07-26 23:04:59 PDT
I stumbled on this bug because it was referenced in IRC. The only skepticism that jumps to my mind is whether the "match" is too inclusive.  For instance, this patch is intended to allow ctrl-F and cmd-F to perform a certain action, but the code as written will (I think) allow many key combinations to perform this action:

cmd-ctrl-F
cmd-shift-F
ctrl-shift-F
opt-ctrl-F

Etc.

I don't know enough about the code flow around this to know whether this matters. But generally when keyboard shortcut modifiers are being explicitly tested, it's important to confirm that the desired modifier is being pressed TO THE EXCLUSION of the other modifier keys.

Comment 4 Daniel Jalkut 2008-07-27 00:44:25 PDT
Created attachment 22500 [details]
Same as original but endeavors to avoid false-positive keystrokes...

I confirmed my suspicion that the original patch made it possible to invoke the find command by a number of undesired keystrokes. With this adjusted patch, the keystroke only invokes find if it's a pure cmd-f or ctrl-f.
Comment 5 Daniel Jalkut 2008-07-27 01:43:31 PDT
Created attachment 22502 [details]
Same as original + avoid false positives + style edits via bdash

Fixes style mistakes I made. Props to bdash for noticing/informing.
Comment 6 Daniel Jalkut 2008-07-27 01:44:56 PDT
Comment on attachment 22500 [details]
Same as original but endeavors to avoid false-positive keystrokes...

Obsoleted by later patch.
Comment 7 Daniel Jalkut 2008-07-27 10:39:45 PDT
Created attachment 22506 [details]
Add credit/blame line in case there are questions or requests for fixes from review.
Comment 8 Daniel Jalkut 2008-07-27 10:43:42 PDT
Created attachment 22507 [details]
Ugh - learn to spell my email addr.
Comment 9 mitz@webkit.org 2008-07-27 10:49:02 PDT
Comment on attachment 22506 [details]
Add credit/blame line in case there are questions or requests for fixes from review.

Is it okay to hard-code 'F' in inspector.js? How will it work with non-US keyboard layouts or with localizations where the keyboard shortcut for Find is not Command-F?
Comment 10 Daniel Jalkut 2008-07-27 10:53:06 PDT
Perhaps a better long-term solution is to arrange for the Inspector to react to the existing menu items of the app. First-responder style.  For instance, it's kind of dumb that "Cmd-F" will now work to activate the find field, yet Edit -> Find is disabled in the menu bar.

This also touches on another issue that a friend noticed, where he wanted a "printable" version of the inspector window.  If "Print" from the hosting app's menu was hooked up to the inspector, it might "just work" for some definition of working.

Daniel

Comment 11 Timothy Hatcher 2008-08-01 15:20:22 PDT
Comment on attachment 22507 [details]
Ugh - learn to spell my email addr.

We should find a way to hook up to the menu, for sure. But I think this is fine for now. We might also have different find in the future, like a current file find (Command-F and Command-G). Then the global search, so the shortcut might change to mean local find in the future.
Comment 12 Timothy Hatcher 2008-08-15 10:03:10 PDT
Landed in r35785.