Last Comment Bug 747603 - Firefox:Inspector cannot start up with DOMi
: Firefox:Inspector cannot start up with DOMi
Status: RESOLVED FIXED
: dogfood, regression
Product: Firefox
Classification: Client Software
Component: Developer Tools: Inspector (show other bugs)
: Trunk
: All All
: -- normal with 2 votes (vote)
: Firefox 15
Assigned To: Tetsuharu OHZEKI [:tetsuharu] [UTC+9]
:
Mentors:
: 747091 749762 749991 750756 751815 752166 764110 (view as bug list)
Depends on:
Blocks: 735214
  Show dependency treegraph
 
Reported: 2012-04-20 21:45 PDT by Tetsuharu OHZEKI [:tetsuharu] [UTC+9]
Modified: 2012-07-05 15:53 PDT (History)
20 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed


Attachments
proposal (1.82 KB, patch)
2012-04-20 22:13 PDT, Tetsuharu OHZEKI [:tetsuharu] [UTC+9]
no flags Details | Diff | Splinter Review
patch for firefox (4.07 KB, patch)
2012-04-24 09:41 PDT, Tetsuharu OHZEKI [:tetsuharu] [UTC+9]
no flags Details | Diff | Splinter Review
patch for firefox v2 (4.21 KB, patch)
2012-04-25 05:50 PDT, Tetsuharu OHZEKI [:tetsuharu] [UTC+9]
no flags Details | Diff | Splinter Review
patch for firefox(central) v3 (4.44 KB, patch)
2012-04-25 18:59 PDT, Tetsuharu OHZEKI [:tetsuharu] [UTC+9]
dao+bmo: review-
Details | Diff | Splinter Review
patch v1 - Aurora (3.68 KB, patch)
2012-04-30 21:36 PDT, Paul Rouget [:paul]
dao+bmo: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
patch for firefox(central) v4 (4.73 KB, patch)
2012-05-02 10:26 PDT, Tetsuharu OHZEKI [:tetsuharu] [UTC+9]
no flags Details | Diff | Splinter Review
patch for firefox(central) v5 (5.53 KB, patch)
2012-05-02 12:01 PDT, Tetsuharu OHZEKI [:tetsuharu] [UTC+9]
dao+bmo: review-
Details | Diff | Splinter Review
patch for firefox(central) v6 (5.30 KB, patch)
2012-05-02 17:37 PDT, Tetsuharu OHZEKI [:tetsuharu] [UTC+9]
dao+bmo: review+
Details | Diff | Splinter Review

Description Tetsuharu OHZEKI [:tetsuharu] [UTC+9] 2012-04-20 21:45:56 PDT
Firefox:Inspector cannot start up with DOMi.
DOM Inspector breaks the keyset of Firefox:Inspector.

Firefox Environment:
http://hg.mozilla.org/mozilla-central/rev/cd8b66649278

Firefox Error:
----------------------
Error: TypeError: key is null
Source file: resource://app/modules/inspector.jsm
Line: 300
----------------------
Error: ReferenceError: InspectorUI is not defined
Source file: chrome://browser/content/nsContextMenu.js
Line: 401
----------------------
Comment 1 Tetsuharu OHZEKI [:tetsuharu] [UTC+9] 2012-04-20 22:13:02 PDT
Created attachment 617182 [details] [diff] [review]
proposal

This patch removes the shortcut of start up DOMi for Firefox.

