280920 - Make DHTML menus accessible, if role attribute used

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Aaron Leventhal]

As part of the ongoing DHTML accessibility work, we need special changes to
accomodate DHTML menus.

Comment 1

[Aaron Leventhal]

Attachment: 1) Allow some roles, such as menu items to expose accessible name by collecting text from subtree, 2) Fire menu open/closed events when appropriate

Comment 2

[Aaron Leventhal]

Attachment: 1) Fix leaks and forgotten null check 2) Add comments describing menu event computation, 3) Add loop to deal with deeply nested menupopup objects, in menubar check

Comment 3

[Aaron Leventhal]

Thanks pkw for noticing the leaks and the forgotten null check.

Comment 4

[Philip K. Warren]

Comment on attachment 173294 [details] [diff] [review]
1) Fix leaks and forgotten null check 2) Add comments describing menu event computation, 3) Add loop to deal with deeply nested menupopup objects, in menubar check

>Index: accessible/src/base/nsRootAccessible.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/accessible/src/base/nsRootAccessible.cpp,v
>retrieving revision 1.105
>diff -p -u -5 -r1.105 nsRootAccessible.cpp
>--- accessible/src/base/nsRootAccessible.cpp	1 Feb 2005 08:50:47 -0000	1.105
>+++ accessible/src/base/nsRootAccessible.cpp	3 Feb 2005 18:02:38 -0000
...
>+      while (containerRole == ROLE_MENUPOPUP) {
>+        nsIAccessible *current = mMenuAccessible;
>+        current->GetParent(getter_AddRefs(mMenuAccessible));
>+        if (!mMenuAccessible) {
>+          break;
>+        }
>+        mMenuAccessible->GetRole(&containerRole);        
>+      }
>+      // Will fire EVENT_MENUEND as well if parent of menu is menubar
>+      if (containerRole == ROLE_MENUBAR) {
>+        mMenuAccessible = nsnull;
>+      }

This logic seems reversed. You are only firing EVENT_MENUEND below if
mMenuAccessible is non-null.

>+    while (0 == (state & STATE_MULTISELECTABLE)) {
>+      nsIAccessible *current = container;
>+      current->GetParent(getter_AddRefs(container));
>+      container->GetFinalRole(&containerRole);
>+      if (!container || containerRole == ROLE_PANE) {

This seems wrong. You are getting the parent, calling a function on it, and
then checking if the parent is null.

Comment 5

[Aaron Leventhal]

Attachment: Address pkw's comments

Comment 6

[Philip K. Warren]

Comment on attachment 173296 [details] [diff] [review]
Address pkw's comments

This looks like the patch for another bug.

Comment 7

[Aaron Leventhal]

Attachment: Correct patch

Comment 8

[Johnny Stenback (:jst)]

Comment on attachment 173298 [details] [diff] [review]
Correct patch

sr=jst

Comment 9

[Aaron Leventhal]

Checking in accessible/src/base/nsAccessibilityAtomList.h;
/cvsroot/mozilla/accessible/src/base/nsAccessibilityAtomList.h,v  <-- 
nsAccessibilityAtomList.h
new revision: 1.12; previous revision: 1.11
done
Checking in accessible/src/base/nsAccessible.h;
/cvsroot/mozilla/accessible/src/base/nsAccessible.h,v  <--  nsAccessible.h
new revision: 1.55; previous revision: 1.54
done
Checking in accessible/src/base/nsAccessible.cpp;
/cvsroot/mozilla/accessible/src/base/nsAccessible.cpp,v  <--  nsAccessible.cpp
new revision: 1.127; previous revision: 1.126
done
Checking in accessible/src/base/nsRootAccessible.cpp;
/cvsroot/mozilla/accessible/src/base/nsRootAccessible.cpp,v  <-- 
nsRootAccessible.cpp
new revision: 1.106; previous revision: 1.105
done
Checking in accessible/src/base/nsRootAccessible.h;
/cvsroot/mozilla/accessible/src/base/nsRootAccessible.h,v  <--  nsRootAccessible.h
new revision: 1.44; previous revision: 1.43
done

Comment 10

[Bastiaan Jacques]

(From attachment 173298 [details] [diff] [review]) accessible/src/base/nsRootAccessible.cpp:

+  // Fire focus if it changes, but always fire focus events for menu items
+  PRBool fireFocus = gLastFocusedNode != aNode || (role == ROLE_MENUITEM);

Correct me if I'm wrong, but it appears to me that apart from initialization,
fireFocus is never used.

Comment 11

[Aaron Leventhal]

> Correct me if I'm wrong, but it appears to me that apart from initialization,
> fireFocus is never used.
Hmm, nice catch -- I'll fix that.

Comment 12

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1527381