226097 - DOM Inspector/Page Info accesskey conflict in Tools menu; commandkey doesn't work

Status: RESOLVED FIXED

(Firefox :: Menus, defect)

Description

[Jeff Walden [:Waldo]]

Currently in Firebird's Tools menu both the DOM Inspector and Page Info have the
same accesskey of I.  I would lean towards changing the DOM Inspector's
accesskey, as the menu option for it is new while Page Info is more ingrained. 
Perhaps the DOM Inspector should have an accesskey of D?

I would make a patch, but as I don't know which accesskey we want here and my
Mozilla tree is stored on my Linux boot, it's probably a little more trouble for
me than for someone else.  For the would-be patch maker, the accesskey is stored
in this file in the entity inspectorTaskCmd.accesskey:

mozilla/extensions/inspector/resources/locale/en-US/tasksOverlay.dtd

Comment 1

[Steffen Wilberg]

The accesskey "d" would be fine.

While we're at it, the commandkey doesn't work.
Mozilla uses Ctrl-Shift-I, which isn't bad, considering the that the few using
DOMi are probably geeky enough to use a Ctrl-Shift-shortcut; that Ctrl-D is in
use, it's "add bookmark"; and that Ctrl-I is in use as well, it shows the
bookmarks toolbar (IE-like).

Comment 2

[Steffen Wilberg]

Attachment: patch

This patch only changes
mozilla/browser/extensions/inspector/content/tasksOverlay.xul. We can't change
tasksOverlay.dtd unless we fork it. I don't think it's worth to fork because
the only change would be that accesskey. The commandkey has to be hardcoded in
tasksOverlay.xul, like it already is, otherwise a nice XUL "error bar" is
displayed below the status bar.
There's some whitespace cleanup in my patch, the real changes are:
- use "mainKeyset" to make it work in Firebird
- accesskey="d" instead of "&inspectorTaskCmd.accesskey;".

Comment 3

[Steffen Wilberg]

Comment on attachment 135832 [details] [diff] [review]
patch

Ben, since you added DOMi and forked this file, would you mind a quick look,
please?

Comment 4

[Steffen Wilberg]

taking.

Comment 5

[Jeremy M. Dolan]

> Ctrl-I is in use as well, it shows the bookmarks toolbar (IE-like).

actually, Ctrl-I is page info, as shown in the tools menu.

> accesskey="d" command="Tasks:InspectPage"/>

If <menuitem> is like XUL I've worked with in the past, you might want to use a
capital-D there, as otherwise I believe it will search the entire string for a
lowercase-d first, and then fallback to 'D', wasting a few cycles.

I'd also suggest, unless someone sees some risk here, the be nominated for 0.8.
It's nice polish. Especially since DOMi will be the new feature everyone wants
to play with.

Comment 6

[Steffen Wilberg]

> actually, Ctrl-I is page info, as shown in the tools menu.
It's page info on Mac and Linux. But it's the bookmarks sidebar on Win, see bug
226273 comment 0.

You're right, it should be accesskey="D":
http://www.mozilla.org/projects/ui/accessibility/accesskey.html
I guess I don't need to make a new patch for this.

Targetting.

Comment 7

[Jeff Walden [:Waldo]]

Comment on attachment 135832 [details] [diff] [review]
patch

The new Downloads Manager has usurped the D accesskey, as it IMHO rightly
should - Downloads will be much more used than the DOM Inspector.  Now what for
an accesskey?

Comment 8

[Steffen Wilberg]

Comment on attachment 135832 [details] [diff] [review]
patch

Ben has stolen my accesskey.

Comment 9

[Steffen Wilberg]

Which accesskey? D,O,I and S are taken.
How about "n"? Like 'nspector.
We could also change Page Info from I to P and use I for DOMi.

Comment 10

[Steffen Wilberg]

Attachment: patch v.2

Forks taksOverlay.dtd, saving a couple of bytes since we don't need the
"inspect this page" stuff. Use "n" as accesskey.
Use "mainKeyset" to make the commandkey work in Firebird. The rest is
whitespace cleanup in tasksOverlay.xul.
The commandkey has to be hardcoded in tasksOverlay.xul, like it already is,
otherwise a nice XUL "error bar" is displayed below the status bar.

Comment 11

[Steffen Wilberg]

Comment on attachment 136593 [details] [diff] [review]
patch v.2

Ben, please have a look.

Comment 12

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 136593 [details] [diff] [review]
patch v.2

You add a command key entity here:

inspectorTaskCmd.commandkey

But you still hardcode the value here: 

<key id="key_inspectPage" key="i" modifiers="accel,shift"
command="Tasks:InspectPage"/>

Give me a revised patch for this and I'll check it in.

Comment 13

[Steffen Wilberg]

Attachment: patch without the entity

Same as before, but without the inspectorTaskCmd.commandkey entity.

Comment 14

[Steffen Wilberg]

Comment on attachment 136866 [details] [diff] [review]
patch without the entity

As I've said before, the commandkey has to be hardcoded in tasksOverlay.xul,
like it already is. Using an entity results in displaying a XUL "error bar"
below the status bar.
So I removed the entity from the dtd.

Comment 15

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 136866 [details] [diff] [review]
patch without the entity

I've checked in this latest patch, but used the accesskey for the <key> since I
don't get any XML errors.. maybe you made a typo in your own attempts?

Comment 16

[Steffen Wilberg]

Yes, the error is displayed if I introduce a typo. Grr.
Whatever, the files you checked in work.
Marking fixed!