249292 - Ensure accessible children for <toolbarbutton> types "menu" and "menu-button"

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Philip K. Warren]

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.

Comment 1

[Aaron Leventhal]

Sounds like a good plan.

Comment 2

[[e-mail]]

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

Comment 3

[[e-mail]]

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

Comment 4

[Aaron Leventhal]

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.

Comment 5

[Aaron Leventhal]

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 6

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 7

[Marco Zehe (:MarcoZ)]

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.

Comment 8

[alexander :surkov (:asurkov)]

(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 9

[Marco Zehe (:MarcoZ)]

Comment on attachment 410730 [details] [diff] [review]
patch

r=me. Screen readers still work correctly with these changes.

Comment 10

[alexander :surkov (:asurkov)]

try server build - http://build.mozilla.org/tryserver-builds/[e-mail]-try-095a7930fb3f

Comment 11

[David Bolter [:davidb] (NeedInfo me for attention)]

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)

Comment 12

[alexander :surkov (:asurkov)]

(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?

Comment 13

[David Bolter [:davidb] (NeedInfo me for attention)]

(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 14

[David Bolter [:davidb] (NeedInfo me for attention)]

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.

Comment 15

[alexander :surkov (:asurkov)]

(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>

Comment 16

[alexander :surkov (:asurkov)]

landed on 1.9.3 with David's addressed comments