734982 - Map ARIA role "form".

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

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

For some reason this one is missing from our nsARIAMap.

Comment 1

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

http://www.w3.org/WAI/PF/aria/roles#form

Comment 2

[alexander :surkov (:asurkov)]

spec: http://www.w3.org/WAI/PF/aria/roles#form
aria impl guide: http://www.w3.org/WAI/PF/aria-implementation/#mapping_role_table

add form entry to nsARIAMap.cpp

add a test to test_aria_roles.html and elm/test_landmarks.html

Comment 3

[Mark Capella [:capella]]

Takes it ... could have rolled it in with # 737156 ... separate works also

Comment 4

[Mark Capella [:capella]]

Attachment: Patch (v1)

First try ... up for review for correctness / completeness ...

Comment 5

[Eitan Isaacson [:eeejay]]

Comment on attachment 608550 [details] [diff] [review]
Patch (v1)

This looks correct, thank you!

Comment 6

[Mark Capella [:capella]]

Attachment: Patch (v2)

Added :surkov as review to commit message, re-posted

Comment 7

[alexander :surkov (:asurkov)]

Comment on attachment 608962 [details] [diff] [review]
Patch (v2)

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

::: accessible/tests/mochitest/elm/test_landmarks.html
@@ -28,1 @@
>        testRole("main", ROLE_DOCUMENT);

don't remove empty line, it's used to separate HTML landmarks and ARIA landmarks

@@ +40,5 @@
>        testAttrs("footer", {"tag" : "FOOTER"}, true);
>        testAttrs("article", {"tag" : "ARTICLE"}, true);
>        testAttrs("aside", {"tag" : "ASIDE"}, true);
>        testAttrs("main", {"tag" : "ARTICLE"}, true); // no override expected
> +      testAttrs("form", {"tag" : "ARTICLE"}, true); // no override expected

no idea why that 'no override expected' mean, 'tag' is never gets overridden. pls remove those comments

@@ +74,1 @@
>  

pls refer to a bug number

::: accessible/tests/mochitest/test_aria_roles.html
@@ +34,5 @@
>        testRole("marquee_h1", ROLE_ANIMATION);
>  
>        // strong landmark
>        testRole("application", ROLE_APPLICATION);
> +      testRole("form", ROLE_FORM);

ok, clean dupe of test_landmarks.html but it makes sense to keep them both until we merge these two tests.

@@ -116,5 @@
>    <div role="search" id="search">search</div>
>  
> -  <!-- landmarks are tables -->
> -  <table role="application" id="application_table">application table
> -    <tr><td>hi<td></tr></table>

Bug 469688 - <table role="*"> should expose inner table structure only for weak roles; r=MarcoZ,surkov 

I think David missed the test for this case. I'd be nice if you can add it (in either case pls don't remove it).

Comment 8

[Mark Capella [:capella]]

Attachment: Patch (v3)

Nits addressed - re-requesting review

Comment 9

[alexander :surkov (:asurkov)]

Comment on attachment 609279 [details] [diff] [review]
Patch (v3)

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

since you rolled back the change <table role="application" id="application_table">application table instead of adding a test for it then please file bug for this

::: accessible/tests/mochitest/test_aria_roles.html
@@ +93,5 @@
>    <a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=469688">Mozilla Bug 469688</a>
>    <a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=469688">Mozilla Bug 520188</a>
>    <a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=529289">Mozilla Bug 529289</a>
>    <a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=529289">Mozilla Bug 607219</a>
> +  <a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=735645">Mozilla Bug 735645</a>

please put @title attribute on it

Comment 10

[alexander :surkov (:asurkov)]

(In reply to alexander :surkov from comment #9)

> > +  <a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=735645">Mozilla Bug 735645</a>
> 
> please put @title attribute on it

btw, I'll do this locally before landing

Comment 11

[Mark Capella [:capella]]

I wondered about it when copied the line above it ... will be sure to from now on ...

Comment 12

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2302e6bc12d7

Comment 13

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/2302e6bc12d7