Last Comment Bug 725259 - ARIA role combobox shouldn't allow aria-valueXXX attributes
: ARIA role combobox shouldn't allow aria-valueXXX attributes
Status: RESOLVED FIXED
[good first bug][mentor=surkov.alexan...
: access
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla13
Assigned To: Mark Capella [:capella]
:
Mentors:
Depends on:
Blocks: aria
  Show dependency treegraph
 
Reported: 2012-02-08 03:16 PST by alexander :surkov
Modified: 2012-02-25 02:35 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
nsAriaMap DIFF (v1) (544 bytes, patch)
2012-02-23 07:53 PST, Mark Capella [:capella]
dbolter: review+
surkov.alexander: feedback+
Details | Diff | Splinter Review

Description alexander :surkov 2012-02-08 03:16:30 PST
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 2012-02-08 03:18:22 PST
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] 2012-02-23 07:53:06 PST
Created attachment 600023 [details] [diff] [review]
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] ***PTO until 29th*** 2012-02-23 08:34:09 PST
Please add a test that breaks without your patch and passes with your patch.
Comment 4 alexander :surkov 2012-02-23 20:21:15 PST
(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] 2012-02-23 20:27:51 PST
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 2012-02-23 20:34:58 PST
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 2012-02-23 20:35:38 PST
(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] 2012-02-23 20:38:28 PST
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] ***PTO until 29th*** 2012-02-24 09:53:29 PST
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 11 Marco Bonardo [::mak] 2012-02-25 02:35:15 PST
https://hg.mozilla.org/mozilla-central/rev/03fff1f8fa79

Note You need to log in before you can comment on or make changes to this bug.