modify HTML5 header and footer accessibility API mapping

RESOLVED FIXED in mozilla22

Status

()

Core
Disability Access APIs
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: steve faulkner, Assigned: Brandon Coffman)

Tracking

(Blocks: 1 bug, {html5})

22 Branch
mozilla22
x86
Windows Vista
html5
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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
(Reporter)

Updated

5 years ago
Component: Untriaged → Disability Access
Keywords: html5

Updated

5 years ago
Blocks: 368880

Comment 1

5 years ago
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

Updated

5 years ago
Component: Disability Access → Disability Access APIs
Product: Firefox → Core

Comment 2

5 years ago
See HyperTextAccessible::NativeAttributes() and follow comment #0 to expose xmlroles attribute properly
Whiteboard: [mentor=surkov.alexander@gmail.com][lang=c++]
(Assignee)

Comment 3

5 years ago
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.

Comment 5

5 years ago
(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?
(Assignee)

Comment 6

5 years ago
(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.
(Assignee)

Comment 7

4 years ago
Created attachment 731074 [details] [diff] [review]
Updated tests for bug 849624
Attachment #731074 - Flags: review?(surkov.alexander)
(Assignee)

Comment 8

4 years ago
Created attachment 731075 [details] [diff] [review]
Fix for bug 849624
Attachment #731075 - Flags: review?(surkov.alexander)

Comment 9

4 years ago
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 10

4 years ago
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

4 years ago
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+

Updated

4 years ago
Assignee: nobody → brandon.coffman
(Assignee)

Comment 12

4 years ago
Created attachment 731118 [details] [diff] [review]
Fixed testAbsentAttrs call, rearranged element placement
Attachment #731074 - Attachment is obsolete: true
Attachment #731118 - Flags: review?(surkov.alexander)
(Assignee)

Comment 13

4 years ago
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.
Attachment #731075 - Attachment is obsolete: true
Attachment #731119 - Flags: review?(surkov.alexander)

Comment 14

4 years ago
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+
(Assignee)

Comment 15

4 years ago
Got it. Sorry.

Updated

4 years ago
Attachment #731119 - Flags: review?(surkov.alexander) → review+

Comment 16

4 years ago
(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?
(Assignee)

Updated

4 years ago
Attachment #731119 - Attachment description: Fixed nits, paritally removed obsolete comment → Fixed nits, partially removed obsolete comment
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d50859261b0
https://hg.mozilla.org/integration/mozilla-inbound/rev/1bd3110dcc82
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6d50859261b0
https://hg.mozilla.org/mozilla-central/rev/1bd3110dcc82
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in before you can comment on or make changes to this bug.