Last Comment Bug 891556 - Add Ctrl-Shift-C (Cmd-Opt-C) shortcut to toggle highlighting
: Add Ctrl-Shift-C (Cmd-Opt-C) shortcut to toggle highlighting
Status: RESOLVED FIXED
[landed-in-fxteam]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Inspector (show other bugs)
: unspecified
: All All
: -- normal with 1 vote (vote)
: Firefox 25
Assigned To: Brian Grinstead [:bgrins]
:
Mentors:
: 892180 (view as bug list)
Depends on: 907891
Blocks: devtools-shortcuts 934088
  Show dependency treegraph
 
Reported: 2013-07-09 14:02 PDT by Heather Arthur [:harth]
Modified: 2014-08-31 14:11 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
bug-891556-1.patch (4.85 KB, patch)
2013-07-31 12:18 PDT, Brian Grinstead [:bgrins]
no flags Details | Diff | Splinter Review
bug-891556-2.patch (6.35 KB, patch)
2013-07-31 15:41 PDT, Brian Grinstead [:bgrins]
no flags Details | Diff | Splinter Review
bug-891556-3.patch (6.86 KB, patch)
2013-08-01 12:52 PDT, Brian Grinstead [:bgrins]
no flags Details | Diff | Splinter Review
bug-891556-4.patch (18.51 KB, patch)
2013-08-02 10:46 PDT, Brian Grinstead [:bgrins]
jwalker: review+
Details | Diff | Splinter Review
bug-891556-5.patch (14.60 KB, patch)
2013-08-02 11:30 PDT, Brian Grinstead [:bgrins]
no flags Details | Diff | Splinter Review

Description Heather Arthur [:harth] 2013-07-09 14:02:26 PDT
Currently we don't have a shortcut to toggle the highlighter. We need this. Chrome uses Ctrl-Shift-C (Cmd-Opt-C), and it's available in Mac/Win at least.

If the toolbox is open, this will switch to the Inspector tab and start the highlighter. If the toolbox is closed, it will open the devtools window with the Inspector tab selected and start highlighting. If the inspector tab is already open, this will just start highlighting. And if the highlighter happens to be on, this will stop highlighting.
Comment 1 Heather Arthur [:harth] 2013-07-10 15:36:52 PDT
*** Bug 892180 has been marked as a duplicate of this bug. ***
Comment 2 Brian Grinstead [:bgrins] 2013-07-31 12:18:25 PDT
Created attachment 783905 [details] [diff] [review]
bug-891556-1.patch
Comment 3 Brian Grinstead [:bgrins] 2013-07-31 15:41:32 PDT
Created attachment 784059 [details] [diff] [review]
bug-891556-2.patch

Version that works, except for when devtools is undocked and you are focused on the web
Comment 4 Brian Grinstead [:bgrins] 2013-08-01 12:52:09 PDT
Created attachment 784535 [details] [diff] [review]
bug-891556-3.patch

This patch implements the cmd+alt+c keyboard shortcut for toggling highlighting, and fixes another issue that happened when devtools were undocked *and* focused - keyboard shortcuts would not fire in the same way.  This is because _addKeysToWindow (toolbox.js) was actually running different logic than selectToolCommand (gDevTools.jsm).  Rather than trying to figure out a way to pass gBrowser to selectToolCommand from toolbox.js (which is not possible AFAIK), I implemented a fireCustomKey instance method  on the toolbox.  This way at least some of the logic can be shared between gDevTools and toolbox.  It will take a while to work out how to properly test this, so this patch is still a WIP.
Comment 5 Brian Grinstead [:bgrins] 2013-08-02 10:46:09 PDT
Created attachment 785086 [details] [diff] [review]
bug-891556-4.patch

Adds a basic keybinding test for the panels.  The behavior is such that on inspector, pressing ctrl+shift+c/cmd+alt+c will always switch to inspector tab and toggle highlighter.  For the other panels, they will continue to switch to their tab if not already selected, or close devtools otherwise.  The behavior for other tabs is different when devtools in undocked, since it will never close the window.  For inspector, the behavior is the same when docked and undocked.  Had to work around a situation where the keyboard shortcut for inspector did not work when the devtools window was focused (_addKeysToWindow had to be modified to fix this).
Comment 6 Joe Walker [:jwalker] (needinfo me or ping on irc) 2013-08-02 11:16:13 PDT
Comment on attachment 785086 [details] [diff] [review]
bug-891556-4.patch

Review of attachment 785086 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/framework/gDevTools.jsm
@@ +349,3 @@
>  
>      if (toolbox && toolbox.currentToolId == toolId) {
> +      let customKeyHandled = toolbox.fireCustomKey(toolId);

As discussed, we can probably find a more obvious solution than this, perhaps using a property to indicate how the tool wants to handle toggling.

::: browser/devtools/framework/toolbox.js
@@ +285,5 @@
> +
> +    if (activeToolDefinition.onkey && this.currentToolId === toolId) {
> +        activeToolDefinition.onkey(this.getCurrentPanel());
> +        return true;
> +    }

I know that undefined==false, but I still think it makes sense to 'return false;' here in case anyone assumes a boolean.
Comment 7 Brian Grinstead [:bgrins] 2013-08-02 11:30:47 PDT
Created attachment 785109 [details] [diff] [review]
bug-891556-5.patch

Fixes minor issues from previous patch
Comment 8 Brian Grinstead [:bgrins] 2013-08-02 11:35:37 PDT
Joe,
Can you push this to try?
Comment 10 Tim Taubert [:ttaubert] 2013-08-05 01:08:11 PDT
https://hg.mozilla.org/mozilla-central/rev/9e0f2aef72e3
Comment 11 Sebastian Zartner [:sebo] 2013-11-12 00:00:46 PST
FYI this new shortcut conflicts with Firebug.[1]

> Currently we don't have a shortcut to toggle the highlighter.
Actually you already had Ctrl+Shift+I for it since you introduced the inspector.

Sebastian

[1] http://code.google.com/p/fbug/issues/detail?id=6957
Comment 12 Jan Honza Odvarko [:Honza] PTO 07/23 - 08/08 2013-11-15 05:01:38 PST
Any tips, how Firebug can simply override the default Ctrl+Shift+C shortcut?

Honza

Note You need to log in before you can comment on or make changes to this bug.