849624 - modify HTML5 header and footer accessibility API mapping

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[steve faulkner]

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

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

See HyperTextAccessible::NativeAttributes() and follow comment #0 to expose xmlroles attribute properly

Comment 3

[Brandon Coffman]

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

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

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

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

Attachment: Updated tests for bug 849624

Comment 8

[Brandon Coffman]

Attachment: Fix for bug 849624

Comment 9

[alexander :surkov (:asurkov)]

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

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

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]

Attachment: Fixed testAbsentAttrs call, rearranged element placement

Comment 13

[Brandon Coffman]

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

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]

Got it. Sorry.

Comment 16

[alexander :surkov (:asurkov)]

(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?

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6d50859261b0
https://hg.mozilla.org/integration/mozilla-inbound/rev/1bd3110dcc82

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6d50859261b0
https://hg.mozilla.org/mozilla-central/rev/1bd3110dcc82