Closed Bug 249292 Opened 16 years ago Closed 11 years ago

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

Categories

(Core :: Disability Access APIs, defect, minor)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla1.9alpha1

People

(Reporter: pkwarren, Assigned: surkov)

References

(Blocks 2 open bugs)

Details

(Keywords: access)

Attachments

(1 file)

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.
Assignee: aaronleventhal → pkwarren
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 ;-)
Assignee: pkwarren → aaronleventhal
Assignee: aaronleventhal → pilgrim
Target Milestone: --- → mozilla1.9alpha
Blocks: keya11y
Keywords: access
Blocks: xula11y
No longer blocks: keya11y
Summary: Extend support for <toolbarbutton> types "menu" and "menu-button" → Ensure accessible children for <toolbarbutton> types "menu" and "menu-button"
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.

Attached patch patchSplinter Review
Assignee: pilgrim → surkov.alexander
Status: NEW → ASSIGNED
Attachment #410730 - Flags: review?(marco.zehe)
Attachment #410730 - Flags: review?(bolterbugz)
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.
Attachment #410730 - Flags: review?(marco.zehe) → review+
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.
Attachment #410730 - Flags: review?(bolterbugz) → review+
(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
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Blocks: treea11y
You need to log in before you can comment on or make changes to this bug.