Last Comment Bug 892168 - Ctrl+Shift+I/Cmd+Opt+I should toggle the devtools open/closed
: Ctrl+Shift+I/Cmd+Opt+I should toggle the devtools open/closed
Status: RESOLVED FIXED
[landed-in-fxteam]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: Firefox 25
Assigned To: Brian Grinstead [:bgrins]
:
Mentors:
Depends on:
Blocks: devtools-shortcuts
  Show dependency treegraph
 
Reported: 2013-07-10 15:30 PDT by Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7]
Modified: 2013-08-07 14:10 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
bug-892168-1.patch (4.29 KB, patch)
2013-07-31 09:01 PDT, Brian Grinstead [:bgrins]
no flags Details | Diff | Splinter Review
bug-892168-2.patch (6.67 KB, patch)
2013-07-31 09:40 PDT, Brian Grinstead [:bgrins]
jwalker: review+
Details | Diff | Splinter Review
bug-892168-3.patch (5.92 KB, patch)
2013-07-31 10:43 PDT, Brian Grinstead [:bgrins]
jwalker: review+
Details | Diff | Splinter Review
bug-892168-4.patch (5.18 KB, patch)
2013-08-02 12:44 PDT, Brian Grinstead [:bgrins]
jwalker: review+
Details | Diff | Splinter Review

Description Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2013-07-10 15:30:45 PDT
Should open to the panel that was last used.

Should not activate the highlighter.

Should not matter the state of the devtools, whether the highlighter is active, which panel is active; should always just toggle.
Comment 1 Heather Arthur [:harth] 2013-07-10 16:16:12 PDT
F12 is here: bug 878412
Comment 2 Brian Grinstead [:bgrins] 2013-07-31 09:01:32 PDT
Created attachment 783801 [details] [diff] [review]
bug-892168-1.patch

This patch still has two things that need to be done (I am still working through figuring these out).  I have based the changes off of what it appears that other menu items are doing.

* Show the label Ctrl+Shift+I / Cmd+Shift+I next to the "Toggle Tools" menu item
* Do not automatically start highlighting node if previous panel was the inspector.
Comment 3 Brian Grinstead [:bgrins] 2013-07-31 09:40:31 PDT
Created attachment 783819 [details] [diff] [review]
bug-892168-2.patch

Removed inspector panel keybinding for now, will be handled by Bug 891556
Comment 4 Joe Walker [:jwalker] (needinfo me or ping on irc) 2013-07-31 10:30:28 PDT
Comment on attachment 783819 [details] [diff] [review]
bug-892168-2.patch

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

::: browser/base/content/browser-sets.inc
@@ +275,5 @@
>  #endif
>      />
> +
> +    <key id="key_devToolboxMenuItem" keycode="&devToolboxMenuItem.keycode;" 
> +         keytext="&devToolboxMenuItem.keytext;" command="Tools:DevToolbox" key="&devToolboxMenuItem.keytext;"       

This is a nit, but we should avoid adding trailing spaces.

