989958 - A html:button element with aria-pressed="true" does not expose pressed state

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect)

Description

[Marco Zehe (:MarcoZ)]

STR:
1. Go to http://heydonworks.com/practical_aria_examples/#toolbar-widget
2. Look at the properties for the "A to Z" and "Z to A" buttons.
They are html:button elements, one with aria-pressed="true", the other with aria-pressed="false".

Expected: An IA2 role of ToggleButton and a "pressed" state should be exposed on the button that has aria-pressed="true", by default to "A to Z" button in the example above.

Actual: The role of ToggleButton is exposed, the "pressed" state is not.

Additional info: Adding role="button" fixes this, but role="button" should not be necessary because html:button is a valid native element for a button.

Comment 1

[Marco Zehe (:MarcoZ)]

Attachment: 989958.diff

1. Add a button generic type.
2. Expose it on ARIA, HTML, and XUL buttons.
3. Make aria-pressed="true" expose states::PRESSED.
4. Remove states::checkable from ARIA toggle buttons (aria-pressed attribute) AKA fix bug 825114.
5. Adjust AccessFu's OutputGenerator to take these changes into account.
6. Add and fix tests.
7. And this is not an April fool's joke! ;)

Comment 2

[Marco Zehe (:MarcoZ)]

Comment on attachment 8400038 [details] [diff] [review]
989958.diff

Adding Eitan for the AccessFu bits.

Comment 3

[alexander :surkov (:asurkov)]

Comment on attachment 8400038 [details] [diff] [review]
989958.diff

