725259 - ARIA role combobox shouldn't allow aria-valueXXX attributes

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

ARIA combobox doesn't allow aria-valuenow and etc attributes (http://www.w3.org/TR/wai-aria/roles#combobox). 

The behavior was introduced by changeset http://hg.mozilla.org/mozilla-central/rev/a55d93c82a17 which is referred to bug 390129. Unfortunately patch of bug 390129 doesn't contain this change and the bug doesn't provide any description of the change. I think we should revert this part.

Note, neither HTML combobox nor XUL combobox don't implement nsIAccessibleValue interface which is consequence of aria-value... attributes.

Comment 1

[alexander :surkov (:asurkov)]

to fix this bug you should turn off corresponding flag of combobox role (http://mxr.mozilla.org/mozilla-central/source/accessible/src/base/nsARIAMap.cpp#139)

fix mochitests if they are depended on this behavior

Comment 2

[Mark Capella [:capella]]

Attachment: nsAriaMap DIFF (v1)

This looks fairly straight-forward ... 

The only relevant mochitest I found was test_valuechange.html ... (3) sliders, a scrollbar and a combobox ... the combobox had no aria-value's

I got a good local build ... and mochitests ran fine otherwise.

Comment 3

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

Please add a test that breaks without your patch and passes with your patch.

Comment 4

[alexander :surkov (:asurkov)]

(In reply to David Bolter [:davidb] from comment #3)
> Please add a test that breaks without your patch and passes with your patch.

that'd be nice but personally I ask for mochitest when something wasn't presented but it should be and I'm more polite when something was presented but it shouldn't. If we start to test ARIA for things that shouldn't be there then I bet it must be a big work and it doesn't have a critical value if the presented wrong behavior doesn't confuse ATs.

Comment 5

[Mark Capella [:capella]]

Comment on attachment 600023 [details] [diff] [review]
nsAriaMap DIFF (v1)

Asking for review+ if that's the case ... then I can do a push to try server via autoland ...

Comment 6

[alexander :surkov (:asurkov)]

Comment on attachment 600023 [details] [diff] [review]
nsAriaMap DIFF (v1)

let's ask David for review since he had a concern

Comment 7

[alexander :surkov (:asurkov)]

(In reply to Mark Capella [:capella] from comment #5)
> Comment on attachment 600023 [details] [diff] [review]
> nsAriaMap DIFF (v1)
> 
> Asking for review+ if that's the case ... then I can do a push to try server
> via autoland ...

usually I do try server build before asking for review to ensure the patch works :)

Comment 8

[Mark Capella [:capella]]

I haven't gotten L1 access yet, so I can only do an autoland to TRY by first getting a review+ from an L2 or above ... or (of course) having someone do it for me  :-)

Comment 9

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

Comment on attachment 600023 [details] [diff] [review]
nsAriaMap DIFF (v1)

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

r=me

Mochitests ran clean locally.
I agree with comment 4.

Thanks for the patch!

Comment 10

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/03fff1f8fa79

Comment 11

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/mozilla-central/rev/03fff1f8fa79