The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Firefox 25

Status

()

Firefox
Developer Tools
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: fitzgen, Assigned: bgrins)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 25
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [landed-in-fxteam])

Attachments

(1 attachment, 3 obsolete attachments)

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)

Updated

4 years ago
Assignee: nobody → bgrinstead
(Assignee)

Comment 2

4 years ago
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.
(Assignee)

Comment 3

4 years ago
Created attachment 783819 [details] [diff] [review]
bug-892168-2.patch

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+
(Assignee)

Comment 5

4 years ago
Created attachment 783850 [details] [diff] [review]
bug-892168-3.patch

Fixes issues with patch #2
Attachment #783819 - Attachment is obsolete: true
Attachment #783850 - Flags: review?(jwalker)
(Assignee)

Comment 6

4 years ago
> ::: 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.

Updated

4 years ago
Status: NEW → ASSIGNED

Comment 8

4 years ago
So is that an r+?
Attachment #783850 - Flags: review?(jwalker) → review+

Comment 9

4 years ago
Some inspector tests fail with this change, presumably due to the highlighter now being locked after opening.
(Assignee)

Comment 10

4 years ago
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
Attachment #783850 - Attachment is obsolete: true
Attachment #785146 - Flags: review?(jwalker)
Attachment #785146 - Flags: review?(jwalker) → review+
(Assignee)

Comment 11

4 years ago
The try build for this, Bug 891556 and Bug 878412 is here: https://tbpl.mozilla.org/?tree=Try&rev=88d88a70f972&pusher=bgrinstead@mozilla.com
Flags: needinfo?(jwalker)
https://tbpl.mozilla.org/?tree=Fx-Team&rev=79edca22de3f
https://hg.mozilla.org/integration/fx-team/rev/8780f778bc45
Flags: needinfo?(jwalker)
Whiteboard: [landed-in-fxteam]
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
(Assignee)

Comment 14

4 years ago
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.

Comment 16

4 years ago
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
Last Resolved: 4 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.
(Assignee)

Comment 19

4 years ago
(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.
You need to log in before you can comment on or make changes to this bug.