593368 - Provide mappings for html5 <nav> <header> <footer> <article>

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[David Bolter [:davidb] (NeedInfo me for attention)]

Was pinged on IRC about this.

Comment 1

[David Bolter [:davidb] (NeedInfo me for attention)]

Current spec map:
nav : navigation
header : banner
footer : contentinfo
article : (article, document, application, or main)

Comment 2

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: WIP

Comment 3

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 484055 [details] [diff] [review]
WIP

New patch coming...

Comment 4

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: WIP2

This one will expose header and footer with better IA2 roles. Looking for feedback.

Comment 5

[alexander :surkov (:asurkov)]

Comment on attachment 484064 [details] [diff] [review]
WIP2


>+  if (nsAccUtils::IsLandmark(tag)) {
>+    nsAccessible* accessible = new nsHyperTextAccessibleWrap(aContent, aWeakShell);
>+    NS_IF_ADDREF(accessible);
>+    return accessible;
>+  }

What kind of frame are used for these by default that we don't create accessbiles for them?

Another issue: I think AT should use different algorithm to find landmarks than checking their roles, perhaps they look for role object attribute. You need to check this with AT vendors.

Comment 6

[Marco Zehe (:MarcoZ)]

Comment on attachment 484064 [details] [diff] [review]
WIP2

Sorry, have to give an f- for this one:

1. ROLE_HEADER and ROLE_FOOTER are, I believe, meant for document header and footer sections like in MS Word the header and footer panes.
2. We should adhere strictly to the ARIA mapping: All of these landmark elements should be exposed as ROLE_SECTION, and the faked ARIA landmark role provided in the object attribute as Alexander said. I suggest for the article type to currently stick to mapping it to main. I believe this is the most common use case. If web authors want something different, they should provide an additioal ARIA role like "application" or "document" if they need it.

Comment 7

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #5)

> >+    nsAccessible* accessible = new nsHyperTextAccessibleWrap(aContent, aWeakShell);
> 
> What kind of frame are used for these by default that we don't create
> accessbiles for them?

We do already create accessibles for them, but they are uninteresting paragraphs.

> Another issue: I think AT should use different algorithm to find landmarks than
> checking their roles, perhaps they look for role object attribute. You need to
> check this with AT vendors.

I think they currently look for xml-roles in the object attributes, but I worry about exposing html5 elements in this way. It smells a bit funny to me. I did do it in an earlier WIP but convinced myself it was dangerous... need to get my head back into why. We can discuss on channel.

