482258 - ARIA controls with STATE_MIXED must not have STATE_CHECKED set also

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Marco Zehe (:MarcoZ)]

Right now, ARIA controls with aria-checked="mixed" get STATE_CHECKED set. This was uncovered in fix for bug 477975 through the change in nsAccessible::GetActionName, where "uncheck" would be exposed on something that was "only" aria-checked="mixed".

Comment 1

[Marco Zehe (:MarcoZ)]

Attachment: Mochitest

Comment 2

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

Attachment: patch 1

Marco, thanks for the tests, which pass now. I think this is the right fix. An alternative would be to check for ! "true", but that would miss some common (mis)use cases like checked="checked" etc. This fix is slightly less performant, but more robust.

Comment 3

[Marco Zehe (:MarcoZ)]

Comment on attachment 366325 [details] [diff] [review]
patch 1

r=me, thanks!

Comment 4

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

Attachment: patch to go in (includes small perf tweak for bug 477975)

Marco, push at will :)  (Suite passes for me)

Comment 5

[alexander :surkov (:asurkov)]

Comment on attachment 366332 [details] [diff] [review]
patch to go in (includes small perf tweak for bug 477975)


>    case eCheckUncheckAction:
>-     if (states & nsIAccessibleStates::STATE_MIXED)
>+     if (states & nsIAccessibleStates::STATE_CHECKED)
>+       aName.AssignLiteral("uncheck");
>+     else if (states & nsIAccessibleStates::STATE_MIXED)
>        aName.AssignLiteral("cycle");
>-     else if (states & nsIAccessibleStates::STATE_CHECKED)
>-       aName.AssignLiteral("uncheck");

if I get right this is perf twak, what is this more performant?

Comment 6

[alexander :surkov (:asurkov)]

Comment on attachment 366332 [details] [diff] [review]
patch to go in (includes small perf tweak for bug 477975)


>     if (aStateMapEntry->attributeValue == kBoolState) {
>       // No attribute value map specified in state map entry indicates state cleared
>-      if (attribValue.EqualsLiteral("false")) {
>+      if (attribValue.EqualsLiteral("false") ||
>+          attribValue.EqualsLiteral("mixed")) {
>         *aStateInOut &= ~aStateMapEntry->state;

it doesn't go well with ARIAMap:

{&nsAccessibilityAtoms::aria_checked, kBoolState, nsIAccessibleStates::STATE_CHECKED},
{&nsAccessibilityAtoms::aria_checked, "mixed", nsIAccessibleStates::STATE_MIXED},

I would suggest to rethink this approach.

Btw, who set STATE_CHECKED so that we are forced to remove this state?

Comment 7

[Marco Zehe (:MarcoZ)]

Pushed on Davidb's behalf in changeset:
http://hg.mozilla.org/mozilla-central/rev/9e485bb3604e