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

Status: VERIFIED FIXED

(DevTools :: Console, defect, P1)

Description

[Dirkjan Ochtman (:djc)]

In 20.0a2 (2013-01-17).

Comment 1

[Mihai Sucan [:msucan]]

Sounds like a toolbox issue.

Comment 2

[Paul Rouget [:paul]]

@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)

Comment 3

[Dirkjan Ochtman (:djc)]

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.

Comment 4

[Paul Rouget [:paul]]

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.

Comment 5

[Paul Rouget [:paul]]

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?

Comment 6

[Neil Deakin]

You need to remove the <keyset> and reinsert it into the document.

Comment 7

[Paul Rouget [:paul]]

Attachment: v1

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.

Comment 8

[Paul Rouget [:paul]]

Attachment: v1.1

typo

Comment 9

[Paul Rouget [:paul]]

https://tbpl.mozilla.org/?tree=Try&rev=1ad733830171

Comment 10

[Joe Walker [: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);
}

Comment 11

[Paul Rouget [:paul]]

Attachment: v1.2

addressed Joe's comment.

Comment 12

[Paul Rouget [:paul]]

We need that in Firefox 20.

Comment 13

[Paul Rouget [:paul]]

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

Comment 14

[Victor Porof [:vporof][:vp]]

https://hg.mozilla.org/integration/fx-team/rev/8d85fac98a8c

Comment 15

[Panos Astithas (he/him) [:past] (please ni?)]

https://hg.mozilla.org/mozilla-central/rev/8d85fac98a8c

Comment 16

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ca21e77bb5e7

Comment 18

[Mihai Morar, (:MihaiMorar)]

Verified as fixed on FF 20.b1:

Mozilla/5.0 (Windows NT 6.1; rv:20.0) Gecko/20100101 Firefox/20.0

Comment 19

[Mihai Morar, (:MihaiMorar)]

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)