Menu exposes 2-level accessible.

RESOLVED FIXED

Status

()

RESOLVED FIXED
14 years ago
13 years ago

People

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

Tracking

({access, fixed1.8.1})

Trunk
x86
Linux
access, fixed1.8.1
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: sunport17)

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

14 years ago
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.
(Reporter)

Comment 1

14 years ago
Created attachment 170810 [details] [diff] [review]
patch v1

Comment 2

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

Comment 3

14 years ago
(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?
(Reporter)

Updated

14 years ago
Attachment #170810 - Attachment is obsolete: true
(Reporter)

Comment 4

14 years ago
Created attachment 170888 [details] [diff] [review]
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.

Updated

14 years ago
Whiteboard: sunport17
(Assignee)

Comment 5

13 years ago
*** Bug 319752 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 6

13 years ago
Created attachment 205708 [details] [diff] [review]
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
Assignee: Louie.Zhao → ginn.chen
Attachment #170888 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #205708 - Flags: review?(aaronleventhal)

Comment 7

13 years ago
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 8

13 years ago
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!
(Assignee)

Comment 9

13 years ago
Created attachment 206838 [details] [diff] [review]
patch v3

only care about the case when parent node is menu at this moment
Attachment #206838 - Flags: superreview?(neil.parkwaycc.co.uk)

Comment 10

13 years ago
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+
(Assignee)

Updated

13 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
(Assignee)

Updated

13 years ago
Attachment #205708 - Flags: superreview?(neil.parkwaycc.co.uk)
(Assignee)

Comment 11

13 years ago
Comment on attachment 206838 [details] [diff] [review]
patch v3

Sync Firefox and Seamonkey.
Risk is low.
Attachment #206838 - Flags: approval1.8.1?

Updated

13 years ago
Attachment #206838 - Flags: approval1.8.1? → branch-1.8.1?(aaronleventhal)
(Assignee)

Updated

13 years ago
Depends on: 328270

Comment 12

13 years ago
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

Updated

13 years ago
Attachment #206838 - Flags: approval-branch-1.8.1?(aaronleventhal) → approval-branch-1.8.1+
Whiteboard: sunport17 → [checkin needed (1.8 branch)] sunport17
(Assignee)

Comment 13

13 years ago
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
(Assignee)

Updated

13 years ago
No longer depends on: 328270

Comment 14

13 years ago
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

13 years ago
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)
(Assignee)

Comment 16

13 years ago
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.