Bug 149198 – Web Inspector: Make Find / Save keyboard shortcuts work better globally

• Component: Web Inspector
• Status: RESOLVED FIXED

[Joseph Pecoraro]

* 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

[Joseph Pecoraro]

<rdar://problem/22708645>

[Joseph Pecoraro]

Created attachment 261268 [details]
[PATCH] Proposed Fix

[Blaze Burg]

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

[Blaze Burg]

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

[Devin Rousso]

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.

[Joseph Pecoraro]

(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.

[Joseph Pecoraro]

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

[Blaze Burg]

Comment on attachment 261342 [details]
[PATCH] Proposed Fix

r=me, but see the blocked bug (which seems like separate work)

[WebKit Commit Bot]

Comment on attachment 261342 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 261342

Committed r189924: <http://trac.webkit.org/changeset/189924>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.