Closed Bug 747603 Opened 7 years ago Closed 7 years ago

Firefox:Inspector cannot start up with DOMi

Categories

(DevTools :: Inspector, defect)

defect
Not set

Tracking

(firefox14+ fixed)

RESOLVED FIXED
Firefox 15
Tracking Status
firefox14 + fixed

People

(Reporter: tetsuharu, Assigned: tetsuharu)

References

Details

(Keywords: dogfood, regression)

Attachments

(2 files, 6 obsolete files)

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
----------------------
Depends on: 735214
OS: Windows 7 → All
Hardware: x86_64 → All
Attached patch proposal (obsolete) — Splinter Review
This patch removes the shortcut of start up DOMi for Firefox.

Should we remove the shortcut key from Firefox any more?
buildButtonsTooltip in inspector.jsm could just handle this by returning in case key is null.
(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...
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.
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.
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.
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"?
Component: DOM Inspector → Developer Tools: Inspector
Product: Other Applications → Firefox
QA Contact: dom-inspector → developer.tools.inspector
Blocks: 735214
No longer depends on: 735214
Attached patch patch for firefox (obsolete) — Splinter Review
Attachment #617182 - Attachment is obsolete: true
Attachment #617909 - Flags: review?(dao)
Duplicate of this bug: 747091
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?
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.
No longer blocks: DOMi2.0.12
Keywords: regression
(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.
(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.
Attached patch patch for firefox v2 (obsolete) — Splinter Review
add the comment to localization note.
Attachment #617909 - Attachment is obsolete: true
Attachment #617909 - Flags: review?(dao)
Attachment #618223 - Flags: review?(dao)
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 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.
Attached patch patch for firefox(central) v3 (obsolete) — Splinter Review
By comment 15, I add & change locale properties.
Attachment #618223 - Attachment is obsolete: true
Attachment #618223 - Flags: review?(dao)
Attachment #618510 - Flags: review?(dao)
Duplicate of this bug: 749762
Duplicate of this bug: 749991
Keywords: dogfood
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?
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?
Attachment #619831 - Flags: review?(dao)
Attachment #619831 - Flags: review?(dao) → review+
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.
Attachment #618510 - Flags: review?(dao) → review-
(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 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
Attachment #619831 - Flags: approval-mozilla-aurora?
(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.
Duplicate of this bug: 750756
Attached patch patch for firefox(central) v4 (obsolete) — Splinter Review
Attachment #618510 - Attachment is obsolete: true
Attachment #620372 - Flags: review?(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.
Attachment #620372 - Flags: review?(dao)
Attached patch patch for firefox(central) v5 (obsolete) — Splinter Review
Attachment #620372 - Attachment is obsolete: true
Attachment #620415 - Flags: review?(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.
Attachment #620415 - Flags: review?(dao) → review-
Comment on attachment 619831 [details] [diff] [review]
patch v1 - Aurora

[Triage comment]
Low risk patch, go ahead with landing to Aurora (14)
Attachment #619831 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assigning this to Paul to drive landings once patches approved.
Assignee: nobody → paul
Attachment #620415 - Attachment is obsolete: true
Attachment #620538 - Flags: review?(dao)
Assignee: paul → saneyuki.s.snyk
Attachment #620538 - Flags: review?(dao) → review+
I'll land these 2 patches.
> 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.
https://hg.mozilla.org/mozilla-central/rev/3b7072da140c
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 15
Duplicate of this bug: 752166
No longer blocks: 735214
Blocks: 735214
Duplicate of this bug: 764110
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.