Review of attachment 8400038 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/generic/Accessible.cpp
@@ +1577,5 @@
> +  if (IsButton()) {
> +    if (mContent->AttrValueIs(kNameSpaceID_None,
> +                              nsGkAtoms::aria_pressed,
> +                              nsGkAtoms::_true, eCaseMatters)) {
> +      *aState |= states::PRESSED;

you can do aria::MapToState instead, also it's good to remove eARIAPressed from role map to avoid double processing

::: accessible/src/generic/DocAccessible.cpp
@@ +1065,5 @@
>                       aAccessible);
>  
>    nsIContent* elm = aAccessible->GetContent();
> +  if (!elm->HasAttr(kNameSpaceID_None, nsGkAtoms::role) &&
> +      !aAccessible->IsButton()) {

you know I would remove role check at all, either way (with or without check) we don't follow the spec here (but IsButton() check would move where appropriate)

::: accessible/src/xul/XULFormControlAccessible.cpp
@@ +44,1 @@
>      mGenericTypes |= eMenuButton;

is it wanted that aria-pressed aren't applied to menu buttons?

Comment 4

[Eitan Isaacson [:eeejay]]

Comment on attachment 8400038 [details] [diff] [review]
989958.diff

Review of attachment 8400038 [details] [diff] [review]:
-----------------------------------------------------------------

From an AccessFu perspective this patch looks good. My only concern is the inconsistency in the braille display when a toggle button is pressed. If it is untoggled, the user would press the router key, and then the whole display jumps 4 cells forward to make room for the "(x)" indicator. Is there something more subtler and friendly we could do here?

::: accessible/tests/mochitest/jsat/test_output.html
@@ +330,5 @@
>          },{
>            accOrElmOrID: "togglebutton_notpressed",
> +          expectedUtterance: [["toggle button", "I ain't pressed"],
> +                              ["I ain't pressed", "toggle button"]],
> +          expectedBraille: [["I ain't pressed"],

It seems strange to me that braille will get a toggle indicator only when it is pressed. Is there another style that denotes toggle buttons instead of check boxes?

Comment 5

[Marco Zehe (:MarcoZ)]

(In reply to alexander :surkov from comment #3)
> you can do aria::MapToState instead, also it's good to remove eARIAPressed
> from role map to avoid double processing

Thanks, will do!

> ::: accessible/src/generic/DocAccessible.cpp
> @@ +1065,5 @@
> >                       aAccessible);
> >  
> >    nsIContent* elm = aAccessible->GetContent();
> > +  if (!elm->HasAttr(kNameSpaceID_None, nsGkAtoms::role) &&
> > +      !aAccessible->IsButton()) {
> 
> you know I would remove role check at all, either way (with or without
> check) we don't follow the spec here (but IsButton() check would move where
> appropriate)

The check would then no longer be necessary either. Processing would just continue and see if the changing of attributes affects *any* node. So, I would just remove that wole block.

> is it wanted that aria-pressed aren't applied to menu buttons?

If a menu button is pressed, the menu is usually active, and focus is inside it. So there is not much point in applying aria-pressed to it really. Besides, we always morph a button that has the aria-pressed state to become a toggle button. Similarly, we morph a button to a menu button if aria-haspopup is present if I'm not mistaken. So these sort of exclude one another.

Comment 6

[Marco Zehe (:MarcoZ)]

(In reply to Eitan Isaacson [:eeejay] from comment #4)
> From an AccessFu perspective this patch looks good. My only concern is the
> inconsistency in the braille display when a toggle button is pressed. If it
> is untoggled, the user would press the router key, and then the whole
> display jumps 4 cells forward to make room for the "(x)" indicator. Is there
> something more subtler and friendly we could do here?

Not sure. We're basically venturing into largely uncharted territory. Toggle buttons aren't that common in Windows, for example, so screen readers haven't really figured out how to deal with them in braille. Some over-speak via speech, for example, like we did until this patch, and in braille, they all look similar.

> It seems strange to me that braille will get a toggle indicator only when it
> is pressed. Is there another style that denotes toggle buttons instead of
> check boxes?

Nothing standardized or commonly accepted that I know of. I could make it a bit more subtle than a checkbox, to denote the difference, and make it show always I think. Problem is *what* to choose without seriously impacting the meaning. Using the symbols we have is already pretty subtle in terms of the weight of the braille dots used.

Comment 7

[Eitan Isaacson [:eeejay]]

(In reply to Marco Zehe (:MarcoZ) from comment #6)
> Nothing standardized or commonly accepted that I know of. I could make it a
> bit more subtle than a checkbox, to denote the difference, and make it show
> always I think. Problem is *what* to choose without seriously impacting the
> meaning. Using the symbols we have is already pretty subtle in terms of the
> weight of the braille dots used.

What about leaving the braille bits alone and have it look like a checkbox?

Comment 8

[Marco Zehe (:MarcoZ)]

(In reply to Eitan Isaacson [:eeejay] from comment #7)
> What about leaving the braille bits alone and have it look like a checkbox?

Yeah, I'll just do that, then. I need to touch the braille bits since they previously relied on pressed items being checkable, but I'll just make changes so the tests include a checked and non-checked braille indicator *and* pass. ;)

Comment 9

[Marco Zehe (:MarcoZ)]

Attachment: Bug 989958 Patch, take 2

Addressed review comments.

Comment 10

[Marco Zehe (:MarcoZ)]

Comment on attachment 8400552 [details] [diff] [review]
Bug 989958 Patch, take 2

re-requesting review from Eitan as well since I needed to adjust the code a bit to make braille work as discussed for toggle buttons. Note that as per spec, toggle buttons are the only ones that actually get a "pressed" state, but aren't technically checkable (see bug 825114). Hence all these changes in the first place.

Comment 11

[alexander :surkov (:asurkov)]

(In reply to Marco Zehe (:MarcoZ) from comment #5)

> > you know I would remove role check at all, either way (with or without
> > check) we don't follow the spec here (but IsButton() check would move where
> > appropriate)
> 
> The check would then no longer be necessary either. Processing would just
> continue and see if the changing of attributes affects *any* node. So, I
> would just remove that wole block.

nah, it's technically wrong to apply aria-pressed to any role, it doesn't go with spec. So we need to have IsButton() check for aria-pressed.

> > is it wanted that aria-pressed aren't applied to menu buttons?
> 
> If a menu button is pressed, the menu is usually active, and focus is inside
> it. So there is not much point in applying aria-pressed to it really.
> Besides, we always morph a button that has the aria-pressed state to become
> a toggle button. Similarly, we morph a button to a menu button if
> aria-haspopup is present if I'm not mistaken. So these sort of exclude one
> another.

ok, I don't mind. If we need it later then we can add it

Comment 12

[alexander :surkov (:asurkov)]

Comment on attachment 8400552 [details] [diff] [review]
Bug 989958 Patch, take 2

Review of attachment 8400552 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comments addressed/fixed

::: accessible/src/base/ARIAMap.cpp
@@ +85,5 @@
>      kUseMapRole,
>      eNoValue,
>      ePressAction,
>      eNoLiveAttr,
> +    eButton,

I'm curious if it's correct to consider "key" role as a button

@@ +86,5 @@
>      eNoValue,
>      ePressAction,
>      eNoLiveAttr,
> +    eButton,
> +    kNoReqStates

it'd be good to have a comment here like

// eARIAPressed is auto applied on any button

::: accessible/src/generic/DocAccessible.cpp
@@ +1069,2 @@
>    if (aAttribute == nsGkAtoms::aria_checked ||
>        aAttribute == nsGkAtoms::aria_pressed) {

pls add IsButton() check for aria-pressed

Comment 13

[alexander :surkov (:asurkov)]

(In reply to alexander :surkov from comment #12)

> r=me with comments addressed/fixed

and btw thanks for the awesome work

Comment 14

[Marco Zehe (:MarcoZ)]

(In reply to alexander :surkov from comment #12)
> ::: accessible/src/base/ARIAMap.cpp
> @@ +85,5 @@
> >      kUseMapRole,
> >      eNoValue,
> >      ePressAction,
> >      eNoLiveAttr,
> > +    eButton,
> 
> I'm curious if it's correct to consider "key" role as a button

No, I don't think so. The "key" role was specifically invented for on-screen keyboards such as that on a Firefox OS device, and keys cannot be turned into toggle buttons. aria-pressed may get a special meaning for the role of "key", for example when considering locking the shift key so that it becomes CAPS Lock. But that's for a different bug and a different strategy.

> @@ +86,5 @@
> >      eNoValue,
> >      ePressAction,
> >      eNoLiveAttr,
> > +    eButton,
> > +    kNoReqStates
> 
> it'd be good to have a comment here like
> 
> // eARIAPressed is auto applied on any button

Done!

> ::: accessible/src/generic/DocAccessible.cpp
> @@ +1069,2 @@
> >    if (aAttribute == nsGkAtoms::aria_checked ||
> >        aAttribute == nsGkAtoms::aria_pressed) {
> 
> pls add IsButton() check for aria-pressed

Done!

Comment 15

[Marco Zehe (:MarcoZ)]

Updated patch pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=d0dacc8d66d4

Comment 16

[Eitan Isaacson [:eeejay]]

Comment on attachment 8400552 [details] [diff] [review]
Bug 989958 Patch, take 2

Review of attachment 8400552 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with one change below.

::: accessible/src/jsat/OutputGenerator.jsm
@@ +851,5 @@
>  
> +    let getCheckedState = function getCheckedState() {
> +      let state = aState;
> +      return getResultMarker(state.contains(States.CHECKED));
> +    };

You could get rid of this function.

@@ +857,2 @@
>      if (aState.contains(States.CHECKABLE)) {
>        stateBraille.push(getCheckedState());

.. and just do:
stateBraille.push(getResultMarker(aState.contains(States.CHECKED)));

Comment 17

[Marco Zehe (:MarcoZ)]

Pushed with Eitan's comment addressed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1f9fc68fcf4

(Note that this push also fixes bug 825114).

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f1f9fc68fcf4

Comment 19

[Marco Zehe (:MarcoZ)]

Verified this landed correctly in Mozilla/5.0 (Windows NT 6.3; WOW64; rv:31.0) Gecko/20100101 Firefox/31.0