Closed Bug 277750 Opened 16 years ago Closed 15 years ago

Menu exposes 2-level accessible.

Categories

(Core :: Disability Access APIs, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: Louie.Zhao, Assigned: ginnchen+exoracle)

References

Details

(Keywords: access, fixed1.8.1, Whiteboard: sunport17)

Attachments

(2 files, 2 obsolete files)

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.
Attached patch patch v1 (obsolete) — Splinter Review
*** Bug 249104 has been marked as a duplicate of this bug. ***
(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?
Attachment #170810 - Attachment is obsolete: true
Attached patch patch v2 (obsolete) — Splinter Review
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.
Whiteboard: sunport17
*** Bug 319752 has been marked as a duplicate of this bug. ***
Most of patch v2 were already commited by Bug 257099.
Need to port xpfe part to toolkit/content/widgets
Assignee: Louie.Zhao → ginn.chen
Attachment #170888 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #205708 - Flags: review?(aaronleventhal)
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.
Attachment #205708 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #205708 - Flags: review?(aaronleventhal)
Attachment #205708 - Flags: review+
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!
Attached patch patch v3Splinter Review
only care about the case when parent node is menu at this moment
Attachment #206838 - Flags: superreview?(neil.parkwaycc.co.uk)
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.
Attachment #206838 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview+
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Attachment #205708 - Flags: superreview?(neil.parkwaycc.co.uk)
Comment on attachment 206838 [details] [diff] [review]
patch v3

Sync Firefox and Seamonkey.
Risk is low.
Attachment #206838 - Flags: approval1.8.1?
Attachment #206838 - Flags: approval1.8.1? → branch-1.8.1?(aaronleventhal)
Depends on: 328270
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
Attachment #206838 - Flags: approval-branch-1.8.1?(aaronleventhal) → approval-branch-1.8.1+
Whiteboard: sunport17 → [checkin needed (1.8 branch)] sunport17
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
Keywords: access, fixed1.8.1
Whiteboard: [checkin needed (1.8 branch)] sunport17 → sunport17
No longer depends on: 328270
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.

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)
Without this patch, user have to click twice for the first level of menu in GOK
i.e.
Menus->File->File->Quit
You need to log in before you can comment on or make changes to this bug.