(In reply to comment #6)
> Comment on attachment 484064 [details] [diff] [review]
> WIP2
> 
> 1. ROLE_HEADER and ROLE_FOOTER are, I believe, meant for document header and
> footer sections like in MS Word the header and footer panes.

Right but this is exactly how the header and footer elements are expected to be used no?

Comment 8

[alexander :surkov (:asurkov)]

(In reply to comment #7)
> (In reply to comment #5)
> 
> > >+    nsAccessible* accessible = new nsHyperTextAccessibleWrap(aContent, aWeakShell);
> > 
> > What kind of frame are used for these by default that we don't create
> > accessbiles for them?
> 
> We do already create accessibles for them, but they are uninteresting
> paragraphs.

Does it mean IsLandmark is useless?

> > Another issue: I think AT should use different algorithm to find landmarks than
> > checking their roles, perhaps they look for role object attribute. You need to
> > check this with AT vendors.
> 
> I think they currently look for xml-roles in the object attributes, but I worry
> about exposing html5 elements in this way. It smells a bit funny to me. I did
> do it in an earlier WIP but convinced myself it was dangerous... need to get my
> head back into why. We can discuss on channel.

Yes, technically it might be not good if xml-roles are ARIA roles only, and we might get into ambiguity if ARIA role is used on these elements. In this case we should decide who wins. Perhaps we should leave xml-roles unchanged especially if some AT wants to deal with ARIA markup in their code. But then we should provide landmark attribute or similar stuffs that would have universal meaning not depending on used markup.

> (In reply to comment #6)
> > Comment on attachment 484064 [details] [diff] [review] [details]
> > WIP2
> > 
> > 1. ROLE_HEADER and ROLE_FOOTER are, I believe, meant for document header and
> > footer sections like in MS Word the header and footer panes.
> 
> Right but this is exactly how the header and footer elements are expected to be
> used no?

I thought about this. Technically header and footer in MS word are something that describes the page, not a main sections as they're used in the web. So perhaps we shouldn't use these roles as Marco said.

Technically we shouldn't get resolution inside of Firefox a11y, these should be open discussions, at least when the subject is obviously debatable.

Comment 9

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #8)

> Technically we shouldn't get resolution inside of Firefox a11y, these should be
> open discussions, at least when the subject is obviously debatable.

This is always true for everything we do in Accessibility :)

(Note: Linux, IA2, and IDI members were just now discussing on #accessibility)

Comment 10

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #5)
> > 
> > > >+    nsAccessible* accessible = new nsHyperTextAccessibleWrap(aContent, aWeakShell);
> > > 
> > > What kind of frame are used for these by default that we don't create
> > > accessbiles for them?
> > 
> > We do already create accessibles for them, but they are uninteresting
> > paragraphs.
> 
> Does it mean IsLandmark is useless?

Yes for this version of the "WIP" ;)

Comment 11

[alexander :surkov (:asurkov)]

(In reply to comment #9)

> (Note: Linux, IA2, and IDI members were just now discussing on #accessibility)

Ok, great, please make sure to put summary here :)

Comment 12

[Pete Brunet]

I think using the existing IA2/ATK roles of header/footer is OK.  You'll have to ask the ATVs how they would use these role.  I suspect they speak the role name and the contents.  (The roles might also be useful for filtering during the generation of various quick navigation lists.  I don't think just because those roles are currently used in office documents that this implies that the semantics of header/footer have to remain the same in web documents.  The user knows what kind of document they are in and will supply their own understanding of what a header/footer is.

It could be OK to map these to however you handle the ARIA equivalents and not map to the IA2/ATK roles of header/footer, assuming there is a good semantic match between header/footer and the existing aria equivalents.  Joanie feels that there is enough difference, e.g. header/footer are not sections, but information that describes sections, and thus you should use the IA2/ATK roles of header/footer for these html5 tags.

Getting input from the ATVs will be very important.

Comment 13

[David Bolter [:davidb] (NeedInfo me for attention)]

Thanks Pete.

Alexander, in terms of a summary of the IRC discussion it is tricky. Here's my perception:

1. We are reluctant to break the xml-roles are for ARIA and tag is for html pattern we've already got.
2. We discussed the idea of an additional object attribute like "landmark".
3. We discussed the fact that creatively using xml-roles for html landmarks would likely be the quickest route to accessibility, but it feels unclean.

I'll fire of an email to capture vendor opinions, and hopefully Jamie can report his thoughts directly here.

Comment 14

[James Teh [:Jamie]]

(In reply to comment #7)
> I think they currently look for xml-roles in the object attributes
We do.
> but I worry
> about exposing html5 elements in this way. It smells a bit funny to me.
This is *precisely* what I argued in bug 459395 a year and a half ago, but I was defeated for reasons I still don't understand/agree with. :)

(In reply to comment #13)
> 1. We are reluctant to break the xml-roles are for ARIA and tag is for html
> pattern we've already got.
That's going to make searching for landmarks (both ARIA and HTML5) incredibly difficult for ATs.

> 2. We discussed the idea of an additional object attribute like "landmark".
Again, that's what I argued in bug 459395. Unfortunately, in order to maintain backwards compatibility, this will mean ATs need to try the landmark attribute and then fall back to xml-roles if that fails. Alternatively, they could check the Gecko version (ug).

With regard to ATK/IA2 role, we need a quick way to fetch/move between landmarks, regardless of what they are. Thus, role isn't a good fit, as other landmarks can't be exposed that way. We do not use the role for landmarks at all. Personally, I don't mind whether the role is changed, but it needs to be exposed another way as well; e.g. landmark or xml-roles attribute.

Comment 15

[James Teh [:Jamie]]

Answering a couple of questions from David's email that I haven't already answered:
> 1. Do you use the 'tag' object attribute, and when?
Never for landmarks. For reference, we only use it to detect blockquote (since that doesn't fit naturally into the idea of roles) and embedded objects (because some versions of Gecko don't expose the embedded object role correctly in some cases).

> 2. Do you think tag is correct and sufficient for exposing html landmark elements?
No. This makes searching for both ARIA and HTML 5 landmarks extremely difficult.

Comment 16

[alexander :surkov (:asurkov)]

Thanks, Jamie, for bringing up bug 459395.

1) I think we shouldn't use xml-roles, this concept is wider than landmark concept and might be used by AT who has own ARIA processing so we run into ambiguity when ARIA is used on these HTML elements.

2) I would like to know why PF group has rejected landmark idea but we can always make it Firefox specific while AT needs it.

Comment 17

[David Bolter [:davidb] (NeedInfo me for attention)]

I'm trying to figure out what Jamie wants (smile).(In reply to comment #14)

> (In reply to comment #7)
> (In reply to comment #13)
> > 1. We are reluctant to break the xml-roles are for ARIA and tag is for html
> > pattern we've already got.
> That's going to make searching for landmarks (both ARIA and HTML5) incredibly
> difficult for ATs.

Why?

Comment 18

[alexander :surkov (:asurkov)]

(In reply to comment #17)
> I'm trying to figure out what Jamie wants (smile).(In reply to comment #14)
> 
> > (In reply to comment #7)
> > (In reply to comment #13)
> > > 1. We are reluctant to break the xml-roles are for ARIA and tag is for html
> > > pattern we've already got.
> > That's going to make searching for landmarks (both ARIA and HTML5) incredibly
> > difficult for ATs.
> 
> Why?

Perhaps because they might be not aware about all cases. If we provide list of allowed landmarks then it's much simpler to implement this. Now they will forced to do like:

role = getAttribtue(xml-roles)
if (role == landmark1) {
  do something
} else if (....

tag = getAttribute(tag);
if (tag == landmark1) {
  do something but what about namespace? Should we start to use ISimpleDOMNode interface?
} else if (...
// here we will append new cases when we get aware of.

but we can suggest
landmark = getAttribute("landmark");
if (landmark == landmark1)
  do something
else if (...

Comment 19

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #18)
> (In reply to comment #17)
> > I'm trying to figure out what Jamie wants (smile).(In reply to comment #14)
> > 
> > > (In reply to comment #7)
> > > (In reply to comment #13)
> > > > 1. We are reluctant to break the xml-roles are for ARIA and tag is for html
> > > > pattern we've already got.
> > > That's going to make searching for landmarks (both ARIA and HTML5) incredibly
> > > difficult for ATs.
> > 
> > Why?
> 
> Perhaps because they might be not aware about all cases. If we provide list of
> allowed landmarks then it's much simpler to implement this. Now they will
> forced to do like:
> 
> role = getAttribtue(xml-roles)
> if (role == landmark1) {
>   do something
> } else if (....
> 

If there is an xml-role why continue?

> tag = getAttribute(tag);
> if (tag == landmark1) {
>   do something but what about namespace? Should we start to use ISimpleDOMNode
> interface?

I wouldn't worry about namespace.

> but we can suggest
> landmark = getAttribute("landmark");
> if (landmark == landmark1)
>   do something

Would that call only happen if there is no known xml-role?

Comment 20

[alexander :surkov (:asurkov)]

(In reply to comment #19)

> If there is an xml-role why continue?

you don't need

> 
> > tag = getAttribute(tag);
> > if (tag == landmark1) {
> >   do something but what about namespace? Should we start to use ISimpleDOMNode
> > interface?
> 
> I wouldn't worry about namespace.

Ideally they should because we have HTML, XUL and few other namespaces and Firefox can be extended by XTF like in the case of XForms.

> > but we can suggest
> > landmark = getAttribute("landmark");
> > if (landmark == landmark1)
> >   do something
> 
> Would that call only happen if there is no known xml-role?

No, that's unique check they will do.

Comment 21

[David Bolter [:davidb] (NeedInfo me for attention)]

I'm just not understanding the problem with checking tag if there is no xml-role.

Comment 22

[James Teh [:Jamie]]

(In reply to comment #21)
> I'm just not understanding the problem with checking tag if there is no
> xml-role.
Three reasons:
1. principle: It's ugly to have to check two places to determine whether something is a landmark; it's not abstract at all. It's similar to the argument for why we use a11y APIs instead of traversing the DOM directly.
2. practical: One of the tasks we perform most often in relation to landmarks is searching. We need to (quickly!) search the document for landmarks, ARIA, HTML5 or otherwise. Having a single source for that information (e.g. a landmark attribute) makes the search simpler and thus faster. We could get around this by grabbing xml-roles and tag and creating a landmark attribute internally, but that is again fairly ugly.
3. future-proof: The browser will always have a better idea of the document standards than ATs do. If the browser starts handling a different document type which specifies landmarks differently and the landmark info is exposed in a standard, abstract way, it will "just work", the same way that accessible role "just works" regardless of how it is defined (ARIA, tag, etc.).

So, what I want:
Personally, I think the unified landmark attribute is the best approach. Unfortunately, this means that newer ATs have to do version checks or check for both landmark and xml-roles in order to work with older versions of Gecko. However, now is probably the right time to do it if we're going to.

Problems:
I think one of the reasons the landmark attribute was rejected was that there is no unified decision on which ARIA roles should be landmark roles. For example, some people believe that "application" should be considered a kind of landmark (I personally disagree). In NVDA, we use a relatively conservative set: banner, complementary, contentinfo, main, navigation, search.

Comment 23

[David Bolter [:davidb] (NeedInfo me for attention)]

I think we can add a landmark attribute. We can't wait for new desktop 

I'll note that a general concern I have is the object attributes are in danger of turning into a de-facto IAccessible3.

How would these be exposed in UIA?

Comment 24

[James Teh [:Jamie]]

(In reply to comment #23)
> We can't wait for new desktop
Huh? :)

> I'll note that a general concern I have is the object attributes are in danger
> of turning into a de-facto IAccessible3.
Agreed, and it's expensive to calculate them all at once. This also applies to ATK.

> How would these be exposed in UIA?
I'm not sure about landmarks specifically. UIA seems to have AriaRole and AriaProperties, which is ugly and totally non-abstract. That said, iirc, UIA is easier to extend in that it supports the idea of "properties", which are grouped into "control patterns". Both properties and control patterns have IDs. I'm not sure whether the spec allows you to define a property outside a control pattern, although I think it's *technically* possible. I think you can do all of this without modifying the binary server side interface, although the client API does define convenience interfaces for the control patterns in the spec. Mick knows more about UIA than I do. There are three ways to solve this problem for IA2:
1. Add new interfaces for this new functionality. This is the cleanest according to the current spec and won't break the existing interfaces.
2. Push forward on a better API for fetching attributes; e.g. fetch individual attributes or perhaps only requested attributes. This has been discussed on the IA2 list, but it seems to have stalled.
3. Move towards an API where *everything* is an attribute and the core API just defines the functionality to fetch and set attributes by name or ID. We still have a spec for what attributes are used, but this way, the binary interface never has to be changed. Afaik, this is how the Apple accessibility API works and is closer to how UIA works on the server side.
This is getting hideously off topic for this bug. Nevertheless, it is a discussion that probably needs to be had sooner or later.

Comment 25

[James Teh [:Jamie]]

(In reply to comment #13)
> 3. We discussed the fact that creatively using xml-roles for html landmarks
> would likely be the quickest route to accessibility, but it feels unclean.
It's worth noting that while I totally agree that this is unclean/hacky, it is indeed the quickest route to accessibility, involves the least amount of work on the AT side and means that HTML 5 landmarks would "just work" for ATs which already use xml-roles. As such, if others prefer this solution, we won't argue. :)

Comment 26

[alexander :surkov (:asurkov)]

(In reply to comment #24)
> (In reply to comment #23)
> > We can't wait for new desktop
> Huh? :)
> 
> > I'll note that a general concern I have is the object attributes are in danger
> > of turning into a de-facto IAccessible3.
> Agreed, and it's expensive to calculate them all at once. This also applies to
> ATK.

IA2 group seems to agree to change semantics of attribute getting making it performant. What is IAccessible3 exactly and what's danger?

Comment 27

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #26)
> (In reply to comment #24)
> > (In reply to comment #23)
> > > We can't wait for new desktop
> > Huh? :)
> > 
> > > I'll note that a general concern I have is the object attributes are in danger
> > > of turning into a de-facto IAccessible3.
> > Agreed, and it's expensive to calculate them all at once. This also applies to
> > ATK.
> 
> IA2 group seems to agree to change semantics of attribute getting making it
> performant.

OK but we don't know how far away that is.

> What is IAccessible3 exactly and what's danger?

I mean instead of pushing change on desktop API, we are putting more information into object attributes. If we then start 'mapping' things like elements to xml-roles, it feels even more like we are treating object attributes as our 'API'. I'm not sure this is good.

Comment 28

[alexander :surkov (:asurkov)]

(In reply to comment #27)

> > IA2 group seems to agree to change semantics of attribute getting making it
> > performant.
> 
> OK but we don't know how far away that is.

I'll get back to this after Fx4. I think we can get this since AT is interesting to it.

> 
> > What is IAccessible3 exactly and what's danger?
> 
> I mean instead of pushing change on desktop API, we are putting more
> information into object attributes. If we then start 'mapping' things like
> elements to xml-roles, it feels even more like we are treating object
> attributes as our 'API'. I'm not sure this is good.

It's not good and not bad, it's evaluation process. We can't change API for one feature, for feature approbation. We use object attribute. But then when we get set of demanded features then we should propose interface changes for this.

Comment 29

[alexander :surkov (:asurkov)]

(In reply to comment #28)

> It's not good and not bad, it's evaluation process. We can't change API for one
> feature, for feature approbation. We use object attribute. But then when we get
> set of demanded features then we should propose interface changes for this.

For example, that was happen with table-cell-index attribute what was resulted into IAccessibleTableCell interface.

Sure we get some burden of backward compatibility but I'm not sure whether we have a better way.

Comment 30

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #28)
> (In reply to comment #27)
> 
> > > IA2 group seems to agree to change semantics of attribute getting making it
> > > performant.
> > 
> > OK but we don't know how far away that is.
> 
> I'll get back to this after Fx4. I think we can get this since AT is
> interesting to it.
> 

Let's be sure to look at how Apple does this.

Comment 31

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #24)
> (In reply to comment #23)

> 3. Move towards an API where *everything* is an attribute and the core API just
> defines the functionality to fetch and set attributes by name or ID. We still
> have a spec for what attributes are used, but this way, the binary interface
> never has to be changed. Afaik, this is how the Apple accessibility API works
> and is closer to how UIA works on the server side.
> This is getting hideously off topic for this bug. Nevertheless, it is a
> discussion that probably needs to be had sooner or later.

Jamie would you be interesting in helping make this happen... say over a 4-5 year parallel activity?

Comment 32

[David Bolter [:davidb] (NeedInfo me for attention)]

Actually it could be baby steps.

Comment 33

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: patch 1

Anyways, back to the bug.

This patch goes with Marco's original suggestion of using the xml-roles attribute, but in terms of IA2 role, we expose headers as headers and footers as footers, while nav and article are sections.

For the xml-roles I use the mapping suggested in the html5 aria draft document, and am going with main for article since NVDA looks for main and it seems reasonable.

I now feel this is a good compromise overall and hope we can agree.

A functionally equivalent try build should eventually appear at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-adc1975548ea

Comment 34

[James Teh [:Jamie]]

(In reply to comment #31)
> > 3. Move towards an API where *everything* is an attribute and the core API just
> > defines the functionality to fetch and set attributes by name or ID.
> Jamie would you be interesting in helping make this happen... say over a 4-5
> year parallel activity?
Yes, but with one major condition: The only reason I would do this is to avoid the difficulties of binary compatibility. We still need a central group (with decent representation from both client and server developers) that maintains a spec that all implementers should follow. I don't want to see a situation where app and AT vendors start inventing attributes in private which aren't well defined and end up causing severe fragmentation. In other words, it should be the same as if we had a binary interface without the headaches of binary compatibility issues when a new spec is released.

I have a whole load of other concerns and requirements, but as I said, this really should be discussed elsewhere.

Comment 35

[alexander :surkov (:asurkov)]

Do we have an example of AT that use xml-roles to find navigation, banner, contentinfo or main landmarks?

Is any AT interested to have landmark object attribute what is more specific than xml-roles?

Comment 36

[James Teh [:Jamie]]

(In reply to comment #35)
> Do we have an example of AT that use xml-roles to find navigation, banner,
> contentinfo or main landmarks?
NVDA does and I suspect Orca does as well.

> Is any AT interested to have landmark object attribute what is more specific
> than xml-roles?
In principle, it's cleaner. However, it's easier to do it using xml-roles, as there's already a patch and no change is required on the AT side.

Comment 37

[alexander :surkov (:asurkov)]

(In reply to comment #36)

> > Is any AT interested to have landmark object attribute what is more specific
> > than xml-roles?
> In principle, it's cleaner. However, it's easier to do it using xml-roles, as
> there's already a patch and no change is required on the AT side.

Could it be a perf issue because you don't need to compare strings looking for landmarks while there's no landmark?

Comment 38

[alexander :surkov (:asurkov)]

I'm thinking to support xml-roles as short term and landmark as long term. Does it sounds good?

Comment 39

[James Teh [:Jamie]]

(In reply to comment #37)
> Could it be a perf issue because you don't need to compare strings looking for
> landmarks while there's no landmark?
If there's a perf impact, I'd say it is very negligible. In most cases, the xml-roles attribute isn't present anyway. The only case where there'd be a perf impact is if a lot of nodes in the document had an ARIA role that wasn't a landmark, in which case we'd be doing a lot of string splitting and searching for no reason. However, as I say, I doubt this would ever be noticed.

(In reply to comment #38)
> I'm thinking to support xml-roles as short term and landmark as long term. Does
> it sounds good?
While it sounds good in principle, the reality is that once the xml-roles hack is implemented, most ATs (even us!) probably won't bother to implement support for the landmark attribute, as it would offer no practical gain at all. If this is going to be changed, I think it should be changed now while there's motivation.

Comment 40

[alexander :surkov (:asurkov)]

Ok, perhaps we need to have new API to get all interesting landmarks from the page as long term so that AT don't need to navigate through whole page looking for landmarks.

Comment 41

[alexander :surkov (:asurkov)]

Comment on attachment 485469 [details] [diff] [review]
patch 1


>+void
>+nsAccUtils::GetARIARoleForElement(nsIContent* aContent, nsAString& aRole)
>+{
>+  nsIAtom* tag = aContent->Tag();

nsIContent::Tag() is inline method, it's not recommended to create local variables while they're not necessary.

>+  if (tag == nsAccessibilityAtoms::nav)
>+    aRole = NS_LITERAL_STRING("navigation");
>+  else if (tag == nsAccessibilityAtoms::header) 
>+    aRole = NS_LITERAL_STRING("banner");
>+  else if (tag == nsAccessibilityAtoms::footer) 
>+    aRole = NS_LITERAL_STRING("contentinfo");
>+  else if (tag == nsAccessibilityAtoms::article) 
>+    aRole = NS_LITERAL_STRING("main");

>+
>   nsAutoString xmlRoles;
>-  if (mContent->GetAttr(kNameSpaceID_None, nsAccessibilityAtoms::role, xmlRoles)) {
>-    attributes->SetStringProperty(NS_LITERAL_CSTRING("xml-roles"),  xmlRoles, oldValueUnused);          
>+  if (!mContent->GetAttr(kNameSpaceID_None, nsAccessibilityAtoms::role, xmlRoles)) {
>+    // For the html landmark elements we expose them like we do aria landmarks
>+    // to make AT navigation schemes "just work".
>+    nsAccUtils::GetARIARoleForElement(mContent, xmlRoles);
>+  }

that doesn't make sense to do for any accessible while it's applicable for hypertext accessibles only.

in general I don't like an idea to check every hypertext accessible for this, can we subclass it to exclude this checks?

>+  if (!xmlRoles.IsEmpty()) {
>+    attributes->SetStringProperty(NS_LITERAL_CSTRING("xml-roles"),
>+                                  xmlRoles, oldValueUnused);

in general we use nsAccUtils::SetAccAttr, I'm not sure this is best way but it's common.

>diff --git a/accessible/tests/mochitest/Makefile.in b/accessible/tests/mochitest/Makefile.in
>--- a/accessible/tests/mochitest/Makefile.in
>+++ b/accessible/tests/mochitest/Makefile.in
>@@ -90,16 +90,17 @@ _TEST_FILES =\
> 		test_aria_roles.xul \
> 		test_aria_token_attrs.html \
> 		test_bug420863.html \
> 	$(warning   test_childAtPoint.html temporarily disabled) \
> 	$(warning	test_childAtPoint.xul temporarily disabled) \
> 		test_descr.html \
> 		test_editabletext_1.html \
> 		test_editabletext_2.html \
>+		test_elm_landmarks.html \

add attribute stuffs test to attributes folder, add role tests to test_role_nsHyperTextAcc. While you're here please move all role tests into role folder.

Comment 42

[James Teh [:Jamie]]

(In reply to comment #40)
> Ok, perhaps we need to have new API to get all interesting landmarks from the
> page as long term so that AT don't need to navigate through whole page looking
> for landmarks.
Current Windows ATs all render virtual buffers, so they can gather this info from the nodes as they render. Moving away from virtual buffers, we will need something like this, but it's part of a more general problem; i.e. we'll need an interface which allows for searching, retrieval of multiple objects, etc.

Comment 43

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #41)
> Comment on attachment 485469 [details] [diff] [review]
> patch 1
> 
> 
> >+void
> >+nsAccUtils::GetARIARoleForElement(nsIContent* aContent, nsAString& aRole)
> >+{
> >+  nsIAtom* tag = aContent->Tag();
> 
> nsIContent::Tag() is inline method, it's not recommended to create local
> variables while they're not necessary.

OK.

> >+    // to make AT navigation schemes "just work".
> >+    nsAccUtils::GetARIARoleForElement(mContent, xmlRoles);
> >+  }
> 
> that doesn't make sense to do for any accessible while it's applicable for
> hypertext accessibles only.

Agreed.

> in general I don't like an idea to check every hypertext accessible for this,
> can we subclass it to exclude this checks?

I thought about this. Do you think one subclass for html landmarks or one for each?

> >+  if (!xmlRoles.IsEmpty()) {
> >+    attributes->SetStringProperty(NS_LITERAL_CSTRING("xml-roles"),
> >+                                  xmlRoles, oldValueUnused);
> 
> in general we use nsAccUtils::SetAccAttr, I'm not sure this is best way but
> it's common.

OK

> >+		test_elm_landmarks.html \
> 
> add attribute stuffs test to attributes folder, add role tests to
> test_role_nsHyperTextAcc.

The reason I didn't do this, is because I wanted this file to be a form of 'documentation'.

> While you're here please move all role tests into
> role folder.

OK I might do this as a separate changeset/bug depending on how much cleanup I might do along the way.

Comment 44

[alexander :surkov (:asurkov)]

(In reply to comment #43)

> > in general I don't like an idea to check every hypertext accessible for this,
> > can we subclass it to exclude this checks?
> 
> I thought about this. Do you think one subclass for html landmarks or one for
> each?

for all, this could be similar to nsEnumRoleAccessible

> > >+		test_elm_landmarks.html \
> > 
> > add attribute stuffs test to attributes folder, add role tests to
> > test_role_nsHyperTextAcc.
> 
> The reason I didn't do this, is because I wanted this file to be a form of
> 'documentation'.

On the another hand a11y mochitests has structure what allows us to look what tests we have what prevents us to add dupe tests. You could refer between tests to add some doc stuffs. I think I would prefer this.

> > While you're here please move all role tests into
> > role folder.
> 
> OK I might do this as a separate changeset/bug depending on how much cleanup I
> might do along the way.

Please, if would be nice while you're here. Separate changeset is fine.

Comment 45

[Marco Zehe (:MarcoZ)]

Comment on attachment 485469 [details] [diff] [review]
patch 1

r=me.

1. The try-server build works. NVDA recognizes all the landmarks as expected without any change required on NVDA's side.
2. Even overriding the html:article tag with a role of document works. And here's the one nit:

>+  <nav id="nav">a nav</nav>
>+  <header id="header">a header</header>
>+  <footer id="footer">a footer</footer>
>+  <article id="article">an article</article>

Please add a test like

<nav id="document" role="document">A document</article>

Where the expected role is "document". This is because html:article is ambiguous in that it allows both document and application to be contained, too, but since screen readers need this more explicit, ARIA has to be used to actually achieve this. I'd like us to have a test for at least document, you can add one for application too if you feel like it.

Comment 46

[Marco Zehe (:MarcoZ)]

Additional info: JAWS Version 11 does not recognize the landmarks. But that was to be expected, kind of, with their different method of consuming Firefox content. They appear to be looking at the tag no matter what, not just the XML Role.

Comment 47

[alexander :surkov (:asurkov)]

(In reply to comment #45)

> 2. Even overriding the html:article tag with a role of document works.

Is it expected? Shouldn't ARIA override this?

Comment 48

[Marco Zehe (:MarcoZ)]

(In reply to comment #47)
> (In reply to comment #45)
> 
> > 2. Even overriding the html:article tag with a role of document works.
> 
> Is it expected? Shouldn't ARIA override this?

It should. I was just confirming that it actually does, and requested that David add tests for this.

Comment 49

[David Bolter [:davidb] (NeedInfo me for attention)]

I've added Marco's test case and it passes.

As per IRC discussion, I'll put the attributes logic into nsHyperTextAccessible::GetAttributes. I'll leave test_elm_landmarks intact for now. We can do follow up for test reorg. I think I promised to move the test_elm* into a folder :)

Comment 50

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: patch 2 (moves attribute logic into hypertext subclass)

Quick! Review before you change your mind ;)

Comment 51

[alexander :surkov (:asurkov)]

Comment on attachment 487924 [details] [diff] [review]
patch 2 (moves attribute logic into hypertext subclass)

surkov: davidb, what's about nsAccUtils::SetAccAttr usage?
[12:20am] surkov: davidb, do we really need separate method for few lines of code that used in unique place?
[12:21am] surkov: davidb, which type of frame article and nav have?

if it's block frame then should you move article and nav processing on top where div and blockquote is processed?

r=me with these addressed/answered

Comment 52

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #51)
> Comment on attachment 487924 [details] [diff] [review]
> patch 2 (moves attribute logic into hypertext subclass)
> 
> surkov: davidb, what's about nsAccUtils::SetAccAttr usage?

SetAccAttr takes an nsIAtom. Our code for setting "xml-roles" in nsAccessible also uses SetStringProperty for this reason.

> [12:20am] surkov: davidb, do we really need separate method for few lines of
> code that used in unique place?
> [12:21am] surkov: davidb, which type of frame article and nav have?
> 
> if it's block frame then should you move article and nav processing on top
> where div and blockquote is processed?

davidb: i don't get you regarding the frame type
surkov: ignore it


OK?

Comment 53

[alexander :surkov (:asurkov)]

(In reply to comment #52)
> (In reply to comment #51)
> > Comment on attachment 487924 [details] [diff] [review] [details]
> > patch 2 (moves attribute logic into hypertext subclass)
> > 
> > surkov: davidb, what's about nsAccUtils::SetAccAttr usage?
> 
> SetAccAttr takes an nsIAtom. Our code for setting "xml-roles" in nsAccessible
> also uses SetStringProperty for this reason.

that's an idea of SetAccAttr, to use atoms instead of strings, you should follow it.

> > [12:20am] surkov: davidb, do we really need separate method for few lines of
> > code that used in unique place?

not addressed

> should you move article and nav processing on top
> > where div and blockquote is processed?

not addressed

Comment 54

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #53)
> (In reply to comment #52)
> > (In reply to comment #51)
> > > Comment on attachment 487924 [details] [diff] [review] [details] [details]
> > > patch 2 (moves attribute logic into hypertext subclass)
> > > 
> > > surkov: davidb, what's about nsAccUtils::SetAccAttr usage?
> > 
> > SetAccAttr takes an nsIAtom. Our code for setting "xml-roles" in nsAccessible
> > also uses SetStringProperty for this reason.
> 
> that's an idea of SetAccAttr, to use atoms instead of strings, you should
> follow it.

xml-roles isn't an atom in the DOM sense. We don't treat it as an atom elsewhere in our code.

> 
> > > [12:20am] surkov: davidb, do we really need separate method for few lines of
> > > code that used in unique place?
> 
> not addressed

We don't need it but I prefer it.

> 
> > should you move article and nav processing on top
> > > where div and blockquote is processed?
> 
> not addressed

I'm not sure I understand. Did you see the patch?

Comment 55

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #54)

> > that's an idea of SetAccAttr, to use atoms instead of strings, you should
> > follow it.
> 
> xml-roles isn't an atom in the DOM sense. We don't treat it as an atom
> elsewhere in our code.
> 

I see we do have atoms for other things though so I can make this change I guess.

Comment 56

[alexander :surkov (:asurkov)]

(In reply to comment #54)

> xml-roles isn't an atom in the DOM sense.

what is the DOM sense of atoms?

> We don't treat it as an atom
> elsewhere in our code.

we could and perhaps should, right?

> We don't need it but I prefer it.

why? no really why? for stuffs that we're not going to spread through the code

> Did you see the patch?

yes, I did, strange question.

> > > should you move article and nav processing on top
> > > > where div and blockquote is processed?
> 
> I'm not sure I understand. 

I mean should we group tags checks by returned role, now it looks like

if (bla)
  return role1;
if (bla2)
  return role2;
if (bla3)
  return 3;
if (bla4)
 return role1;

isn't it nicer to keep bla and bla4 together?

Comment 57

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #56)
> (In reply to comment #54)
> 
> > xml-roles isn't an atom in the DOM sense.
> 
> what is the DOM sense of atoms?

I mean it is the only place I've seen atom types outside our code.

> 
> > We don't treat it as an atom
> > elsewhere in our code.
> 
> we could and perhaps should, right?

Yeah, but neither of us are sure we like SetAccAttr (as per IRC ;) ). I can make this change.

> 
> > We don't need it but I prefer it.
> 
> why? no really why? for stuffs that we're not going to spread through the code

Long methods are generally not my favourite things. I just find grouping stuff like this to be more readable, and self documenting. But I'll make the change.

> 
> > > > should you move article and nav processing on top
> > > > > where div and blockquote is processed?
> > 
> > I'm not sure I understand. 
> 
> I mean should we group tags checks by returned role, now it looks like
> 
> if (bla)
>   return role1;
> if (bla2)
>   return role2;
> if (bla3)
>   return 3;
> if (bla4)
>  return role1;
> 
> isn't it nicer to keep bla and bla4 together?

Aha, this was the source of my strange question. I lost context of which method you were reviewing here. Yeah that's better - I changed this locally.

Comment 58

[alexander :surkov (:asurkov)]

(In reply to comment #57)

> Yeah, but neither of us are sure we like SetAccAttr (as per IRC ;) ). I can
> make this change.

it's code style issue but new bug is worth thing (as per irc) :)

> Long methods are generally not my favourite things. I just find grouping stuff
> like this to be more readable, and self documenting. But I'll make the change.

I don't like we create local variables, call not inline methods for few lines of code, I'm not sure but we can get excess string copy with this method if strings aren't smart enough.

> you were reviewing here. Yeah that's better - I changed this locally.

thanks

Comment 59

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: final patch

Comment 60

[David Bolter [:davidb] (NeedInfo me for attention)]

http://hg.mozilla.org/mozilla-central/rev/e2646aba6b16