Key element with id 'key_devToolboxMenuItem' / 'key_webide' not found when opening additional browser window

REOPENED
Assigned to
(Needinfo from 2 people)

Status

defect
P3
normal
REOPENED
3 years ago
5 months ago

People

(Reporter: jryans, Assigned: ochameau, NeedInfo)

Tracking

({leave-open})

unspecified
Firefox 52

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

3 years ago
This is quite similar to bug 1261920.

STR:

1. Start Firefox
2. A browser window appears, no errors so far
3. Open an additional browser window, errors appear:

console.error: CustomizableUI:
  Key element with id 'key_devToolboxMenuItem' for widget 'developer-button' not found!
console.error: CustomizableUI:
  Key element with id 'key_webide' for widget 'webide-button' not found!
(Reporter)

Updated

3 years ago
Summary: Key element with id 'key_devToolboxMenuItem' / 'key_webide' not found! → Key element with id 'key_devToolboxMenuItem' / 'key_webide' not found when opening additional browser window
(Reporter)

Comment 1

3 years ago
:ochameau, do you have any ideas about this?  I know you've worked in this area with the similar bug 1261920 in the past.
Flags: needinfo?(poirot.alex)
(Assignee)

Comment 2

3 years ago
One thing to add to the STR, open WebIDE at least once.

So the story is that <xul:key> are added dynamically now, via browser-menus.js.
But it is done too late regarding CustomizableUI.
CustomizableUI is already loading on browser.xul's load event and expect to find any related <xul:key>.
Whereas browser-menus's entry point which is "addMenus" is only called on browser-delayed-startup-finished.
We delay as far as we can to prevent impacting firefox startup, but we may be more agressive for newly opened windows.
(Assignee)

Updated

3 years ago
Flags: needinfo?(poirot.alex)
(Assignee)

Updated

3 years ago
Assignee: nobody → poirot.alex
(Assignee)

Comment 3

3 years ago
Posted patch patch v1 (obsolete) — Splinter Review
Load devtools in two steps. Watch for browser.xul's DOMContentLoaded that, to register keys before CustomizableUI.
I'm not a big fan of this as we aren't that lazy anymore and complexity gDevToolsBrowser...
But I don't see how to address this differently?
You'll get these errors when detaching tabs into their own process/windows as well.

STR:

* launch the latest version of m-a
** build used: fx50.0a2, buildId: 20160824004001, changeset: a72bfbdf5c9b
* open a new tab so there's two tabs opened
* detach one of the tabs into it's own separate process/window

You'll receive the following errors in the browser console:

* CustomizableUI:Key element with id 'key_devToolboxMenuItem' for widget 'developer-button' not found! CustomizableUI.jsm:1370
* CustomizableUI:Key element with id 'key_webide' for widget 'webide-button' not found! CustomizableUI.jsm:1370
(Assignee)

Comment 6

3 years ago
Comment on attachment 8784373 [details] [diff] [review]
patch v1

Tests look green on opt.
Do you thin that's a sensible approach?
I wish we could keep things simple and lazy...
Attachment #8784373 - Flags: feedback?(jryans)
(Assignee)

Comment 7

3 years ago
Thanks kamil for your report, this patch should fix both STRs.
(Reporter)

Comment 8

3 years ago
Comment on attachment 8784373 [details] [diff] [review]
patch v1

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

Seems to work well, thanks for the fix.  It seems reasonable enough to me.  You may want to do some Talos runs as well, to be sure it's not regressing browser startup.
Attachment #8784373 - Flags: feedback?(jryans) → feedback+
(Assignee)

Comment 10

3 years ago
Here is my try run:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=82a7837b76a2&selectedJob=26324861
The base changeset, from mozilla-central:
  https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=01748a2b1a463f24efd9cd8abad9ccfd76b037b8&filter-searchStr=talos&selectedJob=4739416

And if I understand talos and the related tools correctly, here is the results:
  https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&originalRevision=01748a2b1a46&newProject=try&newRevision=f8cd7ed8b05f&framework=1

Which reports scary things regarding fileio but not that much around startup itself.
There is tabpaint opt e10s on linux with a 7% regression but some others better improvements on others like tp5o responsiveness opt e10s.
Hard to tell, I've just started some new runs on the base build.

fileio regression makes sense if we start pulling many dependencies via addMenus... which doesn't surprise me.
(Reporter)

Comment 11

3 years ago
Looking at the results, it does seem there are some regressions.  Can we try to be more lazy inside addMenus code path at least?
(Assignee)

