Bug 149198 - Web Inspector: Make Find / Save keyboard shortcuts work better globally
Summary: Web Inspector: Make Find / Save keyboard shortcuts work better globally
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords: InRadar
Depends on:
Blocks: 142533 149275
  Show dependency treegraph
 
Reported: 2015-09-15 17:12 PDT by Joseph Pecoraro
Modified: 2015-09-17 12:29 PDT (History)
8 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (15.44 KB, patch)
2015-09-15 17:23 PDT, Joseph Pecoraro
bburg: review-
bburg: commit-queue-
Details | Formatted Diff | Diff
[PATCH] Proposed Fix (18.32 KB, patch)
2015-09-16 17:40 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2015-09-15 17:12:04 PDT
* SUMMARY
Make Find / Save keyboard shortcuts work better globally.

Currently find/save requires you to have the ContentView in the ContentBrowser focused. This fails if you have focus on the Sidebars. We should consider making these global, like Copy.

* STEPS TO REPRODUCE
1. Inspect <https://webkit.org>
2. Show Resources Tab
3. Select a CSS resource
4. Cmd + F
  => expected find banner to show for the editor, instead Safari may have shown a find banner if docked, nothing happens if not docked
5. Cmd + S
  => expected resource save dialog, if docked Safari handles the key event, nothing happens if not docked
Comment 1 Joseph Pecoraro 2015-09-15 17:12:40 PDT
<rdar://problem/22708645>
Comment 2 Joseph Pecoraro 2015-09-15 17:23:29 PDT
Created attachment 261268 [details]
[PATCH] Proposed Fix
Comment 3 BJ Burg 2015-09-16 10:46:30 PDT
Comment on attachment 261268 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=261268&action=review

I like this patch and it works a lot better than existing behavior, but r- because save and find don't do anything if QuickConsole has focus. In fact, the save shortcuts do nothing if any text field has focus, in my testing. I'm fine with the search shortcut doing nothing if a search box is already selected (so it doesn't switch between LogContentView and the split ContentView).

Focus management seems ripe for extracting into a FocusController class, someday. We might want to do more things with it.

> Source/WebInspectorUI/ChangeLog:7
> +

Please copy the problem/fix description from the bug into here.

> Source/WebInspectorUI/ChangeLog:28
> +        If nothing is focused, default ot the main content browser.

Nit: typo

> Source/WebInspectorUI/UserInterface/Base/Main.js:1689
> +    if (this.splitContentBrowser.element.isSelfOrAncestor(this.currentFocusElement) || this.quickConsole.element.isSelfOrAncestor(this.currentFocusElement))

This seems wrong: if quickConsole has focus but the split content browser is not open, then it won't work.

> Source/WebInspectorUI/UserInterface/Base/Main.js:1701
> +    var tabContentView = this.tabBrowser.selectedTabContentView;

Nit: let
Comment 4 BJ Burg 2015-09-16 10:59:08 PDT
While testing this out, I also filed/updated the following bugs:

* https://bugs.webkit.org/show_bug.cgi?id=142533
* https://bugs.webkit.org/show_bug.cgi?id=149223
Comment 5 Devin Rousso 2015-09-16 11:30:42 PDT
Comment on attachment 261268 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=261268&action=review

> Source/WebInspectorUI/UserInterface/Base/Main.js:1761
> +    if (!contentView || !contentView.supportsSave)

NIT: From what I can see, any class with supportsSave also has a saveDataToFile function, but would it be safer to also check to see that that function exists?

You have also used "var" instead of "let" in multiple places.
Comment 6 Joseph Pecoraro 2015-09-16 15:34:12 PDT
(In reply to comment #5)
> Comment on attachment 261268 [details]
> [PATCH] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=261268&action=review
> 
> > Source/WebInspectorUI/UserInterface/Base/Main.js:1761
> > +    if (!contentView || !contentView.supportsSave)
> 
> NIT: From what I can see, any class with supportsSave also has a
> saveDataToFile function, but would it be safer to also check to see that
> that function exists?

Some ContentViews may have the ability to save but don't want to save at the moment. The LogContentView when not visible or in the split console is an example.
Comment 7 Joseph Pecoraro 2015-09-16 17:40:31 PDT
Created attachment 261342 [details]
[PATCH] Proposed Fix

Change this to generically handle Cmd+F / Cmd+S in the QuickConsole when split console is showing/closed.

  - QuickConsole focused and split console CLOSED => Forward events to the main ContentBrowser/ContentView
  - QuickConsole focused and split console OPEN   => Forward events to the split ContentBrowser/ContentView
Comment 8 BJ Burg 2015-09-17 11:40:46 PDT
Comment on attachment 261342 [details]
[PATCH] Proposed Fix

r=me, but see the blocked bug (which seems like separate work)
Comment 9 WebKit Commit Bot 2015-09-17 12:29:53 PDT
Comment on attachment 261342 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 261342

Committed r189924: <http://trac.webkit.org/changeset/189924>
Comment 10 WebKit Commit Bot 2015-09-17 12:29:57 PDT
All reviewed patches have been landed.  Closing bug.