Closed Bug 832984 Opened 9 years ago Closed 9 years ago

Ctrl + Shift + K shortcut for the Web Console stopped working (sometimes)

Categories

(DevTools :: Console, defect, P1)

20 Branch
x86_64
Windows 7
defect

Tracking

(firefox20+ verified, firefox21 verified)

VERIFIED FIXED
Firefox 21
Tracking Status
firefox20 + verified
firefox21 --- verified

People

(Reporter: djc, Assigned: paul)

References

Details

Attachments

(1 file, 2 obsolete files)

In 20.0a2 (2013-01-17).
Sounds like a toolbox issue.
@Dirkjan, a couple of questions:
- do you see the web console menu entry in the Web Developer menu?
- if you restart Firefox, does it re-work? (serious question)
Yes, I saw the web console menu entry in the Web Developer menu, and it worked fine.

I'll try restarting Firefox tomorrow, when I'm back at work.

To be sure: this seems to work fine on OS X.
Ok - this might be an important issue. Keybindings might not be attached correctly at startup. I think it happened to me once. We'll need some help to confirm this bug.
Priority: -- → P1
Summary: Ctrl + Shift + K shortcut for the Web Console stopped working → Ctrl + Shift + K shortcut for the Web Console stopped working (sometimes)
Blocks: 830911
In this loop:

> while true
> do
> firefox -P dev --no-remote #Firefox opens 3 windows from restored session
> done

... after ~15 attempts, I managed to run into this situation. 3 windows. In 2 of them, we can't use ctrl-k (or any dynamically registered keybinding). But menuitems are presents.

Looking at the DOM of the non-working windows, the key is in place.

So apparently, the dynamically-added <key> element is not taken into account.

We create the <key> here:
http://mxr.mozilla.org/mozilla-central/source/browser/devtools/framework/gDevTools.jsm#463

Neil, do you see any reason why adding a <key> dynamically would, sometimes, fail?
Flags: needinfo?(enndeakin)
You need to remove the <keyset> and reinsert it into the document.
Flags: needinfo?(enndeakin)
No longer blocks: 830911
Attached patch v1 (obsolete) — Splinter Review
So I don't feel like re-attaching the main keyset. So in this patch, I build a devtools specific keyset that we re-attach if necessary.

This appears to work.
Assignee: nobody → paul
Status: NEW → ASSIGNED
Attachment #704860 - Flags: review?(jwalker)
Attached patch v1.1 (obsolete) — Splinter Review
typo
Attachment #704860 - Attachment is obsolete: true
Attachment #704860 - Flags: review?(jwalker)
Attachment #704861 - Flags: review?(jwalker)
Comment on attachment 704861 [details] [diff] [review]
v1.1

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

I think it would be good to have a function instead of the duplicated code?

function(child) {
  let devtoolsKeyset = doc.getElementById("devtoolsKeyset");
  if (!devtoolsKeyset) {
    devtoolsKeyset = doc.createElement("keyset");
    devtoolsKeyset.setAttribute("id", "devtoolsKeyset");
  }
  devtoolsKeyset.appendChild(child);

  // We have to re-attach the <keyset> for the <key>s to be taken
  // into account.
  let mainKeyset = doc.getElementById("mainKeyset");
  mainKeyset.parentNode.insertBefore(devtoolsKeyset, mainKeyset);
}
Attachment #704861 - Flags: review?(jwalker) → review+
Attached patch v1.2Splinter Review
addressed Joe's comment.
Attachment #704888 - Flags: review+
Blocks: 816946
Whiteboard: [land-in-fx-team]
We need that in Firefox 20.
Attachment #704861 - Attachment is obsolete: true
Comment on attachment 704888 [details] [diff] [review]
v1.2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): new feature toolbox
User impact if declined: pretty bad. Often, all the devtools shortcut are broken
Testing completed (on m-c, etc.): local only
Risk to taking this patch (and alternatives if risky): Low. But Talos hasn't tested this patch yet. I'm not 100% sure we won't have a small perf regression (building one node dynamically at startup).
String or UUID changes made by this patch: no
Attachment #704888 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/integration/fx-team/rev/8d85fac98a8c
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/8d85fac98a8c
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 21
Comment on attachment 704888 [details] [diff] [review]
v1.2

Approving for aurora uplift, assuming that devs will keep an eye on talos once this lands and report any perf issues.
Attachment #704888 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 839890
Verified as fixed on FF 20.b1:

Mozilla/5.0 (Windows NT 6.1; rv:20.0) Gecko/20100101 Firefox/20.0
Verified fix on FF 21b1:

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20100101 Firefox/21.0
(20130401192816)
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:21.0) Gecko/20100101 Firefox/21.0
(20130401192816)
Status: RESOLVED → VERIFIED
No longer blocks: DevToolsPaperCuts
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.