Firefox:Inspector cannot start up with DOMi

RESOLVED FIXED in Firefox 14

Status

()

Firefox
Developer Tools: Inspector
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: tetsuharu, Assigned: tetsuharu)

Tracking

({dogfood, regression})

Trunk
Firefox 15
dogfood, regression
Points:
---

Firefox Tracking Flags

(firefox14+ fixed)

Details

Attachments

(2 attachments, 6 obsolete attachments)

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

Updated

5 years ago
Depends on: 735214
OS: Windows 7 → All
Hardware: x86_64 → All
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?
(Assignee)

Updated

5 years ago
Blocks: 738048
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"?

Updated

5 years ago
Component: DOM Inspector → Developer Tools: Inspector
Product: Other Applications → Firefox
QA Contact: dom-inspector → developer.tools.inspector

Updated

5 years ago
Blocks: 735214
No longer depends on: 735214
Created attachment 617909 [details] [diff] [review]
patch for firefox
Attachment #617182 - Attachment is obsolete: true
Attachment #617909 - Flags: review?(dao)

Updated

5 years ago
Duplicate of this bug: 747091

Comment 10

5 years ago
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: 738048

Updated

5 years ago
tracking-firefox14: --- → ?
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.

Comment 13

5 years ago
(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.
Created attachment 618223 [details] [diff] [review]
patch for firefox v2

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 16

5 years ago
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.
Created attachment 618510 [details] [diff] [review]
patch for firefox(central) v3

By comment 15, I add & change locale properties.
Attachment #618223 - Attachment is obsolete: true
Attachment #618223 - Flags: review?(dao)
Attachment #618510 - Flags: review?(dao)
(Assignee)

Updated

5 years ago
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?
tracking-firefox14: ? → +

Comment 21

5 years ago
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

5 years ago
Created attachment 619831 [details] [diff] [review]
patch v1 - Aurora

Updated

5 years ago
Attachment #619831 - Flags: review?(dao)

Updated

5 years ago
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 25

5 years ago
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
Created attachment 620372 [details] [diff] [review]
patch for firefox(central) v4
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)
Created attachment 620415 [details] [diff] [review]
patch for firefox(central) v5
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
Created attachment 620538 [details] [diff] [review]
patch for firefox(central) v6
Attachment #620415 - Attachment is obsolete: true
Attachment #620538 - Flags: review?(dao)

Updated

5 years ago
Assignee: paul → saneyuki.s.snyk

Updated

5 years ago
Attachment #620538 - Flags: review?(dao) → review+

Comment 35

5 years ago
I'll land these 2 patches.

Comment 36

5 years ago
Aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/a09e941412d1
status-firefox14: --- → fixed

Comment 37

5 years ago
Fx-team: https://hg.mozilla.org/integration/fx-team/rev/3b7072da140c
Whiteboard: [fixed-in-fx-team]
Duplicate of this bug: 751815
> 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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 15

Updated

5 years ago
Duplicate of this bug: 752166

Updated

5 years ago
No longer blocks: 735214

Updated

5 years ago
Blocks: 735214
Duplicate of this bug: 764110
You need to log in before you can comment on or make changes to this bug.