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.

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 on attachment 8400038 [details] [diff] [review] 989958.diff Adding Eitan for the AccessFu bits.

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

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

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

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

(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. ;)

Addressed review comments.

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.

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

(In reply to alexander :surkov from comment #12) > r=me with comments addressed/fixed and btw thanks for the awesome work

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

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

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)));

Pushed with Eitan's comment addressed: https://hg.mozilla.org/integration/mozilla-inbound/rev/f1f9fc68fcf4 (Note that this push also fixes bug 825114).

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