Closed Bug 892168 Opened 6 years ago Closed 6 years ago

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

Categories

(DevTools :: General, defect)

x86
macOS
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 25

People

(Reporter: fitzgen, Assigned: bgrins)

References

(Blocks 1 open bug)

Details

(Whiteboard: [landed-in-fxteam])

Attachments

(1 file, 3 obsolete files)

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.
F12 is here: bug 878412
Summary: F12 and Ctrl+Shift+I/CMd+Opt+I should toggle the devtools open/closed → Ctrl+Shift+I/Cmd+Opt+I should toggle the devtools open/closed
Assignee: nobody → bgrinstead
Attached patch bug-892168-1.patch (obsolete) — Splinter Review
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.
Attached patch bug-892168-2.patch (obsolete) — Splinter Review
Removed inspector panel keybinding for now, will be handled by Bug 891556
Attachment #783801 - Attachment is obsolete: true
Attachment #783819 - Flags: review?(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?
Attachment #783819 - Flags: review?(jwalker) → review+
Attached patch bug-892168-3.patch (obsolete) — Splinter Review
Fixes issues with patch #2
Attachment #783819 - Attachment is obsolete: true
Attachment #783850 - Flags: review?(jwalker)
> ::: 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).
(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.
Status: NEW → ASSIGNED
So is that an r+?
Attachment #783850 - Flags: review?(jwalker) → review+
Some inspector tests fail with this change, presumably due to the highlighter now being locked after opening.
highlighter.unlock is now handled by https://bugzilla.mozilla.org/show_bug.cgi?id=891556
Attachment #783850 - Attachment is obsolete: true
Attachment #785146 - Flags: review?(jwalker)
Attachment #785146 - Flags: review?(jwalker) → review+
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
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?
(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.
https://hg.mozilla.org/integration/fx-team/rev/74dd77fd840c
Whiteboard: [landed-in-fxteam] → [landed-in-fxteam][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/74dd77fd840c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [landed-in-fxteam][fixed-in-fx-team] → [landed-in-fxteam]
Target Milestone: --- → Firefox 25
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.
(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.
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.