Comment 12

3 years ago
Hum. I looked at what modules we are pulling earlier and it looks like there is only two:
  resource://devtools/client/framework/browser-menus.js
  resource://devtools/client/menus.js
Which is the bare minimum for executing addMenus.
That's for the commonjs modules, but we are also loading this properties file:
  chrome://devtools/locale/menus.properties

I'm wondering if just these 3 resources being loaded earlier are the reason of the possible reported regressions ?!?
(Assignee)

Comment 13

3 years ago
Another patch that has nothing to do with this bug, but just to see the influence
of loading a bunch of modules during firefox startup.
This patch stops loading a bunch of SDK modules related to unload modules
and tons of helpers pulled by JSON viewer xpcom...
(Assignee)

Comment 15

3 years ago
https://treeherder.mozilla.org/perf.html#/compare?originalProject=autoland&originalRevision=7293879d87d7&newProject=try&newRevision=4c8154e148f9&framework=1

It looks like loading less modules during very early devtools startup, from devtools-startup.js, so very early during firefox startup, has an effect on sessionrestore, ts_paint and tp5o. Between 2.7% to 6.5% win. I don't know yet about fileio because it needs talos run on windows, which I juts spawn on my try build.
(Assignee)

Comment 16

3 years ago
Hum. fileio test seems to be misleading. It also report a regression with the second patch which clearly optimize things for most of the other tests.

So I'm wondering if we should just ignore fileio?

But still, the offending patch seems to regress "tabpaint opt e10s" by 7%, whereas my second optimizing patch doesn't affect this particular test. (so I can't trade this regression with an optimization)
(Reporter)

Comment 17

3 years ago
:jmaher, any thoughts on how to best interpret the data here?
Flags: needinfo?(jmaher)
for fileio- there is bug 1274018 for that which documents the misleading nature of it, so please ignore that.

As for the tabpaint, is there more data you need?
Flags: needinfo?(jmaher)
(Assignee)

Comment 21

3 years ago
Just pushed two new try with patches being rebased.
It no longer report any regression for the main patch (attachment 8784373 [details] [diff] [review]).
And still confirm a win with the second patch that has nothing to do with this bug.

https://treeherder.mozilla.org/perf.html#/compare?originalProject=fx-team&originalRevision=cfdb7af3af2e&newProject=try&newRevision=e5e8ad106067&framework=1&filter=tabpaint&showOnlyImportant=0

https://treeherder.mozilla.org/perf.html#/compare?originalProject=fx-team&originalRevision=cfdb7af3af2e&newProject=try&newRevision=3332f63169be&framework=1

Joel, could you confirm that I ran every important test on talos convering firefox startup? And that the test results looks green to you? I'm a bit confused the first run reported tabpaint regression and this one doesn't.
Also, what do you think about the second patch, there is around 2 to 4% wins in various tests. Is that significant? Is it worth spending time to get such win?
Flags: needinfo?(jmaher)
yes, i see all the tests ran for linux64/win7.  We have winxp/win8/osx10.10 to potentially run tests on, but these two platforms is a great representations of issues to see.

I do see a nice set of wins in the second patch, since it is on many tests and between the two platforms, I would be excited to see this land- it might not be worth 20+ hours of work to make it work without failure.
Flags: needinfo?(jmaher)
(Assignee)

Updated

3 years ago
Attachment #8784373 - Attachment is obsolete: true
(Assignee)

Comment 24

3 years ago
Comment on attachment 8785984 [details] [diff] [review]
Stop loading various useless SDK modules on early devtools startup.

Opened bug 1302996 to followup on this.

Comment 25

3 years ago
mozreview-review
Comment on attachment 8791559 [details]
Bug 1297535 - Register devtools menu and keys on DOMContentLoaded to happen before CustomizableUI.

https://reviewboard.mozilla.org/r/78962/#review77588

::: devtools/client/framework/devtools-browser.js:146
(Diff revision 1)
>            }
>          }
>          break;
> +      case "domwindowopened":
> +        let win = subject.QueryInterface(Ci.nsIDOMEventTarget);
> +        win.addEventListener("DOMContentLoaded", this);

Assuming this doesn't need to be uplifted beyond 50, we should be able to use the new 'once' option on addEventListener to avoid removing it later on: https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener#Parameters

