640707 - aria-sort should fire attributechange

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

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

Request from the wild. Patch coming.

Comment 1

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

Attachment: patch

Comment 2

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

Note I created a separate test file for this instead of merging attr change tests because of our coalescence :)

Comment 3

[alexander :surkov (:asurkov)]

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] (NeedInfo me for attention)]

(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 (:asurkov)]

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 (:asurkov)]

David, is this patch ready taking into account comments 3-5 concerning to mochitest organization?

Comment 7

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

(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 (:asurkov)]

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] (NeedInfo me for attention)]

Attachment: patch to land (combined tests)

Found some time on the plane on Sunday :)

Comment 10

[alexander :surkov (:asurkov)]

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]]

Ping? This is needed for full accessibility of the new version of Y! Mail.

Comment 12

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

(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] (NeedInfo me for attention)]

Landed on central: http://hg.mozilla.org/mozilla-central/rev/43fa778d3f1b
Should see this in FF6.

Comment 14

[Eric Shepherd [:sheppy]]

Mentioned on Firefox 6 for developers.