892168 - Ctrl+Shift+I/Cmd+Opt+I should toggle the devtools open/closed

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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]]

F12 is here: bug 878412

Comment 2

[Brian Grinstead [:bgrins]]

Attachment: 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]]

Attachment: bug-892168-2.patch

Removed inspector panel keybinding for now, will be handled by Bug 891556

Comment 4

[Joe Walker [:jwalker]]

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]]

Attachment: bug-892168-3.patch

Fixes issues with patch #2

Comment 6

[Brian Grinstead [:bgrins]]

> ::: 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]]

(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)]

So is that an r+?

Comment 9

[Dave Camp (:dcamp)]

Some inspector tests fail with this change, presumably due to the highlighter now being locked after opening.

Comment 10

[Brian Grinstead [:bgrins]]

Attachment: bug-892168-4.patch

highlighter.unlock is now handled by https://bugzilla.mozilla.org/show_bug.cgi?id=891556

Comment 11

[Brian Grinstead [:bgrins]]

The try build for this, Bug 891556 and Bug 878412 is here: https://tbpl.mozilla.org/?tree=Try&rev=88d88a70f972&pusher=[e-mail]

Comment 12

[Joe Walker [:jwalker]]

https://tbpl.mozilla.org/?tree=Fx-Team&rev=79edca22de3f
https://hg.mozilla.org/integration/fx-team/rev/8780f778bc45

Comment 13

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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]]

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=[e-mail].  What should I do to resolve this?

Comment 15

[Joe Walker [:jwalker]]

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

[Dave Camp (:dcamp)]

https://hg.mozilla.org/integration/fx-team/rev/74dd77fd840c

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/74dd77fd840c

Comment 18

[Dão Gottwald [:dao]]

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]]

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