747603 - Firefox:Inspector cannot start up with DOMi

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

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

Attachment: 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]]

buildButtonsTooltip in inspector.jsm could just handle this by returning in case key is null.

Comment 3

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

(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]]

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

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]]

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

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

Attachment: patch for firefox

Comment 10

[Paul Rouget [:paul]]

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

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

(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]]

(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)]

Attachment: patch for firefox v2

add the comment to localization note.

Comment 15

[Axel Hecht [:Pike]]

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]

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

Attachment: patch for firefox(central) v3

By comment 15, I add & change locale properties.

Comment 20

[Alex Keybl [:akeybl]]

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]]

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]]

Attachment: patch v1 - Aurora

Comment 23

[Dão Gottwald [:dao]]

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

(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]]

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]]

(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 28

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: patch for firefox(central) v4

Comment 29

[Dão Gottwald [:dao]]

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

Attachment: patch for firefox(central) v5

Comment 31

[Dão Gottwald [:dao]]

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]

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]

Assigning this to Paul to drive landings once patches approved.

Comment 34

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: patch for firefox(central) v6

Comment 35

[Paul Rouget [:paul]]

I'll land these 2 patches.

Comment 36

[Paul Rouget [:paul]]

Aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/a09e941412d1

Comment 37

[Paul Rouget [:paul]]

Fx-team: https://hg.mozilla.org/integration/fx-team/rev/3b7072da140c

Comment 39

[Boris Zbarsky [:bzbarsky]]

> 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] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/3b7072da140c