Last Comment Bug 849624 - modify HTML5 header and footer accessibility API mapping
: modify HTML5 header and footer accessibility API mapping
Status: RESOLVED FIXED
[mentor=surkov.alexander@gmail.com][l...
: html5
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: 22 Branch
: x86 Windows Vista
: -- normal (vote)
: mozilla22
Assigned To: Brandon Coffman
:
Mentors:
Depends on:
Blocks: htmla11y
  Show dependency treegraph
 
Reported: 2013-03-10 06:54 PDT by steve faulkner
Modified: 2013-03-30 17:56 PDT (History)
10 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Updated tests for bug 849624 (2.39 KB, patch)
2013-03-29 01:56 PDT, Brandon Coffman
surkov.alexander: review+
Details | Diff | Splinter Review
Fix for bug 849624 (3.21 KB, patch)
2013-03-29 01:57 PDT, Brandon Coffman
surkov.alexander: review+
Details | Diff | Splinter Review
Fixed testAbsentAttrs call, rearranged element placement (2.24 KB, patch)
2013-03-29 04:17 PDT, Brandon Coffman
surkov.alexander: review+
Details | Diff | Splinter Review
Fixed nits, partially removed obsolete comment (3.37 KB, patch)
2013-03-29 04:20 PDT, Brandon Coffman
surkov.alexander: review+
Details | Diff | Splinter Review

Description steve faulkner 2013-03-10 06:54:02 PDT
User Agent: Mozilla/5.0 (Windows NT 6.0; rv:22.0) Gecko/20130307 Firefox/22.0
Build ID: 20130307030926

Steps to reproduce:

Currently the HTML5 header element is not mapped to the ARIA banner landmark role. The footer element is mapped to the contentinfo landmark role. Mapping in Firefox differs from mapping in webkit: 

Webkit maps header to banner when the header element is not a descendent of and article or section element.

Webkit maps footer to contentinfo role when the footer element is not a descendent of and article or section element.

Suggest implementing the same mapping in Firefox as it reflects the recommended usage of the landmark roles, will lead to and improvement in interoperability.

The recommended mappings are defined here: http://www.w3.org/html/wg/drafts/html/master/dom.html#sec-strong-native-semantics

related webkit bug: https://bugs.webkit.org/show_bug.cgi?id=78992#c1
Comment 1 alexander :surkov 2013-03-10 09:17:22 PDT
it's interesting that the mapping is rather DOM document oriented than application oriented. In other words any subdocument within a web page is allowed to have a banner.
Comment 2 alexander :surkov 2013-03-11 22:40:59 PDT
See HyperTextAccessible::NativeAttributes() and follow comment #0 to expose xmlroles attribute properly
Comment 3 Brandon Coffman 2013-03-21 11:22:38 PDT
Hi, I'd like to adopt this as my first bug. One question, how can I check that an element is not a descendant of an article or section element? I was thinking of using parent = Parent() and then later parent = parent->Parent() to verify each parent != nsGkAtoms::{article,section} until the parent == nullptr. Is there a better (or correct) way to do this?
Comment 4 Trevor Saunders (:tbsaunde) 2013-03-21 12:20:46 PDT
(In reply to Brandon Coffman from comment #3)
> Hi, I'd like to adopt this as my first bug. One question, how can I check
> that an element is not a descendant of an article or section element? I was
> thinking of using parent = Parent() and then later parent = parent->Parent()
> to verify each parent != nsGkAtoms::{article,section} until the parent ==
> nullptr. Is there a better (or correct) way to do this?

no, just write a loop.
Comment 5 Derek Perrin 2013-03-26 18:11:00 PDT
(In reply to Brandon Coffman from comment #3)
> Hi, I'd like to adopt this as my first bug. One question, how can I check
> that an element is not a descendant of an article or section element? I was
> thinking of using parent = Parent() and then later parent = parent->Parent()
> to verify each parent != nsGkAtoms::{article,section} until the parent ==
> nullptr. Is there a better (or correct) way to do this?

Is this still being worked on by you?
Comment 6 Brandon Coffman 2013-03-26 19:41:25 PDT
(In reply to Derek Perrin from comment #5)
> (In reply to Brandon Coffman from comment #3)
> > Hi, I'd like to adopt this as my first bug. One question, how can I check
> > that an element is not a descendant of an article or section element? I was
> > thinking of using parent = Parent() and then later parent = parent->Parent()
> > to verify each parent != nsGkAtoms::{article,section} until the parent ==
> > nullptr. Is there a better (or correct) way to do this?
> 
> Is this still being worked on by you?

Hi, Derek. Yes, I'm still working on it. I have a patch. I just haven't tested it yet.
Comment 7 Brandon Coffman 2013-03-29 01:56:30 PDT
Created attachment 731074 [details] [diff] [review]
Updated tests for bug 849624
Comment 8 Brandon Coffman 2013-03-29 01:57:27 PDT
Created attachment 731075 [details] [diff] [review]
Fix for bug 849624
Comment 9 alexander :surkov 2013-03-29 02:45:20 PDT
Comment on attachment 731074 [details] [diff] [review]
Updated tests for bug 849624

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

::: accessible/tests/mochitest/attributes/test_xml-roles.html
@@ +98,5 @@
> +  </article>
> +  <section id="section_with_header_and_footer">
> +    <header id="section_header">a header within an section</header>
> +    <footer id="section_footer">a footer within an section</footer>
> +  </section>

nit: makes sense to keep this block next to header and footer elements above
Comment 10 alexander :surkov 2013-03-29 02:46:48 PDT
Comment on attachment 731074 [details] [diff] [review]
Updated tests for bug 849624

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

::: accessible/tests/mochitest/attributes/test_xml-roles.html
@@ +22,5 @@
>        // Some AT may look for this
>        testAttrs("nav", {"xml-roles" : "navigation"}, true);
> +      testAttrs("header", {"xml-roles" : "banner"}, true);
> +      testAbsentAttrs("article_header", {"xml-roles" : "banner"}, true);
> +      testAbsentAttrs("section_header", {"xml-roles" : "banner"}, true);

btw, testAbsentAttrs don't take the last argument
Comment 11 alexander :surkov 2013-03-29 02:56:02 PDT
Comment on attachment 731075 [details] [diff] [review]
Fix for bug 849624

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

thank you!

::: accessible/src/generic/HyperTextAccessible.cpp
@@ +1284,3 @@
>      nsAccUtils::SetAccAttr(attributes, nsGkAtoms::xmlroles,
>                             NS_LITERAL_STRING("region"));
> +  } else if (tag == nsGkAtoms::header || tag == nsGkAtoms::footer) { 

nit: a whitespace in the end of line

@@ +1285,5 @@
>                             NS_LITERAL_STRING("region"));
> +  } else if (tag == nsGkAtoms::header || tag == nsGkAtoms::footer) { 
> +    // Only map header and footer if they are not descendants
> +    // of an article or section tag.
> +    nsIContent *parent = mContent->GetParent();

nit: tyep* name, here and below

@@ +1287,5 @@
> +    // Only map header and footer if they are not descendants
> +    // of an article or section tag.
> +    nsIContent *parent = mContent->GetParent();
> +    while (parent) {
> +      nsIAtom *pTag = parent->Tag();

it's better to name it 'tag' or 'parentTag' but you don't need a local variable for inline getter, just do parent->Tag()

@@ +1293,5 @@
> +        break;
> +      parent = parent->GetParent();
> +    }
> +
> +    // No article or section ancestors found.

nit: No article or section elements ...

@@ +1298,5 @@
> +    if (!parent) {
> +      if (tag == nsGkAtoms::header) 
> +        nsAccUtils::SetAccAttr(attributes, nsGkAtoms::xmlroles,
> +                               NS_LITERAL_STRING("banner"));
> +      else if (tag == nsGkAtoms::footer) 

again whitespaces at the end of line and pls add { } for if/else here too
Comment 12 Brandon Coffman 2013-03-29 04:17:20 PDT
Created attachment 731118 [details] [diff] [review]
Fixed testAbsentAttrs call, rearranged element placement
Comment 13 Brandon Coffman 2013-03-29 04:20:54 PDT
Created attachment 731119 [details] [diff] [review]
Fixed nits, partially removed obsolete comment

I've also partially removed a comment this time that I think this patch obsoletes. Let me know if it still belongs there.
Comment 14 alexander :surkov 2013-03-29 04:20:55 PDT
Comment on attachment 731118 [details] [diff] [review]
Fixed testAbsentAttrs call, rearranged element placement

you don't need to rerequest review for trivial changes
Comment 15 Brandon Coffman 2013-03-29 04:22:46 PDT
Got it. Sorry.
Comment 16 alexander :surkov 2013-03-29 04:24:57 PDT
(In reply to Brandon Coffman from comment #13)
> Created attachment 731119 [details] [diff] [review]
> Fixed nits, paritally removed obsolete comment
> 
> I've also partially removed a comment this time that I think this patch
> obsoletes. Let me know if it still belongs there.

thank you

Will you land it? Should I?

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