Summary: | text search (find) keybindings should work in the Web Inspector | ||
---|---|---|---|
Product: | WebKit | Reporter: | Rachael Worthington (cheers) <rachael> |
Component: | Web Inspector (Deprecated) | Assignee: | Nobody <webkit-unassigned> |
Status: | RESOLVED FIXED | ||
Severity: | Normal | CC: | aroben, jalkut, mitz, rik |
Priority: | P2 | Keywords: | InRadar |
Version: | 528+ (Nightly build) | ||
Hardware: | All | ||
OS: | All | ||
Attachments: |
Description
Rachael Worthington (cheers)
2007-12-05 15:13:19 PST
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.
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. 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.
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 on attachment 22500 [details]
Same as original but endeavors to avoid false-positive keystrokes...
Obsoleted by later patch.
Created attachment 22506 [details]
Add credit/blame line in case there are questions or requests for fixes from review.
Created attachment 22507 [details]
Ugh - learn to spell my email addr.
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?
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 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.
Landed in r35785. |