Closed Bug 725259 Opened 10 years ago Closed 9 years ago

ARIA role combobox shouldn't allow aria-valueXXX attributes

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla13

People

(Reporter: surkov, Assigned: capella)

References

(Blocks 1 open bug)

Details

(Keywords: access, Whiteboard: [good first bug][mentor=surkov.alexander@gmail.com][lang=c++])

Attachments

(1 file)

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.
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
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
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.
Attachment #600023 - Flags: feedback?(surkov.alexander)
Please add a test that breaks without your patch and passes with your patch.
Attachment #600023 - Flags: feedback?(surkov.alexander) → feedback+
(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 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 ...
Attachment #600023 - Flags: review?(surkov.alexander)
Comment on attachment 600023 [details] [diff] [review]
nsAriaMap DIFF (v1)

let's ask David for review since he had a concern
Attachment #600023 - Flags: review?(surkov.alexander) → review?(bolterbugz)
(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 :)
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 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!
Attachment #600023 - Flags: review?(bolterbugz) → review+
https://hg.mozilla.org/mozilla-central/rev/03fff1f8fa79
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in before you can comment on or make changes to this bug.