398706 - DOM Inspector DOM tree viewer context menu does not have access keys

Status: RESOLVED FIXED

(Other Applications :: DOM Inspector, defect)

Description

[(no longer active)]

Attachment: Add the access keys

DOM Inspector DOM tree viewer context menu lacks several access keys, and has incorrect ones as well (for example, the After menu item under Paste has access key "P").  The attached patch adds and corrects the necessary access keys.

Comment 1

[[e-mail]]

Comment on attachment 283699 [details] [diff] [review]
Add the access keys

I'm not convinced with your choices of access keys, so here are some suggestions for Shawn to consider :-)

>   <!ENTITY cmdInspectBrowser.label "Inspect Contained Document">
>+  <!ENTITY cmdInspectBrowser.accesskey "e">
>   <!ENTITY cmdInspectInNewWindow.label "Inspect in New Window">
>+  <!ENTITY cmdInspectInNewWindow.accesskey "N">
Maybe these should be m and N, or n and W?

>   <!ENTITY mnEditPasteReplace.label "In place of (Replace)">
>+  <!ENTITY mnEditPasteReplace.accesskey "I">

>   <!ENTITY mnEditPasteAsParent.label "As parent">
>+  <!ENTITY mnEditPasteAsParent.accesskey "r">
R and n, perhaps?

Comment 2

[Shawn Wilsher :sdwilsh]

(In reply to comment #1)
> >   <!ENTITY cmdInspectBrowser.label "Inspect Contained Document">
> >+  <!ENTITY cmdInspectBrowser.accesskey "e">
> >   <!ENTITY cmdInspectInNewWindow.label "Inspect in New Window">
> >+  <!ENTITY cmdInspectInNewWindow.accesskey "N">
> Maybe these should be m and N, or n and W?
Hmm, justification for e, m, or n for the first one?  I don't really see how any of those relate actually.
As for the second one, N or W works for me (I'm indifferent here).

> >   <!ENTITY mnEditPasteReplace.label "In place of (Replace)">
> >+  <!ENTITY mnEditPasteReplace.accesskey "I">
> 
> >   <!ENTITY mnEditPasteAsParent.label "As parent">
> >+  <!ENTITY mnEditPasteAsParent.accesskey "r">
> R and n, perhaps?
I like R better, and as for the latter, I can't think of a good one.  Justification for r or n?

Comment 3

[(no longer active)]

CCing Neil, and turning his attention to comment 2.

Comment 4

[[e-mail]]

(In reply to comment #2)
>(In reply to comment #1)
>>>   <!ENTITY cmdInspectBrowser.label "Inspect Contained Document">
>>>+  <!ENTITY cmdInspectBrowser.accesskey "e">
>>Maybe these should be m and N, or n and W?
>Hmm, justification for e, m, or n for the first one?  I don't really see how
>any of those relate actually.
I = Insert C = Copy D = Delete and we're supposed to avoid vowels.

>>>   <!ENTITY mnEditPasteAsParent.label "As parent">
>>>+  <!ENTITY mnEditPasteAsParent.accesskey "r">
>>R and n, perhaps?
>Justification for r or n?
n is wider than r, so the underline is more visible.

Comment 5

[Shawn Wilsher :sdwilsh]

Comment on attachment 283699 [details] [diff] [review]
Add the access keys

Well, I'm fairly indifferent here, so you can use your judgement.

r=sdwilsh

Comment 6

[Reed Loden [:reed]]

Checking in extensions/inspector/resources/content/viewers/dom/dom.xul;
/cvsroot/mozilla/extensions/inspector/resources/content/viewers/dom/dom.xul,v  <--  dom.xul
new revision: 1.19; previous revision: 1.18
done
Checking in extensions/inspector/resources/locale/en-US/viewers/dom.dtd;
/cvsroot/mozilla/extensions/inspector/resources/locale/en-US/viewers/dom.dtd,v  <--  dom.dtd
new revision: 1.17; previous revision: 1.16
done

Comment 7

[Reed Loden [:reed]]

Next time, please unbitrot your over two-month-old patches before requesting approval / checkin-needed. Thanks.

Comment 8

[(no longer active)]

(In reply to comment #7)
> Next time, please unbitrot your over two-month-old patches before requesting
> approval / checkin-needed. Thanks.

You're right, sorry about the problem.

Comment 9

[Vlado Valastiak [:wladow]]

There is a bug here, see this part of patch

+          <menuitem id="mnEditPasteFirstChild"
+                    label="&mnEditPasteFirstChild.label;"
+                    accesskey="&mnEditPasteFirstChild.accesskey;"
+                    command="cmdEditPasteFirstChild"/>
+          <menuitem id="mnEditPasteFirstChild"
+                    label="&mnEditPasteFirstChild.label;"
+                    accesskey="&mnEditPasteFirstChild.accesskey;"
+                    command="cmdEditPasteFirstChild"/>


Doubled menu-item.

Comment 10

[(no longer active)]

(In reply to comment #9)
> There is a bug here, see this part of patch
> 
> +          <menuitem id="mnEditPasteFirstChild"
> +                    label="&mnEditPasteFirstChild.label;"
> +                    accesskey="&mnEditPasteFirstChild.accesskey;"
> +                    command="cmdEditPasteFirstChild"/>
> +          <menuitem id="mnEditPasteFirstChild"
> +                    label="&mnEditPasteFirstChild.label;"
> +                    accesskey="&mnEditPasteFirstChild.accesskey;"
> +                    command="cmdEditPasteFirstChild"/>
> 
> 
> Doubled menu-item.

Oops.  You're right.  Re-opening...  I'm posting a patch right away.

Comment 11

[(no longer active)]

Attachment: Remove the duplicate menu item

Fix the problem mentioned in comment 9 by removing the mistakenly duplicated menu item...

Comment 12

[Reed Loden [:reed]]

Checking in extensions/inspector/resources/content/viewers/dom/dom.xul;
/cvsroot/mozilla/extensions/inspector/resources/content/viewers/dom/dom.xul,v  <--  dom.xul
new revision: 1.20; previous revision: 1.19
done