::: browser/devtools/inspector/inspector-panel.js
@@ -58,5 @@
>    },
>  
>    _deferredOpen: function(defaultSelection) {
>      let deferred = promise.defer();
> -

Was removing this line deliberate?

::: browser/devtools/main.js
@@ -86,5 @@
>  
>  Tools.inspector = {
>    id: "inspector",
>    accesskey: l10n("inspector.accesskey", inspectorStrings),
> -  key: l10n("inspector.commandkey", inspectorStrings),

We should probably remove the entry from inspector.properties, right?

::: browser/locales/en-US/chrome/browser/browser.dtd
@@ +252,5 @@
>  <!ENTITY devToolbar.keytext                "F2">
>  <!ENTITY devToolboxMenuItem.label          "Toggle Tools">
>  <!ENTITY devToolboxMenuItem.accesskey      "T">
> +<!ENTITY devToolboxMenuItem.keycode          "I">
> +<!ENTITY devToolboxMenuItem.keytext          "I">

Another nit: please could you line these things up?
Comment 5 Brian Grinstead [:bgrins] 2013-07-31 10:43:47 PDT
Created attachment 783850 [details] [diff] [review]
bug-892168-3.patch

Fixes issues with patch #2
Comment 6 Brian Grinstead [:bgrins] 2013-07-31 10:45:10 PDT
> ::: browser/devtools/main.js
> @@ -86,5 @@
> >  
> >  Tools.inspector = {
> >    id: "inspector",
> >    accesskey: l10n("inspector.accesskey", inspectorStrings),
> > -  key: l10n("inspector.commandkey", inspectorStrings),
> 
> We should probably remove the entry from inspector.properties, right?
> 

I left it in since this will be immediately addressed by Bug 891556 (will be changing the character from I to C).
Comment 7 Joe Walker [:jwalker] (needinfo me or ping on irc) 2013-07-31 10:59:37 PDT
(In reply to Brian Grinstead [:bgrins] from comment #6)
> > ::: browser/devtools/main.js
> > @@ -86,5 @@
> > >  
> > >  Tools.inspector = {
> > >    id: "inspector",
> > >    accesskey: l10n("inspector.accesskey", inspectorStrings),
> > > -  key: l10n("inspector.commandkey", inspectorStrings),
> > 
> > We should probably remove the entry from inspector.properties, right?
> > 
> 
> I left it in since this will be immediately addressed by Bug 891556 (will be
> changing the character from I to C).

Yes, good point.
Comment 8 Dave Camp (:dcamp) 2013-07-31 16:23:51 PDT
So is that an r+?
Comment 9 Dave Camp (:dcamp) 2013-08-02 11:29:19 PDT
Some inspector tests fail with this change, presumably due to the highlighter now being locked after opening.
Comment 10 Brian Grinstead [:bgrins] 2013-08-02 12:44:29 PDT
Created attachment 785146 [details] [diff] [review]
bug-892168-4.patch

highlighter.unlock is now handled by https://bugzilla.mozilla.org/show_bug.cgi?id=891556
Comment 11 Brian Grinstead [:bgrins] 2013-08-03 05:27:55 PDT
The try build for this, Bug 891556 and Bug 878412 is here: https://tbpl.mozilla.org/?tree=Try&rev=88d88a70f972&pusher=bgrinstead@mozilla.com
Comment 13 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2013-08-03 11:32:23 PDT
Backed out for causing style editor test failures and "key_devtToolbar should be unique". Apparently the style editor thing is known, but I couldn't find it on tbpl. There is a duplicate key added in the patch, probably why the "unique" message is flaring up.

https://hg.mozilla.org/integration/fx-team/rev/5ef09fce3d1b

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_duplicateIDs.js | key_devToolbar should be unique
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/framework/test/browser_keybindings.js | uncaught exception - TypeError: keysetMap.inspector is undefined at chrome://mochitests/content/browser/browser/devtools/framework/test/browser_keybindings.js:63
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/framework/test/browser_keybindings.js | Test timed out
ReferenceError: is is not defined: inspectorShouldBeOpenAndHighlighting@chrome://mochitests/content/browser/browser/devtools/framework/test/browser_keybindings.js:68
TypeError: profile is undefined: Sidebar.prototype<.getItemByProfile@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/profiler/sidebar.js:91
TypeError: this.selectedEditor.sourceEditor is null: @resource:///modules/devtools/StyleEditorUI.jsm:154
TypeError: this.selectedEditor.sourceEditor is null: @resource:///modules/devtools/StyleEditorUI.jsm:154
TypeError: this.selectedEditor.sourceEditor is null: @resource:///modules/devtools/StyleEditorUI.jsm:154
TypeError: this.selectedEditor.sourceEditor is null: @resource:///modules/devtools/StyleEditorUI.jsm:154
TypeError: this.selectedEditor.sourceEditor is null: @resource:///modules/devtools/StyleEditorUI.jsm:154
TypeError: this.selectedEditor.sourceEditor is null: @resource:///modules/devtools/StyleEditorUI.jsm:154
TypeError: this.selectedEditor.sourceEditor is null: @resource:///modules/devtools/StyleEditorUI.jsm:154
TypeError: this.selectedEditor.sourceEditor is null: @resource:///modules/devtools/StyleEditorUI.jsm:154
Comment 14 Brian Grinstead [:bgrins] 2013-08-03 17:22:31 PDT
So, Joe and I went through these issues yesterday and got a running build by fixing a couple of the issues (one was unique to this patch - the key_devToolbar thing) and one was a conflict between this and the F12 patch.  I believe the patch that got landed was the attachment before this fix.  Sorry for any confusion - I'm not sure if I should have reuploaded the patch for just this case, or combined all three into one file to land, or something else altogether.  The working build, which includes all three patches is here: https://tbpl.mozilla.org/?tree=Try&rev=88d88a70f972&pusher=bgrinstead@mozilla.com.  What should I do to resolve this?
Comment 15 Joe Walker [:jwalker] (needinfo me or ping on irc) 2013-08-04 05:20:05 PDT
(In reply to Brian Grinstead [:bgrins] from comment #14)
> What should I do to resolve this?

I suggest getting a more competent manager, sorry! :(

I'll re-land in a bit. I didn't sleep on the way home, so I don't want to re-land now for fear that I'll accidentally break the Internet.
Comment 17 Tim Taubert [:ttaubert] 2013-08-05 01:06:36 PDT
https://hg.mozilla.org/mozilla-central/rev/74dd77fd840c
Comment 18 Dão Gottwald [:dao] 2013-08-07 13:45:31 PDT
Comment on attachment 785146 [details] [diff] [review]
bug-892168-4.patch

>-  key: l10n("inspector.commandkey", inspectorStrings),
>+  key: null,

Looks like this was the only use of inspector.commandkey, but you didn't remove it from inspector.properties.
Comment 19 Brian Grinstead [:bgrins] 2013-08-07 14:10:27 PDT
(In reply to Dão Gottwald [:dao] from comment #18)
> Comment on attachment 785146 [details] [diff] [review]
> bug-892168-4.patch
> 
> >-  key: l10n("inspector.commandkey", inspectorStrings),
> >+  key: null,
> 
> Looks like this was the only use of inspector.commandkey, but you didn't
> remove it from inspector.properties.

This was immediately taken over with "C" in Bug 891556 https://hg.mozilla.org/mozilla-central/rev/9e0f2aef72e3#l12.12.

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