187208 - give more information for menu

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Bolian Yin]

Give information such as:
  1. Is it submenu
  2. It shortcut
  3. It is AccessKey

Comment 1

[Kyle Yuan]

mass changing QA contact to Jessie Li.

Comment 2

[Simford]

Attachment: patch

seek r= & sr=

Comment 3

[Kyle Yuan]

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

>+NS_IMETHODIMP nsXULMenuitemAccessible::GetAccKeybinding(nsAString& _retval)
>+{
>+  nsCOMPtr<nsIDOMElement> elt(do_QueryInterface(mDOMNode));
>+  if (elt) {
>+    nsAutoString accesskey;
>+    elt->GetAttribute(NS_LITERAL_STRING("acceltext"), accesskey);
>+    if (accesskey.IsEmpty())
>+      return NS_OK;
>+
>+    nsAutoString Shortcut;
>+    PRInt32 oldpos, pos=0;
Please use interCap variable name here, and make the first letter lower-case.
Same with the rest part.

>+
>+    while ( (pos != -1) && (pos < (PRInt32)accesskey.Length()) ) {
You don't need the space between the parentheses.

>+      oldpos = pos;
>+      nsAutoString subString;
>+      pos = accesskey.FindChar('+', oldpos);
>+      if (pos == -1) {
>+        accesskey.Mid(subString, oldpos, accesskey.Length() - oldpos);
>+        Shortcut += subString;
>+      }
>+      else {
>+        accesskey.Mid(subString, oldpos, pos - oldpos);
>+        Shortcut += NS_LITERAL_STRING("<") + subString + NS_LITERAL_STRING(">");
>+        pos++;
>+      }
>+    }
I'd like to see this transformation takes place in thr mai code, it's platform
specific.

> const gchar *
> MaiInterfaceAction::GetKeybinding(gint aActionIndex)
> {
>+    //return all Keybindings including accesskey and shortcut
>+    //
Don't need the empty comment line

>+    nsAutoString autoStr;
I prefer a better name for this, such as keyShortcut. 

>+    nsresult rv = accessible->GetAccKeyboardShortcut(autoStr);
>+    if (NS_FAILED(rv))
>+        return NULL;
>+
Here you should check whether the |autoStr| is empty, as well as
|grandparentKey|.

>+
>+    nsAutoString keyBinding, shortCut;
>+    accessible->GetAccKeybinding(keyBinding);
Need a null checking.

>+    shortCut = autoStr + NS_LITERAL_STRING(";") + keyBinding;
>+

Comment 4

[Simford]

Attachment: patch 2

new patch due to comment #3

Comment 5

[Kyle Yuan]

Comment on attachment 110748 [details] [diff] [review]
patch 2

>+    //get accesskey
>+    nsAutoString accessKey;
>+    nsresult rv = accessible->GetAccKeyboardShortcut(accessKey);
>+    if (NS_FAILED(rv) || accessKey.IsEmpty())
>+        return NULL;
You shouldn't return here, in case of some menu items have a shortcut key, but
no accesskey.

>+    allKeybinding = allKeybinding + NS_LITERAL_STRING(";") + subShortcut;
allKeybinding += NS_LITERAL_STRING(";") + subShortcut; is better

other part looks good to me.

BTW, currently, Aaron is not the right person to ask sr= (I hope he will be
soon :), please choose a sr from http://www.mozilla.org/hacking/reviewers.html

Comment 6

[timeless]

Comment on attachment 110748 [details] [diff] [review]
patch 2

aaronl is not a super reviewer.

Comment 7

[Simford]

Attachment: patch 3

new patch due to comment #5

Comment 8

[Brian Ryner (not reading)]

Comment on attachment 110759 [details] [diff] [review]
patch 3

>+    //get accesskey
>+    nsAutoString accessKey;
>+    nsresult rv = accessible->GetAccKeyboardShortcut(accessKey);
>+
>+    if (!NS_FAILED(rv) && !accessKey.IsEmpty()) {

Use NS_SUCCEEDED(rv) instead of !NS_FAILED(rv) here, and also later in the
patch.

sr=bryner with those changes.

Comment 9

[Simford]

Attachment: new patch due to comment #8

Thanks bryner!

Comment 10

[Kyle Yuan]

checked in!