::: devtools/client/framework/devtools-browser.js:419
(Diff revision 1)
> +   *
> +   * @param {ChromeWindow} window
> +   *        The window to which devtools should be hooked to.
> +   */
> +  _onBrowserWindowLoaded: function (win) {
> +    if (!win.gBrowser) {

In registerBrowserWindow we had a guard against ever adding the menus twice to the same document.

I don't think it's possible here but can you think of a case where it could happen?
Attachment #8791559 - Flags: review?(bgrinstead) → review+
(Assignee)

Comment 26

3 years ago
(In reply to Brian Grinstead [:bgrins] from comment #25)
> Comment on attachment 8791559 [details]
> Bug 1297535 - Register devtools menu and keys on DOMContentLoaded to happen
> before CustomizableUI.
> 
> https://reviewboard.mozilla.org/r/78962/#review77588
> 
> ::: devtools/client/framework/devtools-browser.js:146
> (Diff revision 1)
> >            }
> >          }
> >          break;
> > +      case "domwindowopened":
> > +        let win = subject.QueryInterface(Ci.nsIDOMEventTarget);
> > +        win.addEventListener("DOMContentLoaded", this);
> 
> Assuming this doesn't need to be uplifted beyond 50, we should be able to
> use the new 'once' option on addEventListener to avoid removing it later on:
> https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/
> addEventListener#Parameters

done.

> 
> ::: devtools/client/framework/devtools-browser.js:419
> (Diff revision 1)
> > +   *
> > +   * @param {ChromeWindow} window
> > +   *        The window to which devtools should be hooked to.
> > +   */
> > +  _onBrowserWindowLoaded: function (win) {
> > +    if (!win.gBrowser) {
> 
> In registerBrowserWindow we had a guard against ever adding the menus twice
> to the same document.
> 
> I don't think it's possible here but can you think of a case where it could
> happen?

No, and we would have to introduce a specific guard as we can't reuse the existing one from registerBrowserWindow. The two calls are from this module and are exclusive.
Comment hidden (mozreview-request)
(Assignee)

Comment 28

3 years ago
mozreview-review
Comment on attachment 8791559 [details]
Bug 1297535 - Register devtools menu and keys on DOMContentLoaded to happen before CustomizableUI.

https://reviewboard.mozilla.org/r/78962/#review78188

Comment 30

3 years ago
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/da90ec959a3c
Register devtools menu and keys on DOMContentLoaded to happen before CustomizableUI. r=bgrins

Comment 31

3 years ago
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/29f6034a4ecb
Backed out changeset da90ec959a3c for memory leaks in browser_885530_showInPrivateBrowsing.js
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 36

3 years ago
Comment on attachment 8794803 [details]
fix bug 1297535

I'll merge that into the first changeset, I had to ignore non firefox windows.
The DOMContentLoaded codepath was trigerred by some random window spawn by tests and ended up leaking.
I tried to put helpful comment, but do not hesitate to suggest new ones or new phrasing.
Attachment #8794803 - Flags: review?(bgrinstead)

Comment 37

3 years ago
mozreview-review
Comment on attachment 8794803 [details]
fix bug 1297535

https://reviewboard.mozilla.org/r/81096/#review80464

::: devtools/client/framework/devtools-browser.js:421
(Diff revision 1)
>     *        The window to which devtools should be hooked to.
>     */
>    _onBrowserWindowLoaded: function (win) {
> -    if (!win.gBrowser) {
> +    // This method is called for all top level window, only consider firefox
> +    // windows
> +    if (!win.location.href.endsWith("browser.xul")) {

Should we also check `!win.gBrowser || !win.location.href.endsWith("browser.xul")`?  Not sure if there's some weird case where some other window will end with browser.xul
Attachment #8794803 - Flags: review?(bgrinstead)

Comment 38

3 years ago
mozreview-review
Comment on attachment 8794803 [details]
fix bug 1297535

https://reviewboard.mozilla.org/r/81096/#review80468

Please re-check try before landing, the original push had a lot of bustage.  I just sent up a new push
Attachment #8794803 - Flags: review+
Comment hidden (mozreview-request)
(Assignee)

Updated

3 years ago
Attachment #8794803 - Attachment is obsolete: true
(Assignee)

Comment 40

3 years ago
(In reply to Brian Grinstead [:bgrins] from comment #37)
> Comment on attachment 8794803 [details]
> fix bug 1297535
> 
> https://reviewboard.mozilla.org/r/81096/#review80464
> 
> ::: devtools/client/framework/devtools-browser.js:421
> (Diff revision 1)
> >     *        The window to which devtools should be hooked to.
> >     */
> >    _onBrowserWindowLoaded: function (win) {
> > -    if (!win.gBrowser) {
> > +    // This method is called for all top level window, only consider firefox
> > +    // windows
> > +    if (!win.location.href.endsWith("browser.xul")) {
> 
> Should we also check `!win.gBrowser ||
> !win.location.href.endsWith("browser.xul")`?  Not sure if there's some weird
> case where some other window will end with browser.xul

Added that test, merged the two changesets and pushed to try.

Comment 41

3 years ago
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d271311721bb
Register devtools menu and keys on DOMContentLoaded to happen before CustomizableUI. r=bgrins

Comment 42

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d271311721bb
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52

Updated

3 years ago
Depends on: 1307365
(Assignee)

Comment 43

3 years ago
It looks like the previous patch is still regression Firefox startup (bug 1307365, I requested a backout there).

Here is an alternative one, using a completely different path.
I just avoid hitting the precise code from CustomizableUI that throws errors and break.
(Note that is wasn't just about an error in the console, the tooltip for WebIDE is also wrong on the new window)
By removing the "shortcutId" we prevent this issue, but loss the key shortcut displayed in the tooltip,
by handling the localization in devtools, we can keep displaying it.
Attachment #8797807 - Flags: review?(bgrinstead)
Comment on attachment 8797807 [details] [diff] [review]
Localize WebIDE widget from devtools to prevent exception in CustomizableUI.

I think :jryans would be a better reviewer for this
Attachment #8797807 - Flags: review?(bgrinstead) → review?(jryans)
(Reporter)

Updated

3 years ago
Status: RESOLVED → REOPENED
Keywords: leave-open
Resolution: FIXED → ---
(Reporter)

Comment 45

3 years ago
Comment on attachment 8797807 [details] [diff] [review]
Localize WebIDE widget from devtools to prevent exception in CustomizableUI.

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

Seems good for WebIDE, but what about key_devToolboxMenuItem?  Don't you need a change for it as well?
Attachment #8797807 - Flags: review?(jryans)
(Assignee)

Comment 46

3 years ago
Right, the same happens if you move the developer menu from the hamburger popup to the navbar, I'll also tweak it then.
(Assignee)

Comment 47

3 years ago
Sorry for not using mozreview, but with this previous changeset that isn't backedout
but which is meant to be backed out... I can't make mozreview accept another mozreview request for this bug :/

So, I removed all l10n keys from browser/ regarding customizableUI widgets (also the labels)
and did that for both items (webide and devtools).
Attachment #8799352 - Flags: review?(jryans)
(Assignee)

Updated

3 years ago
Attachment #8797807 - Attachment is obsolete: true
(Reporter)

Comment 48

3 years ago
Comment on attachment 8799352 [details] [diff] [review]
Localize devtools widgets from devtools to prevent exception in CustomizableUI.

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

(Not sure I follow the issues you're having with MozReview, we should talk about it!  Maybe it's related to the single patch queue approach you are using?  You should be able to `git mozreview push X` to push specific commits no matter what, I would think.)

Seems to fix the issue, but I have some questions about the shortcut text.

::: devtools/client/locales/en-US/menus.properties
@@ +59,5 @@
>  webide.key = VK_F8
>  webide.keytext = F8
> +# LOCALIZATION NOTE (webide.widget.tooltiptext): This is the label for the icon
> +# in the navigation bar that is added only after you already opened WebIDE.
> +webide.widget.tooltiptext = Open WebIDE (Shift+F8)

This is currently shown (at least on macOS) with a shift arrow icon.  Is there any way to continue computing this as before?

@@ +69,5 @@
> +devtools.widget.label = Developer
> +# LOCALIZATION NOTE (devtools.widget.tooltiptext): This is the label for the icon
> +# when you customize and move the Developer item from the hamburger menu to the
> +# navigation bar.
> +devtools.widget.tooltiptext = Open Web developer tools (Ctrl+Shift+I)

Hmm, so here's a place where this plan is complicated since we don't use the same shortcut on all OSes (toggle toolbox is Cmd-Opt-I on Mac).  It currently gets replaced with the modifier key symbols, like "Open Web developer tools (⌥⌘I)".

At the same time, if you actually do use this item in the navigation bar, clicking it doesn't open the tools, it shows the Developer menu.  So, why do we list the toggle tools shortcut here at all...?

I guess that is the standard practice though...?  It looks like other submenu widgets like history and bookmarks list the main shortcut for their category as the tooltip text of the menu's button like we are doing now.
Attachment #8799352 - Flags: review?(jryans) → feedback+
Backed out the patch for developer's request, https://bugzilla.mozilla.org/show_bug.cgi?id=1307365#c8

Comment 50

3 years ago
Backout by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/2976adccde9b
Backed out changeset d271311721bb for developer's request
(Assignee)

Comment 51

3 years ago
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #48)
> Comment on attachment 8799352 [details] [diff] [review]
> Localize devtools widgets from devtools to prevent exception in
> CustomizableUI.
> 
> Review of attachment 8799352 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (Not sure I follow the issues you're having with MozReview, we should talk
> about it!  Maybe it's related to the single patch queue approach you are
> using?  You should be able to `git mozreview push X` to push specific
> commits no matter what, I would think.)

It says "Parent review request is submitted or discarded".
I tried erasing the mozreview id that is in the commit message, but it doesn't change anything.
It isn't related to the single patch queue, I'm now cherry-picking patches in a dedicated branch before pushing to mozreview.
Here the existing mozreview was still with the previous "to be backed out" changeset. I didn't wanted to push to that branch as I want only my new changeset to be visible. Also I wanted to just have a new mozreview in order to keep the previous review as-is for the archive.
It looks like we can't have more than one mozreview per bug?

> ::: devtools/client/locales/en-US/menus.properties
> @@ +59,5 @@
> >  webide.key = VK_F8
> >  webide.keytext = F8
> > +# LOCALIZATION NOTE (webide.widget.tooltiptext): This is the label for the icon
> > +# in the navigation bar that is added only after you already opened WebIDE.
> > +webide.widget.tooltiptext = Open WebIDE (Shift+F8)
> 
> This is currently shown (at least on macOS) with a shift arrow icon.  Is
> there any way to continue computing this as before?

Shift arrow icon?? What is that? In the navigation bar?
We should have the worl icon with a pencil on windows/linux. And a regular tooltip when hovering with this text.

> @@ +69,5 @@
> > +devtools.widget.label = Developer
> > +# LOCALIZATION NOTE (devtools.widget.tooltiptext): This is the label for the icon
> > +# when you customize and move the Developer item from the hamburger menu to the
> > +# navigation bar.
> > +devtools.widget.tooltiptext = Open Web developer tools (Ctrl+Shift+I)
> 
> Hmm, so here's a place where this plan is complicated since we don't use the
> same shortcut on all OSes (toggle toolbox is Cmd-Opt-I on Mac).  It
> currently gets replaced with the modifier key symbols, like "Open Web
> developer tools (⌥⌘I)".

We already have KeyShortcuts.stringify. We are already using it for just one key shortcut (minimize the toolbox, which is a hidden feature, still). It stringify to the electron syntax so it isn't as nice nor localized.

> At the same time, if you actually do use this item in the navigation bar,
> clicking it doesn't open the tools, it shows the Developer menu.  So, why do
> we list the toggle tools shortcut here at all...?

Yes it doesn't sound very consistent. Especially given that the first item of the menu is that precise command (toggle tools). So it should be somewhat easy to discover this shortcut.

> I guess that is the standard practice though...?  It looks like other
> submenu widgets like history and bookmarks list the main shortcut for their
> category as the tooltip text of the menu's button like we are doing now.

Listing Bookmarks is somewhar similar, you get a different behavior between clicking on the icon and hitting the shortcut. (open menu versus a window)


I really don't want to spend time optimizing something that can't be much faster (previous patch).
So I would like to figure out something else. This new patch or yet another approach.
I see two tweaks to the current patch:
 * drop the shortcut on developer button.
 * display the shortcut by passing it through KeyShortcuts.stringify
 * display the shortcut by passing it through KeyShortcuts.stringify and improve it to have a localized string.
(Reporter)

Comment 52

3 years ago
(In reply to Alexandre Poirot [:ochameau] from comment #51)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #48)
> > Comment on attachment 8799352 [details] [diff] [review]
> > Localize devtools widgets from devtools to prevent exception in
> > CustomizableUI.
> > 
> > Review of attachment 8799352 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > (Not sure I follow the issues you're having with MozReview, we should talk
> > about it!  Maybe it's related to the single patch queue approach you are
> > using?  You should be able to `git mozreview push X` to push specific
> > commits no matter what, I would think.)
> 
> It says "Parent review request is submitted or discarded".
> I tried erasing the mozreview id that is in the commit message, but it
> doesn't change anything.
> It isn't related to the single patch queue, I'm now cherry-picking patches
> in a dedicated branch before pushing to mozreview.
> Here the existing mozreview was still with the previous "to be backed out"
> changeset. I didn't wanted to push to that branch as I want only my new
> changeset to be visible. Also I wanted to just have a new mozreview in order
> to keep the previous review as-is for the archive.
> It looks like we can't have more than one mozreview per bug?

There is an assumption of one active MozReview session per bug currently, so it either creates one if there's none or updates with new patches if one exists.

To start a totally separate review session, I guess the simplest path would be to go to the old session and discard it first, like when rebasing.  It's not deleted, it just gets "closed", and there are still comments in the bug that link to it for history.

> > ::: devtools/client/locales/en-US/menus.properties
> > @@ +59,5 @@
> > >  webide.key = VK_F8
> > >  webide.keytext = F8
> > > +# LOCALIZATION NOTE (webide.widget.tooltiptext): This is the label for the icon
> > > +# in the navigation bar that is added only after you already opened WebIDE.
> > > +webide.widget.tooltiptext = Open WebIDE (Shift+F8)
> > 
> > This is currently shown (at least on macOS) with a shift arrow icon.  Is
> > there any way to continue computing this as before?
> 
> Shift arrow icon?? What is that? In the navigation bar?
> We should have the worl icon with a pencil on windows/linux. And a regular
> tooltip when hovering with this text.

Sorry, I was talking about in the tooltip text, to represent the key combo.  Instead of the word "Shift", there's an up arrow icon.

> > @@ +69,5 @@
> > > +devtools.widget.label = Developer
> > > +# LOCALIZATION NOTE (devtools.widget.tooltiptext): This is the label for the icon
> > > +# when you customize and move the Developer item from the hamburger menu to the
> > > +# navigation bar.
> > > +devtools.widget.tooltiptext = Open Web developer tools (Ctrl+Shift+I)
> > 
> > Hmm, so here's a place where this plan is complicated since we don't use the
> > same shortcut on all OSes (toggle toolbox is Cmd-Opt-I on Mac).  It
> > currently gets replaced with the modifier key symbols, like "Open Web
> > developer tools (⌥⌘I)".
> 
> We already have KeyShortcuts.stringify. We are already using it for just one
> key shortcut (minimize the toolbox, which is a hidden feature, still). It
> stringify to the electron syntax so it isn't as nice nor localized.
> 
> > At the same time, if you actually do use this item in the navigation bar,
> > clicking it doesn't open the tools, it shows the Developer menu.  So, why do
> > we list the toggle tools shortcut here at all...?
> 
> Yes it doesn't sound very consistent. Especially given that the first item
> of the menu is that precise command (toggle tools). So it should be somewhat
> easy to discover this shortcut.
> 
> > I guess that is the standard practice though...?  It looks like other
> > submenu widgets like history and bookmarks list the main shortcut for their
> > category as the tooltip text of the menu's button like we are doing now.
> 
> Listing Bookmarks is somewhar similar, you get a different behavior between
> clicking on the icon and hitting the shortcut. (open menu versus a window)
> 
> 
> I really don't want to spend time optimizing something that can't be much
> faster (previous patch).
> So I would like to figure out something else. This new patch or yet another
> approach.
> I see two tweaks to the current patch:
>  * drop the shortcut on developer button.
>  * display the shortcut by passing it through KeyShortcuts.stringify
>  * display the shortcut by passing it through KeyShortcuts.stringify and
> improve it to have a localized string.

I guess let's try to display the shortcut for now, to avoid a behavior change here.  Was the shortcut not localized before?
(Assignee)

Comment 53

3 years ago
On Windows and linux customizable ui shortcuts in tooltips are localized. It isn't using icons which is a nice way to not translate strings...
This started happening a lot to me very recently. @ochameau, have there been any recent changes to webide?
Flags: needinfo?(poirot.alex)
(Assignee)

Comment 55

a year ago
I imagine that's because you switched to DevEdition, or moved one of the devtools icon into the customizable navbar (devtools wrench or webide) and you are opening more than one top level window.

Updated

11 months ago
Product: Firefox → DevTools
The leave-open keyword is there and there is no activity for 6 months.
:ochameau, maybe it's time to close this bug?
Flags: needinfo?(poirot.alex)
You need to log in before you can comment on or make changes to this bug.