Last Comment Bug 640707 - aria-sort should fire attributechange
: aria-sort should fire attributechange
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla6
Assigned To: David Bolter [:davidb]
:
:
Mentors:
Depends on:
Blocks: aria a11ynext 2011ymaila11y
  Show dependency treegraph
 
Reported: 2011-03-10 11:39 PST by David Bolter [:davidb]
Modified: 2011-06-17 10:03 PDT (History)
6 users (show)
dbolter: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (5.42 KB, patch)
2011-03-10 12:06 PST, David Bolter [:davidb]
surkov.alexander: review+
Details | Diff | Splinter Review
patch to land (combined tests) (5.95 KB, patch)
2011-04-06 15:03 PDT, David Bolter [:davidb]
no flags Details | Diff | Splinter Review

Description David Bolter [:davidb] 2011-03-10 11:39:11 PST
Request from the wild. Patch coming.
Comment 1 David Bolter [:davidb] 2011-03-10 12:06:59 PST
Created attachment 518482 [details] [diff] [review]
patch
Comment 2 David Bolter [:davidb] 2011-03-10 12:08:10 PST
Note I created a separate test file for this instead of merging attr change tests because of our coalescence :)
Comment 3 alexander :surkov 2011-03-10 21:06:02 PST
Comment on attachment 518482 [details] [diff] [review]
patch


> 		test_aria_hidden.html \
> 		test_aria_menu.html \
>+		test_aria_sort.html \

perhaps you should combine with aria_hidden and name it test_objattrchange.html?
Comment 4 David Bolter [:davidb] 2011-03-11 05:38:02 PST
(In reply to comment #3)
> Comment on attachment 518482 [details] [diff] [review]
> patch
> 
> 
> > 		test_aria_hidden.html \
> > 		test_aria_menu.html \
> >+		test_aria_sort.html \
> 
> perhaps you should combine with aria_hidden and name it
> test_objattrchange.html?

I tried exactly that (same name even) locally before posting the patch. I wasn't getting the both events, which I assumed to be our coalescence? (comment 2)
Comment 5 alexander :surkov 2011-03-11 05:44:49 PST
If you didn't do that in separate invokers and for the same node then yes. Just do it in different invokers.
Comment 6 alexander :surkov 2011-03-22 20:03:27 PDT
David, is this patch ready taking into account comments 3-5 concerning to mochitest organization?
Comment 7 David Bolter [:davidb] 2011-03-23 05:31:43 PDT
(In reply to comment #5)
> If you didn't do that in separate invokers and for the same node then yes. Just
> do it in different invokers.

I had separate invokers. Strange (maybe I forgot a 'var' or something). I'd prefer to land the patch as is, with a follow up; given priorities.
Comment 8 alexander :surkov 2011-03-23 05:46:59 PDT
If we are both agree it makes testsuite cleaner then we should get it fixed. If you don't have time to finish it then you should ask somebody. The fix should be easy.
Comment 9 David Bolter [:davidb] 2011-04-06 15:03:15 PDT
Created attachment 524279 [details] [diff] [review]
patch to land (combined tests)

Found some time on the plane on Sunday :)
Comment 10 alexander :surkov 2011-04-07 00:16:11 PDT
Comment on attachment 524279 [details] [diff] [review]
patch to land (combined tests)


>+      testAttrs("sort", {"sort" : "ascending"}, true);
>       testAttrs("sortAscending", {"sort" : "ascending"}, true);
>+
>+  <div id="sort" role="columnheader" aria-sort="ascending">a column header</div>

why do you need this if you have
<div id="sortAscending" role="columnheader" aria-sort="ascending"></div>

>+      this.getID = function updateSort_getID()
>+      {
>+        return "aria-sort for " + aID + " " + aSort;

perhaps "change aria-sort on " + aSort + " for " + prettyName(aID)?

>+  <div id="sortable" role="columnheader" aria-sort"none">a column</div>

aria-sort="none"
Comment 11 James Teh [:Jamie] 2011-05-16 20:23:06 PDT
Ping? This is needed for full accessibility of the new version of Y! Mail.
Comment 12 David Bolter [:davidb] 2011-05-17 06:49:50 PDT
(In reply to comment #10)
> Comment on attachment 524279 [details] [diff] [review] [review]
> patch to land (combined tests)
> 
> 
> >+      testAttrs("sort", {"sort" : "ascending"}, true);
> >       testAttrs("sortAscending", {"sort" : "ascending"}, true);
> >+
> >+  <div id="sort" role="columnheader" aria-sort="ascending">a column header</div>
> 
> why do you need this if you have
> <div id="sortAscending" role="columnheader" aria-sort="ascending"></div>

Good catch, it is redundant. I've removed it locally.

> >+  <div id="sortable" role="columnheader" aria-sort"none">a column</div>
> 
> aria-sort="none"

Sure, updated locally.

Looking for a clean try run before landing.
Comment 13 David Bolter [:davidb] 2011-05-17 09:38:59 PDT
Landed on central: http://hg.mozilla.org/mozilla-central/rev/43fa778d3f1b
Should see this in FF6.
Comment 14 Eric Shepherd [:sheppy] 2011-06-17 10:03:07 PDT
Mentioned on Firefox 6 for developers.

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