Closed Bug 849624 Opened 7 years ago Closed 7 years ago

modify HTML5 header and footer accessibility API mapping

Categories

(Core :: Disability Access APIs, defect)

22 Branch
x86
Windows Vista
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: faulkner.steve, Assigned: brandon.coffman)

References

(Blocks 1 open bug)

Details

(Keywords: html5, Whiteboard: [mentor=surkov.alexander@gmail.com][lang=c++])

Attachments

(2 files, 2 obsolete files)

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
Component: Untriaged → Disability Access
Keywords: html5
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Component: Disability Access → Disability Access APIs
Product: Firefox → Core
See HyperTextAccessible::NativeAttributes() and follow comment #0 to expose xmlroles attribute properly
Whiteboard: [mentor=surkov.alexander@gmail.com][lang=c++]
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?
(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.
(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?
(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.
Attached patch Updated tests for bug 849624 (obsolete) — Splinter Review
Attachment #731074 - Flags: review?(surkov.alexander)
Attached patch Fix for bug 849624 (obsolete) — Splinter Review
Attachment #731075 - Flags: review?(surkov.alexander)
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
Attachment #731074 - Flags: review?(surkov.alexander) → review+
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 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
Attachment #731075 - Flags: review?(surkov.alexander) → review+
Assignee: nobody → brandon.coffman
Attachment #731074 - Attachment is obsolete: true
Attachment #731118 - Flags: review?(surkov.alexander)
I've also partially removed a comment this time that I think this patch obsoletes. Let me know if it still belongs there.
Attachment #731075 - Attachment is obsolete: true
Attachment #731119 - Flags: review?(surkov.alexander)
Comment on attachment 731118 [details] [diff] [review]
Fixed testAbsentAttrs call, rearranged element placement

you don't need to rerequest review for trivial changes
Attachment #731118 - Flags: review?(surkov.alexander) → review+
Got it. Sorry.
Attachment #731119 - Flags: review?(surkov.alexander) → review+
(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?
Attachment #731119 - Attachment description: Fixed nits, paritally removed obsolete comment → Fixed nits, partially removed obsolete comment
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6d50859261b0
https://hg.mozilla.org/mozilla-central/rev/1bd3110dcc82
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in before you can comment on or make changes to this bug.