Summary: | Web Inspector: Make Find / Save keyboard shortcuts work better globally | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Joseph Pecoraro <joepeck> | ||||||
Component: | Web Inspector | Assignee: | Joseph Pecoraro <joepeck> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | bburg, commit-queue, graouts, joepeck, mattbaker, nvasilyev, timothy, webkit-bug-importer | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 142533, 149275 | ||||||||
Attachments: |
|
Description
Joseph Pecoraro
2015-09-15 17:12:04 PDT
Created attachment 261268 [details]
[PATCH] Proposed Fix
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 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 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. (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. 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 on attachment 261342 [details]
[PATCH] Proposed Fix
r=me, but see the blocked bug (which seems like separate work)
Comment on attachment 261342 [details] [PATCH] Proposed Fix Clearing flags on attachment: 261342 Committed r189924: <http://trac.webkit.org/changeset/189924> All reviewed patches have been landed. Closing bug. |