It is currently not possible to view the items under the "Bookmarks" button on the toolbar in our accessibility tools, since it is exposed as a button and and not a menu. The "Bookmarks" button is defined as a <toolbarbutton> and has type="menu". In addition, the Back/Forward buttons are implemented as <toolbarbutton> with type="menu-button". To improve the accessibility of <toolbarbutton>, we probably want to have <toolbarbutton type="menu"> use nsXULMenuitemAccessible instead of nsXULButtonAccessible, and <toolbarbutton type="menu-button"> implement a new accesible subclass of nsXULMenuitemAccessible, with support for two different actions - one to trigger the button and one to display the menu.

Sounds like a good plan.

Don't forget regular buttons can me menubuttons too - see for example the "File" button in the advanced message search window.

Sorry, I meant "be" not "me"... I should try not to type in the dark ;-)

See also bug 363797 -- implementing an action to show or hide the menu. The menupopup child needs to be init'd via something like: NS_IMETHODIMP nsXULButtonAccessible::Init() { nsresult rv = nsAccessibleWrap::Init(); nsXULMenupopupAccessible::GenerateMenu(mDOMNode); return rv; } Also the menupopup for the button will need to implement nsIAccessibleSelectable.

Currently nsXULButtonAccessible overrides CacheChildren() which is causing the menupopup child to not show up in the hierarchy. Also, it needs its text child and inherit from nsHyperTextAccessible, because any button that shows text needs to implement nsIAccessibleText. It's okay if the image anon child and menupopup show up as embedded object chars via nsIAccessibleText::GetText(). Finaly, in order for the menupopup child to implement nsIAccessibleSelectable, the button will have to support nsIDOMXULSelectControlElement, if it's of type menu or menu-button.

Comment on attachment 410730 [details] [diff] [review] patch Can you kick off a try-server build? I'd like to see if this has any impact on screen readers.

(In reply to comment #7) > (From update of attachment 410730 [details] [diff] [review]) > Can you kick off a try-server build? I'd like to see if this has any impact on > screen readers. I would ask you to make build yourself, it will allow to play all controls in mochitest (I assume you have more or less recent build so you won't be forced to rebuild it entirely).

Comment on attachment 410730 [details] [diff] [review] patch r=me. Screen readers still work correctly with these changes.

try server build - http://build.mozilla.org/tryserver-builds/surkov.alexander@gmail.com-try-095a7930fb3f

Comment on attachment 410730 [details] [diff] [review] patch I'll review this in stages. >+++ b/accessible/src/base/nsAccUtils.cpp >@@ -903,17 +903,16 @@ nsAccUtils::MustPrune(nsIAccessible *aAc > PRUint32 role = nsAccUtils::Role(aAccessible); > > return role == nsIAccessibleRole::ROLE_MENUITEM || > role == nsIAccessibleRole::ROLE_COMBOBOX_OPTION || > role == nsIAccessibleRole::ROLE_OPTION || > role == nsIAccessibleRole::ROLE_ENTRY || > role == nsIAccessibleRole::ROLE_FLAT_EQUATION || > role == nsIAccessibleRole::ROLE_PASSWORD_TEXT || >- role == nsIAccessibleRole::ROLE_PUSHBUTTON || Nit: comment somewhere near this code about why PUSHBUTTON is omitted? >+ child->SetAttr(kNameSpaceID_None, nsAccessibilityAtoms::menugenerated, >+ NS_LITERAL_STRING("true"), PR_TRUE); What effect does this have besides setting the attribute? > void nsXULButtonAccessible::CacheChildren() >+ PRBool isMenu = content->AttrValueIs(kNameSpaceID_None, >+ nsAccessibilityAtoms::type, >+ nsAccessibilityAtoms::menu, >+ eCaseMatters); >+ >+ PRBool isMenuButton = isMenu ? >+ PR_FALSE : >+ content->AttrValueIs(kNameSpaceID_None, nsAccessibilityAtoms::type, >+ nsAccessibilityAtoms::menuButton, eCaseMatters); >+ >+ if (!isMenu && !isMenuButton) >+ return; I don't understand this. If the type is menu-button with isMenu be true because the first part of the type attr value is "menu"? (left off here)

(In reply to comment #11) > Nit: comment somewhere near this code about why PUSHBUTTON is omitted? If you think it's necessary then ok. > > >+ child->SetAttr(kNameSpaceID_None, nsAccessibilityAtoms::menugenerated, > >+ NS_LITERAL_STRING("true"), PR_TRUE); > > What effect does this have besides setting the attribute? it's handled by menupopup layout object to create frames for popup children. > I don't understand this. If the type is menu-button with isMenu be true because > the first part of the type attr value is "menu"? Really? I thought AttrValueIs return true if and only if attribute value is matched completely with the compared value. Am I wrong?

(In reply to comment #12) > (In reply to comment #11) > > I don't understand this. If the type is menu-button with isMenu be true because > > the first part of the type attr value is "menu"? > > Really? I thought AttrValueIs return true if and only if attribute value is > matched completely with the compared value. Am I wrong? No that is right; I was misreading your code late at night :)

Comment on attachment 410730 [details] [diff] [review] patch r=me. Thanks for creating the tree tests... should ease my mind with the bug 525569 work. General comment: in english "nsCoreUtils::GeneratePopup" strongly suggests that something is "popping up" visually. Maybe we should keep "GenerateMenu" or to keep it closer to the actual role (like you did with "Popup"), maybe "GeneratePopupStructure"? >+//////////////////////////////////////////////////////////////////////////////// >+// nsXULButtonAccessible: nsAccessible protected > > void nsXULButtonAccessible::CacheChildren() > { > // An XUL button accessible may have 1 child dropmarker accessible Please update this comment while you're here. >+ if (buttonAccessible) { >+ if (menupopupAcc) >+ menupopupAcc->SetNextSibling(buttonAccessible); Can you put a comment near here that menupopupAcc is the dropmarker? Otherwise I keep asking myself why the button is the next sibling of a menu.

(In reply to comment #14) > >+ if (buttonAccessible) { > >+ if (menupopupAcc) > >+ menupopupAcc->SetNextSibling(buttonAccessible); > > Can you put a comment near here that menupopupAcc is the dropmarker? Otherwise > I keep asking myself why the button is the next sibling of a menu. You're right when you're asking this. It's not dropmarker button, it's a button. If you have <button type="menu-button"/> then you have a tree <button type="menu-button"/> - if you click on this button (or its dropmarker which we don't expose) then popup appears <button> - if you click on this button then button is clicked (no popup) <popup> </button>

landed on 1.9.3 with David's addressed comments