Last Comment Bug 734982 - Map ARIA role "form".
: Map ARIA role "form".
Status: RESOLVED FIXED
[good first bug][mentor=eitan@monoton...
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla14
Assigned To: Mark Capella [:capella]
:
Mentors:
Depends on: 737156
Blocks: aria
  Show dependency treegraph
 
Reported: 2012-03-12 12:29 PDT by David Bolter [:davidb]
Modified: 2012-03-27 05:10 PDT (History)
5 users (show)
surkov.alexander: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (v1) (4.76 KB, patch)
2012-03-22 17:55 PDT, Mark Capella [:capella]
eitan: feedback+
Details | Diff | Splinter Review
Patch (v2) (4.76 KB, patch)
2012-03-23 21:20 PDT, Mark Capella [:capella]
surkov.alexander: review+
Details | Diff | Splinter Review
Patch (v3) (5.48 KB, patch)
2012-03-26 04:03 PDT, Mark Capella [:capella]
surkov.alexander: review+
Details | Diff | Splinter Review

Description David Bolter [:davidb] 2012-03-12 12:29:09 PDT
For some reason this one is missing from our nsARIAMap.
Comment 1 David Bolter [:davidb] 2012-03-12 12:35:06 PDT
http://www.w3.org/WAI/PF/aria/roles#form
Comment 2 alexander :surkov 2012-03-19 17:47:10 PDT
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] 2012-03-22 11:39:38 PDT
Takes it ... could have rolled it in with # 737156 ... separate works also
Comment 4 Mark Capella [:capella] 2012-03-22 17:55:12 PDT
Created attachment 608550 [details] [diff] [review]
Patch (v1)

First try ... up for review for correctness / completeness ...
Comment 5 Eitan Isaacson [:eeejay] 2012-03-23 13:53:33 PDT
Comment on attachment 608550 [details] [diff] [review]
Patch (v1)

This looks correct, thank you!
Comment 6 Mark Capella [:capella] 2012-03-23 21:20:52 PDT
Created attachment 608962 [details] [diff] [review]
Patch (v2)

Added :surkov as review to commit message, re-posted
Comment 7 alexander :surkov 2012-03-26 01:52:03 PDT
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] 2012-03-26 04:03:14 PDT
Created attachment 609279 [details] [diff] [review]
Patch (v3)

Nits addressed - re-requesting review
Comment 9 alexander :surkov 2012-03-26 04:10:32 PDT
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 2012-03-26 04:11:09 PDT
(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] 2012-03-26 04:19:35 PDT
I wondered about it when copied the line above it ... will be sure to from now on ...
Comment 13 Marco Bonardo [::mak] (Away 6-20 Aug) 2012-03-27 05:07:50 PDT
https://hg.mozilla.org/mozilla-central/rev/2302e6bc12d7

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