Should we remove the shortcut key from Firefox any more?
Comment 2 Dão Gottwald [:dao] 2012-04-24 04:05:44 PDT
buildButtonsTooltip in inspector.jsm could just handle this by returning in case key is null.
Comment 3 Tetsuharu OHZEKI [:tetsuharu] [UTC+9] 2012-04-24 06:59:47 PDT
(In reply to Dão Gottwald [:dao] from comment #2)
> buildButtonsTooltip in inspector.jsm could just handle this by returning in
> case key is null.

I agree it is simple to resolve this bug.
However, at now, the need of DOM inspector is not high. It is rather low. I think that it is not necessarily useful that DOM inspector override the invoking keyset of Firefox default inspector...
Comment 4 Dão Gottwald [:dao] 2012-04-24 07:27:16 PDT
Well, anybody without a need for DOMi can disable or uninstall it...
I use it way more often than the built-in inspector, since the latter doesn't work in chrome land.
Comment 5 Tetsuharu OHZEKI [:tetsuharu] [UTC+9] 2012-04-24 07:56:10 PDT
Firebug is also add-on but it does not override the built-in keyset. I understand this may relate the historical reason.

Firebug provide fully upward compatible functions for the current built-in inspector. Also DOMi provides similar upward compatible functions but they are different from the direction of Firebug/built-in inspector. If Firebug overrides the built-in keyset, DOMi's overriding behavior may proper. But Firebug does not it. So I said that DOMi should not override the keyset of built-in inspector *any more* because I thought the direction of DOMi is not same for Firebug/built-in inspector ones.

Or I propose that we'll implement the preference for switching a behavior to override the default keyset.
Comment 6 Dão Gottwald [:dao] 2012-04-24 08:52:16 PDT
Whatever we do, we shouldn't remove the key without a replacement.

FWIW, it would be appropriate for Firebug to hijack key_inspectPage since it basically implements the same feature.
Comment 7 Tetsuharu OHZEKI [:tetsuharu] [UTC+9] 2012-04-24 09:05:03 PDT
OK.

I'll make a patch for buildButtonsTooltip in inspector.jsm could handle this by returning in case key is null for releasing DOMi 2.0.12.

And I file a bug later for replacing the invoke key for DOMi.

Need I change this bug's "Product" & "Component"?
Comment 8 Tetsuharu OHZEKI [:tetsuharu] [UTC+9] 2012-04-24 09:41:48 PDT
Created attachment 617909 [details] [diff] [review]
patch for firefox
Comment 9 Paul Rouget [:paul] 2012-04-24 11:09:13 PDT
*** Bug 747091 has been marked as a duplicate of this bug. ***
Comment 10 Paul Rouget [:paul] 2012-04-24 11:14:24 PDT
Comment on attachment 617909 [details] [diff] [review]
patch for firefox

> let keyTooltip = "(" + combo.join(separator) + ")";
Why do you move the brackets here and don't keep it in inspector.properties?
Comment 11 Colby Russell :crussell (no longer Mozilla-ing) 2012-04-24 13:55:03 PDT
DOM Inspector is switching to a scheduled release cycle (bug 746784).  dom-inspector branches for localization today.  2.0.12 will not block for this.  There is no question that this is a problem, though.

In this case, DOM Inspector's slow development pace is fortunate.  Since this shouldn't involve any string changes, if you can come up with a patch (soon) for DOM Inspector that *conditionally* removes the devtools inspector's key, or remaps it, or something (in contrast to unconditionally clobbering it as we do now, see attachment 466560 [details] [diff] [review] from bug 569054), I will accept it on dom-inspector.

Since this bug is now concerned with how the devtools inspector end of things, please file a new bug for the DOM Inspector bits, or feel free to repurpose bug 688183.  Please keep in mind the condition I mention in bug 688183, comment 5, though: no approach will be accepted where Ctrl/Cmd+Shift+I, by default, doesn't open DOM Inspector when it's installed.
Comment 12 Tetsuharu OHZEKI [:tetsuharu] [UTC+9] 2012-04-24 18:57:38 PDT
(In reply to Paul Rouget [:paul] from comment #10)
> Comment on attachment 617909 [details] [diff] [review]
> patch for firefox
> 
> > let keyTooltip = "(" + combo.join(separator) + ")";
> Why do you move the brackets here and don't keep it in inspector.properties?

If the case for key returns null when the keyset is overrided, the shortcut key string for tooltip will be empty strings.
So the tooltip will be shown "Select element with mouse ()" if I do not move the brackets from inspector.properties. This is strange.
Comment 13 Paul Rouget [:paul] 2012-04-24 20:56:08 PDT
(In reply to OHZEKI Tetsuharu [:saneyuki_s] from comment #12)
> (In reply to Paul Rouget [:paul] from comment #10)
> > Comment on attachment 617909 [details] [diff] [review]
> > patch for firefox
> > 
> > > let keyTooltip = "(" + combo.join(separator) + ")";
> > Why do you move the brackets here and don't keep it in inspector.properties?
> 
> If the case for key returns null when the keyset is overrided, the shortcut
> key string for tooltip will be empty strings.
> So the tooltip will be shown "Select element with mouse ()" if I do not move
> the brackets from inspector.properties. This is strange.

I see.

I would prefer if you could create a new property for this case, like we did here:

http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/devtools/inspector.properties#29

Can you also add a comment in this code to explain why sometimes the key is not accessible?

Thank you.
Comment 14 Tetsuharu OHZEKI [:tetsuharu] [UTC+9] 2012-04-25 05:50:18 PDT
Created attachment 618223 [details] [diff] [review]
patch for firefox v2

add the comment to localization note.
Comment 15 Axel Hecht [:Pike] 2012-04-25 07:53:05 PDT
Comment on attachment 618223 [details] [diff] [review]
patch for firefox v2

PS:

There's at least one localization that doesn't use (), http://mxr.mozilla.org/l10n-mozilla-aurora/source/zh-TW/browser/chrome/browser/devtools/inspector.properties#34

Also, if you make changes like this, you'd need to update the key name. Paul's suggestion for an additional string sounds right.
Comment 16 Robert Kaiser 2012-04-25 10:26:47 PDT
Comment on attachment 618223 [details] [diff] [review]
patch for firefox v2

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

::: browser/locales/en-US/chrome/browser/devtools/inspector.properties
@@ +22,5 @@
>  
>  # LOCALIZATION NOTE (inspectButton.tooltiptext):
>  # This button appears in the Inspector Toolbar. inspectButton is stateful,
>  # if it's pressed users can select an element with the mouse.
> +# %S is the keyboard shortcut with branckets.

Nice word creation ;-)
I guess the "n" is a typo, though.
Comment 17 Tetsuharu OHZEKI [:tetsuharu] [UTC+9] 2012-04-25 18:59:18 PDT
Created attachment 618510 [details] [diff] [review]
patch for firefox(central) v3

By comment 15, I add & change locale properties.
Comment 18 Tetsuharu OHZEKI [:tetsuharu] [UTC+9] 2012-04-27 17:05:18 PDT
*** Bug 749762 has been marked as a duplicate of this bug. ***
Comment 19 Wraithan (Chris McDonald) [:wraithan] 2012-04-28 15:21:56 PDT
*** Bug 749991 has been marked as a duplicate of this bug. ***
Comment 20 Alex Keybl [:akeybl] 2012-04-30 16:22:25 PDT
The patch attached here would require a string change and would have l10n impact. If bug 735214 is the regressing bug, should we consider backing that out from Aurora?
Comment 21 Paul Rouget [:paul] 2012-04-30 21:05:36 PDT
What if we just don't set the tooltip at all for Aurora if there's no <key> element, and for nightly, we add the extra string?
Comment 22 Paul Rouget [:paul] 2012-04-30 21:36:27 PDT
Created attachment 619831 [details] [diff] [review]
patch v1 - Aurora
Comment 23 Dão Gottwald [:dao] 2012-05-01 15:12:05 PDT
Comment on attachment 618510 [details] [diff] [review]
patch for firefox(central) v3

>+++ b/browser/devtools/highlighter/inspector.jsm

Please move 'let separator' up like Paul did in his patch.

>+++ b/browser/locales/en-US/chrome/browser/devtools/inspector.properties

> # LOCALIZATION NOTE (inspectButton.tooltiptext):
> # This button appears in the Inspector Toolbar. inspectButton is stateful,
> # if it's pressed users can select an element with the mouse.
> # %S is the keyboard shortcut.
>-inspectButton.tooltiptext=Select element with mouse (%S)
>+# When the key of inspect is overridden by other addons,
>+# We use inspectButton.tooltip.
>+inspectButton.tooltip=Select element with mouse
>+inspectButton.tooltipWithAccesskey=Select element with mouse (%S)

Make this one LOCALIZATION NOTE for each string and make sure to reference the right string name in both cases.
Rename inspectButton.tooltipWithAccesskey to inspectButtonWithAccesskey.tooltip.
Comment 24 Tetsuharu OHZEKI [:tetsuharu] [UTC+9] 2012-05-01 21:07:06 PDT
(In reply to Dão Gottwald [:dao] from comment #23)
> Comment on attachment 618510 [details] [diff] [review]
> patch for firefox(central) v3
> 
> >+++ b/browser/devtools/highlighter/inspector.jsm
> 
> Please move 'let separator' up like Paul did in his patch.
> 
> >+++ b/browser/locales/en-US/chrome/browser/devtools/inspector.properties
> 
> > # LOCALIZATION NOTE (inspectButton.tooltiptext):
> > # This button appears in the Inspector Toolbar. inspectButton is stateful,
> > # if it's pressed users can select an element with the mouse.
> > # %S is the keyboard shortcut.
> >-inspectButton.tooltiptext=Select element with mouse (%S)
> >+# When the key of inspect is overridden by other addons,
> >+# We use inspectButton.tooltip.
> >+inspectButton.tooltip=Select element with mouse
> >+inspectButton.tooltipWithAccesskey=Select element with mouse (%S)
> 
> Make this one LOCALIZATION NOTE for each string and make sure to reference
> the right string name in both cases.
> Rename inspectButton.tooltipWithAccesskey to
> inspectButtonWithAccesskey.tooltip.


Should we follow the naming rules in same file, like markupButton.tooltipWithAccesskey?
> markupButton.tooltip=Markup Panel
> markupButton.tooltipWithAccesskey=Markup Panel (%S)
Comment 25 Paul Rouget [:paul] 2012-05-01 21:32:51 PDT
Comment on attachment 619831 [details] [diff] [review]
patch v1 - Aurora

[Approval Request Comment]
Regression caused by (bug #): 735214
User impact if declined: Builtin Inspector not working if DOM Inspector installed
Testing completed (on m-c, etc.): locally
Risk to taking this patch (and alternatives if risky): almost in-existent (just adding a `if` and some indentation adjustments)
String changes made by this patch: no
Comment 26 Dão Gottwald [:dao] 2012-05-02 06:21:30 PDT
(In reply to OHZEKI Tetsuharu [:saneyuki_s] from comment #24)
> > Rename inspectButton.tooltipWithAccesskey to
> > inspectButtonWithAccesskey.tooltip.
> 
> Should we follow the naming rules in same file, like
> markupButton.tooltipWithAccesskey?

No, we don't need to.
Comment 27 Panos Astithas [:past] 2012-05-02 06:31:31 PDT
*** Bug 750756 has been marked as a duplicate of this bug. ***
Comment 28 Tetsuharu OHZEKI [:tetsuharu] [UTC+9] 2012-05-02 10:26:44 PDT
Created attachment 620372 [details] [diff] [review]
patch for firefox(central) v4
Comment 29 Dão Gottwald [:dao] 2012-05-02 10:45:55 PDT
Comment on attachment 620372 [details] [diff] [review]
patch for firefox(central) v4

>-# LOCALIZATION NOTE (inspectButton.tooltiptext):
>+# LOCALIZATION NOTE (inspectButton.*):
> # This button appears in the Inspector Toolbar. inspectButton is stateful,
> # if it's pressed users can select an element with the mouse.
>-# %S is the keyboard shortcut.
>-inspectButton.tooltiptext=Select element with mouse (%S)
>+# Ordinarily we use inspectButtonWithAccesskey.tooltip
>+# that contains the shortcut. %S is the keyboard shortcut.
>+# When the key of inspect is overridden by other addons,
>+# We use inspectButton.tooltip.

Make this two LOCALIZATION NOTEs, one for inspectButtonWithAccesskey.tooltip and one for inspectButton.tooltip.

And again, please move 'let separator' up like Paul did in his patch. It's probably best if you just start with Paul's patch and add your modifications to that.
Comment 30 Tetsuharu OHZEKI [:tetsuharu] [UTC+9] 2012-05-02 12:01:53 PDT
Created attachment 620415 [details] [diff] [review]
patch for firefox(central) v5
Comment 31 Dão Gottwald [:dao] 2012-05-02 14:24:01 PDT
Comment on attachment 620415 [details] [diff] [review]
patch for firefox(central) v5

>+    }
>+    else {
>+      tooltip = this.strings.GetStringFromName("inspectButton.tooltip");

nit: } else { on one line

>+# LOCALIZATION NOTE (inspectButtonWithAccesskey.tooltip):

Actually, inspectButtonWithAccesskey doesn't make sense, since this is a shortcut key rather than an access key. So we need a better name, such as inspectButtonWithShortcutKey.tooltip.

> # This button appears in the Inspector Toolbar. inspectButton is stateful,
> # if it's pressed users can select an element with the mouse.
>-# %S is the keyboard shortcut.
>+# Ordinarily we use inspectButtonWithAccesskey.tooltip
>+# that contains the shortcut. %S is the keyboard shortcut.

This change can be avoided.

>+# LOCALIZATION NOTE (inspectButton.tooltip):
>+# This button appears in the Inspector Toolbar. inspectButton is stateful,
>+# if it's pressed users can select an element with the mouse.
>+# If the key of inspect is overridden by other addons,
>+# we use inspectButton.tooltip.

Just cut this down to:

# Same as inspectButtonWithShortcutKey.tooltip but used when an add-on
# overrides the shortcut key.
Comment 32 Lukas Blakk [:lsblakk] use ?needinfo 2012-05-02 15:39:51 PDT
Comment on attachment 619831 [details] [diff] [review]
patch v1 - Aurora

[Triage comment]
Low risk patch, go ahead with landing to Aurora (14)
Comment 33 Lukas Blakk [:lsblakk] use ?needinfo 2012-05-02 15:41:24 PDT
Assigning this to Paul to drive landings once patches approved.
Comment 34 Tetsuharu OHZEKI [:tetsuharu] [UTC+9] 2012-05-02 17:37:45 PDT
Created attachment 620538 [details] [diff] [review]
patch for firefox(central) v6
Comment 35 Paul Rouget [:paul] 2012-05-03 01:37:27 PDT
I'll land these 2 patches.
Comment 36 Paul Rouget [:paul] 2012-05-03 01:59:10 PDT
Aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/a09e941412d1
Comment 37 Paul Rouget [:paul] 2012-05-03 02:02:37 PDT
Fx-team: https://hg.mozilla.org/integration/fx-team/rev/3b7072da140c
Comment 38 Boris Zbarsky [:bz] 2012-05-03 23:25:59 PDT
*** Bug 751815 has been marked as a duplicate of this bug. ***
Comment 39 Boris Zbarsky [:bz] 2012-05-03 23:29:36 PDT
> However, at now, the need of DOM inspector is not high. It is rather low.

It's pretty darned high if you actually want to inspect at DOM in a sane tree view in a separate window from the web page, fwiw.
Comment 40 Tim Taubert [:ttaubert] 2012-05-04 07:25:55 PDT
https://hg.mozilla.org/mozilla-central/rev/3b7072da140c
Comment 41 Scott Johnson (:jwir3) 2012-05-05 11:16:47 PDT
*** Bug 752166 has been marked as a duplicate of this bug. ***
Comment 42 Colby Russell :crussell (no longer Mozilla-ing) 2012-07-05 15:53:27 PDT
*** Bug 764110 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.