277750 - Menu exposes 2-level accessible.

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Louie Zhao]

By AT-Poke, there are 2-level accessible for menu. One is accessible of XUL
menu; the other is accessible of XUL menupopup. From the point of view of end
user, we should keep only one, and the children of menu accessible should be
menu item accessible.

Comment 1

[Louie Zhao]

Attachment: patch v1

Comment 2

[Philip K. Warren]

*** Bug 249104 has been marked as a duplicate of this bug. ***

Comment 3

[Philip K. Warren]

(In reply to comment #0)
> By AT-Poke, there are 2-level accessible for menu. One is accessible of XUL
> menu; the other is accessible of XUL menupopup. From the point of view of end
> user, we should keep only one, and the children of menu accessible should be
> menu item accessible.

I have tested this patch on a build from today, and I still see two levels of
menus in at-poke. Does this patch include all of the changes to fix this problem?

Comment 4

[Louie Zhao]

Attachment: patch v2

Sorry for missing the fix in xpfe directory. After some testing, I found the
patches for menu/popup menu in my hand are hard to verify against the current
trunk. New problems may come out after applying the patch. I am not very clear
about the reason. It may be blocked by other fixes, or due to the difference
between 1.7 branch and trunk.

This patch is posted just for refernece. It don't need to be "reviewed". I'll
take care of this one after finishing other porting.

Comment 5

[Ginn Chen]

*** Bug 319752 has been marked as a duplicate of this bug. ***

Comment 6

[Ginn Chen]

Attachment: port xpfe part of bug 257099 to toolkit

Most of patch v2 were already commited by Bug 257099.
Need to port xpfe part to toolkit/content/widgets

Comment 7

[Aaron Leventhal]

Comment on attachment 205708 [details] [diff] [review]
port xpfe part of bug 257099 to toolkit

Needs to be reviewed by toolkit peer as well. Neil is both an sr= and a toolkitp peer, so he's a good choice.

Comment 8

[[e-mail]]

Comment on attachment 205708 [details] [diff] [review]
port xpfe part of bug 257099 to toolkit

>+            if (this.localName == "popup")
>               return (this.parentNode.localName == "menulist")? accService.createXULSelectListAccessible(this): accService.createXULMenupopupAccessible(this);
>+            //
>+            // Create accessible for menupopup when parent node is:
>+            //   1. menulist: for combobox to visit its menu items
>+            //   2. toolbarbutton: for which has menupopup child
>+            // Otherwise, won't create accessible for menupopup, e.g.
>+            //   1. menu: to avoid redundant accessible for menu
>+            //   2. other situations ...
>+            //
>+            if (this.localName == "menupopup")
>+              return ((this.parentNode.localName == "menulist") ||
>+                      (this.parentNode.localName == "toolbarbutton")) ?
>+                     accService.createXULMenupopupAccessible(this): null;
I'm confused. What difference does it make if this is a popup or a menupopup? Apart from tooltips, there are several ways (menu)popups get used:
a) attached to a parent menu, in case I understand that you want to return null
b) attached to a parent menulist, which has a special meaning as a selection list
c) attached to a (toolbar)button, colour picker, column picker
d) not specifically attached to anything - used as a context menu
Note that it is difficult to distinguish between c) and d) especially when comparing (say) <button type="menu"> with <button context="_child">.

Needless to say, if this patch gets changed to clarify the picture then the xpfe version should get changed too!

Comment 9

[Ginn Chen]

Attachment: patch v3

only care about the case when parent node is menu at this moment

Comment 10

[[e-mail]]

Comment on attachment 206838 [details] [diff] [review]
patch v3

>+            // Won't create accessible for (menu)popup if parent node is menu
>+            // to avoid redundant accessible for menu
>+            if (this.localName == "popup" || this.localName == "menupopup")
>+              return (this.parentNode.localName != "menu")?
>                      accService.createXULMenupopupAccessible(this): null;
>             else if (this.localName == "tooltip")
>               return accService.createXULTooltipAccessible(this);
>             return null;
Minor nit: ideally have a space before the ?

I feel sure it's technically possible to rearrange the conditions to avoid having to use the ?: (and indeed there should not be an else after the first return) but thinking about it further I guess tooltip should have its own <implementation><property> anyway so don't bother.

Comment 11

[Ginn Chen]

Comment on attachment 206838 [details] [diff] [review]
patch v3

Sync Firefox and Seamonkey.
Risk is low.

Comment 12

[Dan Kinnunen]

Aaron,
     I have tested the following bug 277750 for any regress on the levels below & updated the bug in bugzilla.

                     FF 3  1.6a1  build date 03/12/06
                    JAWS beta 7.10.245  03/10/06
                     Window Eyes 5.5 beta

   The areas tested were within Firefox 3 are:
1. combo boxes in the UI
2. context menus
3. main menu bar menus

Comment 13

[Ginn Chen]

Checking in toolkit/content/widgets/menu.xml;
/cvsroot/mozilla/toolkit/content/widgets/menu.xml,v  <--  menu.xml
new revision: 1.9.2.3; previous revision: 1.9.2.2
done
Checking in toolkit/content/widgets/popup.xml;
/cvsroot/mozilla/toolkit/content/widgets/popup.xml,v  <--  popup.xml
new revision: 1.4.52.1; previous revision: 1.4
done
Checking in xpfe/global/resources/content/bindings/popup.xml;
/cvsroot/mozilla/xpfe/global/resources/content/bindings/popup.xml,v  <--  popup.xml
new revision: 1.27.8.1; previous revision: 1.27
done

Comment 14

[Aaron Leventhal]

What end-user problem did this fix?

It caused a problem for MSAA. Since there is now no accessible for the menus in the menubar, there is no accessible to fire a menupopupstart/menupopupend event and thus nothing gets fired.

Comment 15

[Aaron Leventhal]

It appears that MSAA and ATK unfortunately structure menus differently:

ATK:
File ( role = menu)
   New (role = menu item)
   Open (role = menu item)

MSAA:
File (role = menuitem)
   File (role = menupopup)
      New (role = menu item)
      Open (role = menu item)

Comment 16

[Ginn Chen]

Without this patch, user have to click twice for the first level of menu in GOK
i.e.
Menus->File->File->Quit