Bug 176433 – Web Inspector: ⌘E and ⌘G do not work in main content area when quick console drawer is open

• Component: Web Inspector
• Status: RESOLVED FIXED

[Joseph Pecoraro]

⌘E and ⌘G do not work in main content area when quick console drawer is open

Steps to reproduce:
1. Inspect webkit.org
2. Show the main resource's content in the Resources tab
3. Expand the quick console
4. Double click to select "stylesheet" in the main resource content area
5. ⌘E and ⌘G
  => Expected search forward, nothing happened

Notes:
- Both the Main ContentBrowser and the QuickConsole's ContentBrowser have a FindBanner
- Both FindBanners have "enabled" their ⌘E/⌘G handling
- The QuickConsole's happens first
- KeyboardShortcut's global handler runs only 1 handler and bails

[Joseph Pecoraro]

Possible solutions:
1. KeyboardShortcuts should specify if they have handled / if they stop propagation
2. Move away from multiple KeyboardShortcuts and behave like ⌘K where it is a global entry point and the current / active content view attempts to handle the operation.

I like (2) better, since it avoid any conflict. The user has a keyboard shortcut, and they expect it to apply to the active area.

[Joseph Pecoraro]

Created attachment 319963 [details]
[PATCH] Proposed Fix

[Blaze Burg]

This is a great example of something that is not possible to test given our current test harness setup.

[Blaze Burg]

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

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

r=me, Thank you Joe!

> Source/WebInspectorUI/ChangeLog:45
> +        are ContentBrowsers 

Nit: missing end of sentence

> Source/WebInspectorUI/UserInterface/Base/Main.js:291
> +    // FIXME: FindBanner search queries should be shared among search fields and be common with the system.

Nit: you are looking for bugs 151310 and 113588.

> Source/WebInspectorUI/UserInterface/Views/ContentView.js:452
> +        let selection = window.getSelection();

I guess this is better than nothing. We need to do a better job overriding this in more ContentView subclasses.

> Source/WebInspectorUI/UserInterface/Views/FindBanner.js:-227
> -    {

I didn't realize we were doing this, its kinda gross. If we really were going to do this in a transparent way that each content view handles it by itself, then we'd need some notion of a first responder and responder chain. I think the solution in this patch is fine, though.

[Joseph Pecoraro]

Created attachment 320046 [details]
[PATCH] For Landing

[WebKit Commit Bot]

Comment on attachment 320046 [details]
[PATCH] For Landing

Clearing flags on attachment: 320046

Committed r221691: <http://trac.webkit.org/changeset/221691>

[Radar WebKit Bug Importer]

<rdar://problem/34693554>