396632 - Should not require namespaces for ARIA property usage in text/html

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Aaron Leventhal]

Most ARIA usage is actually occuring in text/html.

Because the namespace for ARIA cannot be defined, we require that ARIA properties are set with setAttributeNS(). However, this is just extra hacky JavaScript code,and simply makes ARIA more difficult to use.

It does not make sense to require namespaced prefix for ARIA usage in
text/html. This goes along with Hixie's recommendation here:
https://bugzilla.mozilla.org/show_bug.cgi?id=391713#c10

For more information see http://developer.mozilla.org/en/docs/ARIA:_Accessible_Rich_Internet_Applications/Relationship_to_HTML_FAQ

We should allow:
aria-[propertyname] directly.

The old method of using setAttributeNS() for the ARIA namespaced property should still work, and in fact will override the aria-foo properties.

Comment 1

[Aaron Leventhal]

Attachment: WIP

Comment 2

[Aaron Leventhal]

David, what do you think of Hixie's suggestion? Henri thinks it is wise to move ahead with this.

Comment 3

[Aaron Leventhal]

BTW, Opera, who is also developing ARIA support, also wants this to happen. Microsoft has not announced anything but they've told me they back this as well. Yahoo is working on content, they also want this.

Comment 4

[Henri Sivonen (:hsivonen)]

I agree Hixie’s proposal is indeed better than my earlier comments in bug 391713, especially because Hixie’s proposal avoids adding the namespace complexity in HTML and it is possible to implement aria-* on a much faster schedule than any Namespaces in XML -style generic mechanism to HTML.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Hixie's propoal is fine with me.

Comment 6

[Aaron Leventhal]

Attachment: Work in progress #2

Comment 7

[alexander :surkov (:asurkov)]

looking shortly I see you replace string to constant usage (like eAria_disabled -> "disabled"). I suppose it's not all bug is about. I would suggested to do this reorganization and check in it before actual work. (Sure if the suggestion is applicable at all due to my very quick overview).

Comment 8

[Aaron Leventhal]

Surkov, that's actually part of the fix. It abstracts the ARIA property which allows the getter to check the namespaced property, the hyphenated property, or both, depending on what's necessary.

Comment 9

[Aaron Leventhal]

Attachment: Ready for review, will follow up with detailed patch description

Comment 10

[Aaron Leventhal]

David, Henri -- just saving you from lots of bugmail by un-cc'ing you during the review process.

Comment 11

[alexander :surkov (:asurkov)]

Comment on attachment 281508 [details] [diff] [review]
Ready for review, will follow up with detailed patch description

>Index: accessible/public/nsIAccessibleDocument.idl
>===================================================================
>RCS file: /cvsroot/mozilla/accessible/public/nsIAccessibleDocument.idl,v
>retrieving revision 1.16
>diff -p -u -3 -r1.16 nsIAccessibleDocument.idl
>--- accessible/public/nsIAccessibleDocument.idl	23 Jun 2007 08:25:29 -0000	1.16
>+++ accessible/public/nsIAccessibleDocument.idl	19 Sep 2007 17:50:52 -0000
>@@ -121,4 +121,29 @@ interface nsIAccessibleDocument : nsISup
>    *         to the document root.
>    */
>   nsIAccessible getAccessibleInParentChain(in nsIDOMNode aDOMNode);
>+
>+  const unsigned long eUnknownPropType = 0;
>+  const unsigned long eCheckNamespacedOnly = 1;
>+  const unsigned long eCheckHyphenatedOnly = 2;
>+  const unsigned long eCheckAny = 3;

looks would be better to describe each constants instead of their describtion inside the following up method.

Comment 12

[Aaron Leventhal]

1) Abstract ARIA attributes into an enum that points into one of two arrays of atoms:
gAriaAtomsNS or gAriaAtomsHyphenated. 
2) Each document tracks whether namespaced ARIA attributes (aria:foo), hyphenated (aria-foo) or both are allowed
3) nsAccUtils methods are now responsible for getting an ARIA property based on the enum
4) As an optimization for a batch of ARIA property gets, the types of allowed ARIA atributes can be determined first, and then passed in to the property getter. This means doc accessible doesn't need to be retrieved out of the hash table each time, to look that up
5) FindNeighorPointingToThis() is unnecessary
6) FindNeighborPointingToNode() can take an additional param for the ARIA property enum being used, instead of an attribute atom. 
7) The listener nsDocAccessible::AttributeChanged() checks for aria-foo attributes if they're possible. It also notices when namespaced attributes are used in text/html, and stores that fact so namespaced ARIA attributes are checked in the future

Comment 13

[Aaron Leventhal]

Attachment: Use Surkov's suggestion for commenting nsIAccessibleDocument constants

Comment 14

[alexander :surkov (:asurkov)]

Why do you need to copy atomsNS array into gAriaAtomsNS, I mean why you can't have one array?

Comment 15

[alexander :surkov (:asurkov)]

possibly eAria_latest or something else is better than eAria_none, I don't like 'none' doesn't say it's end of aria properties.

Comment 16

[alexander :surkov (:asurkov)]

EAriaProperty ariaAttribute;  // magic value of nsnull means last entry in map

There is contradiction here between property and attribute. It may confuse. Also 
attributeName
attributeValue
looks better than
ariaAttribute
attributeValue
because nothing says attributeValue is a value of ariaAttribute

Comment 17

[alexander :surkov (:asurkov)]

sorry, why +1

static nsIAtom* gAriaAtomsNS[eAria_none + 1];

because eAria_none is not valid aria attribute.


btw, in code we used 'ARIA', you use 'Aria'. We should follow unique way.

Comment 18

[alexander :surkov (:asurkov)]

(In reply to comment #14)
> Why do you need to copy atomsNS array into gAriaAtomsNS, I mean why you can't
> have one array?
> 

because atoms can't be used before InitXPAccessibility()?

Comment 19

[alexander :surkov (:asurkov)]

what is universal ARIA property? Is the term defined by ARIA spec?

Comment 20

[alexander :surkov (:asurkov)]

does mime type always is available/correct before document is loaded?

Comment 21

[alexander :surkov (:asurkov)]

Do I understand correct: now I can't define ARIA states in text/html document without setAttriubteNS usage?

Comment 22

[Aaron Leventhal]

> what is universal ARIA property? Is the term defined by ARIA spec?
It is a property that can be defined on any element even if there is no role set.

> does mime type always is available/correct before document is loaded?
At that time it is always set.

> Do I understand correct: now I can't define ARIA states in text/html 
> document without setAttriubteNS usage?
Right now you cannot. With this patch you can use aria-[property_name]

Comment 23

[Aaron Leventhal]

> Why do you need to copy atomsNS array into gAriaAtomsNS, I mean why you can't
> have one array?
I wasn't able to just declare the array globallys, for example in nsARIAMap.cpp, I guess because AddRefAtoms() weren't available yet. Once your review is done I'll check with Neil to see if there's a better way.

> possibly eAria_latest or something else is better than eAria_none,
> I don't like 'none' doesn't say it's end of aria properties.
Right, but I use it to mean _none as well, as you'll see. So if I have to choose _none looks better for the usage in nsAccessible.cpp

> EAriaProperty ariaAttribute;  // magic value of nsnull means last entry in map
I will change it back to attributeName then

> sorry, why +1
> static nsIAtom* gAriaAtomsNS[eAria_none + 1];
Okay I will look and see if we can remove that last entry

> btw, in code we used 'ARIA', you use 'Aria'. We should follow unique way.
I decided Aria was easier to read. Can we use ARIA in class/file name of nsARIAMap, but Aria for everything else?

Comment 24

[Aaron Leventhal]

> > Why do you need to copy atomsNS array into gAriaAtomsNS, I mean why you can't
> > have one array?
> because atoms can't be used before InitXPAccessibility()?
Right.

Comment 25

[Aaron Leventhal]

Attachment: Address Surkov's comments

Comment 26

[alexander :surkov (:asurkov)]

(In reply to comment #22)
> > what is universal ARIA property? Is the term defined by ARIA spec?
> It is a property that can be defined on any element even if there is no role
> set.

I mean it would be fine to have a definition of the term then in comment to method.

Comment 27

[alexander :surkov (:asurkov)]

You use 'ARIA properties' for ARIA states and properties. Would it be more correct to use ARIA attributes?

Comment 28

[Aaron Leventhal]

The call them states and properties in the spec. However, we're pushing to have them just call them properties, since there's no good and useful distinction.

Comment 29

[alexander :surkov (:asurkov)]

Comment on attachment 281611 [details] [diff] [review]
Address Surkov's comments

change guid please.

Should be the method and constants be defined as part of private or public interface. If you think it should be in public interface then it makes sense to have readonly attribute unsigned long ariaPropTypes() instead of method. Though I do not thing usecase when it is needed in public interface.

>+  /**
>+   * Returns the type of ARIA properties which should be checked in this document. For text/html,
>+   * documents, this value can change during the life of the document, if setAttributeNS()
>+   * is used to set an ARIA property.

would be better to move the comment about text/html to eCheckHyphenatedOnly and add it changed to eCheckAny.

>+  /**
>+   * Only check properties in the form of:

nit: attributes

>+  /**
>+   * eCheckHyphenatedOnly  If aria-[propname] is 

nit: remove eCheckHyphenatedOnly  

allowed -- this is only checked in text/html
>+   */
>+  const unsigned long eCheckHyphenatedOnly = 2;
>+  
>

Comment 30

[Aaron Leventhal]

Use case of ariaPropTypes in public method is for Fire Vox, so it can save time not looking for extra attributes.

Comment 31

[Aaron Leventhal]

Attachment: Address Surkov's comments

Comment 32

[alexander :surkov (:asurkov)]

thanks for new patch, btw, can translate onme in 'A universal ARIA property is onme' :)

Comment 33

[alexander :surkov (:asurkov)]

nsAccUtils::HasAriaProperty takes an ariaType as an argument and request it from document when eUnknownPropType is given. But eUnknownPropType is valid type when mimetype is not specified. Would be better to have a value for ariaPropType that means uninitialized?

Comment 34

[alexander :surkov (:asurkov)]

(In reply to comment #23)
> > Why do you need to copy atomsNS array into gAriaAtomsNS, I mean why you can't
> > have one array?
> I wasn't able to just declare the array globallys, for example in
> nsARIAMap.cpp, I guess because AddRefAtoms() weren't available yet. Once your
> review is done I'll check with Neil to see if there's a better way.

please fix this before or after checking in this patch but it should be fixed.

Comment 35

[alexander :surkov (:asurkov)]

GetAriaProperty and HasAriaProperty are much similar. Can you think to reduce code  dublication?

Comment 36

[Aaron Leventhal]

(In reply to comment #33)
> nsAccUtils::HasAriaProperty takes an ariaType as an argument and request it
> from document when eUnknownPropType is given. But eUnknownPropType is valid
> type when mimetype is not specified. Would be better to have a value for
> ariaPropType that means uninitialized?

There's no real case when the document doesn't know. I'll make there document never returns that.

Also fixed onme.

I'll look to see if I can reduce code duplication, but basically I'm avoiding extra string copies when I can.

Comment 37

[Aaron Leventhal]

> GetAriaProperty and HasAriaProperty are much similar. Can you think to reduce
> code  dublication?
Actually I tried to remove the code duplication by passing in aValue as a pointer which can be null, and all it does is make the code more confusing. It's not smaller enough to make that worth it.

eUnknownPropType can't actually be returned by the document. So it's the same as undefined.

Comment 38

[alexander :surkov (:asurkov)]

(In reply to comment #36)

> I'll look to see if I can reduce code duplication, but basically I'm avoiding
> extra string copies when I can.
> 

btw, for this case 

nsAutoString multiselectable;
 if (nsAccUtils::GetAriaProperty(content, mWeakShell, eAria_multiselectable, multiselectable) &&multiselectable.EqualsLiteral("true")) {

you can avoid if you'll introduce IsAriaPropertyValue like nsIContent::isAttrValue()

Comment 39

[alexander :surkov (:asurkov)]

(In reply to comment #37)
> > GetAriaProperty and HasAriaProperty are much similar. Can you think to reduce
> > code  dublication?
> Actually I tried to remove the code duplication by passing in aValue as a
> pointer which can be null, and all it does is make the code more confusing.
> It's not smaller enough to make that worth it.

Agree, I do not see a nice way too now

> eUnknownPropType can't actually be returned by the document. So it's the same
> as undefined.
> 

So you should say about this in its documentation.

Comment 40

[Aaron Leventhal]

> you can avoid if you'll introduce IsAriaPropertyValue like
> nsIContent::isAttrValue()
I thought about it but it's not necessary for the few places we have it, which are rare. If it was used in a very common core loop I'd add it.

> So you should say about this in its documentation.
Okay

Comment 41

[Aaron Leventhal]

Attachment: Address Surkov's comments

Neil, any good way to improve the initialization of these arrays of atoms? Right now it essentially has an extra copy of each of the two tables.

Comment 42

[Marco Zehe (:MarcoZ)]

I tested this patch on the Alert, Button, Checkbox, Columnheader, and Tree examples and compared with today's regular nightly. I haven't found any difference in behaviour or rendering.

Comment 43

[[e-mail]]

Comment on attachment 281690 [details] [diff] [review]
Address Surkov's comments

I'm marking this sr+ because I think your approach is sufficiently reasonable and you might not like any of my comments.

>+  readonly attribute unsigned long ariaPropTypes;
>+  const unsigned long eUnknownPropType = 0;
>+  const unsigned long eCheckNamespacedOnly = 1;
>+  const unsigned long eCheckHyphenatedOnly = 2;
>+  const unsigned long eCheckAny = 3;
I think you should treat these flags as bitmasks, and call them eCheckNamespaced, eCheckHyphenated and eCheckBoth. See below.

>  *  When no nsIAccessibleRole neum mapping exists for an ARIA role, the
nsIAccessibleRole enum mapping

>+enum EAriaProperty {    // Don't forget to also change nsAccessibilityAtomsList.h and twice in nsAccessNode.cpp
You need an nsAriaPropertyList.h which consists of lines like this:
ARIA_PROPERTY(activedescendant)
ARIA_PROPERTY(atomic)
...
ARIA_PROPERTY(valuemax)

You would then do
#define ARIA_PROPERTY(atom) eAria_##atom,
enum eAriaProperty {
#include "nsAriaPropertyList.h"
  eAria_none);
#undef ARIA_PROPERTY

>+  static nsIAtom* gAriaAtomsNS[eAria_none];
>+  static nsIAtom* gAriaAtomsHyphenated[eAria_none];
You could init them this way perhaps:
nsAccessibilityAtoms::AddRefAtoms();
#define ARIA_PROPERTY(atom) gAriaAtomsNS[eAria_##atom] = nsAccessibilityAtoms::atom;
#include "nsAriaPropertyList.h"
#undef ARIA_PROPERTY
#define ARIA_PROPERTY(atom) gAriaAtomsHyphenated[eAria_##atom] = nsAccessibilityAtoms::aria_##atom;
#include "nsAriaPropertyList.h"
#undef ARIA_PROPERTY

Or maybe you could make these nsIAtom** and write this:
#define ARIA_PROPERTY(atom) &nsAccessibilityAtoms::atom,
nsIAtom** nsARIAMap::gAriaAtomPtrsNS[eAria_none] = {
#include "nsAriaPropertyList.h"
};
#undef ARIA_PROPERTY
#define ARIA_PROPERTY(atom) &nsAccessibilityAtoms::aria_##atom,
nsIAtom** nsARIAMap::gAriaAtomPtrsHyphenated[eAria_none] = {
#include "nsAriaPropertyList.h"
};
#undef ARIA_PROPERTY
You'd need to change all your deferences, which would be annoying.

Or maybe you could combine the two:
nsAccessibilityAtoms::AddRefAtoms();
for (int i = 0; i < eAria_none; i++) {
  gAriaAtomsNS[i] = *gAriaAtomPtrsNS[i];
  gAriaAtomsHyphenated[i] = *gAriaAtomPtrsHyphenated[i];
}

Or, is it possible to declare the atoms in the correct memory order? That is, so that the first eAria_none atoms are in fact the gAriaAtomsNS and the next eAria_none atoms are the gAriaAtomsHyphenated? Then you could write static const nsIAtom** gAriaAtomsNS = &nsAccessibilityAtoms::activedescendant;
#define ARIA_PROPERTY(atom) ACCESSIBILITY_ATOM(atom, #atom)
#include nsAriaPropertyList.h
#undef ARIA_PROPERTY
#define ARIA_PROPERTY(atom) ACCESSIBILITY_ATOM(aria_#atom, "aria-"#atom)
#include nsAriaPropertyList.h
#undef ARIA_PROPERTY

>+  if (aAriaPropTypes == nsIAccessibleDocument::eCheckNamespacedOnly ||
>+      aAriaPropTypes == nsIAccessibleDocument::eCheckAny) {
>+    if (aContent->HasAttr(kNameSpaceID_WAIProperties, nsARIAMap::gAriaAtomsNS[aProperty])) {
>+      return PR_TRUE;
>+    }
>+    if (aAriaPropTypes == nsIAccessibleDocument::eCheckNamespacedOnly) {
>+      return PR_FALSE;  // Don't fall through to hyphenated check
>+    }
>+  }
>+  // In HTML we allow the aria-foo hyhpenated (non-namespaced) properties
>+  return aContent->HasAttr(kNameSpaceID_None,
>+                           nsARIAMap::gAriaAtomsHyphenated[aProperty]);
So, I would write this as
return ((aAriaPropTypes & nsIAccessibleDocument::eCheckNamespaced) &&
        aContent->HasAttr(kNameSpaceID_WAIProperties,
                          nsARIAMap::gAriaAtomsNS[aProperty])) ||
       ((aAriaPropTypes & nsIAccessibleDocument::eCheckHyphenated) &&
        aContent->HasAttr(kNameSpaceID_None,
                          nsARIAMap::gAriaAtomsHyphenated[aProperty]));

>+  if (aNameSpaceID == kNameSpaceID_WAIProperties &&
>+      mAriaPropTypes == eCheckHyphenatedOnly) {
>+    // Using setAttributeNS() in HTML to set namespaced ARIA properties.
>+    // From this point forward, check both kinds, and namespaced properties will
>+    // take precedence, since they are being set dynamically only.
>+    mAriaPropTypes = eCheckAny;
>+  }
It comes to the same thing, but you could tweak the test and/or assignment:
if (aNameSpaceID == kNameSpaceID_WAIProperties) {
  // boring^H^H^H^H^H^H informative comment as before
  mAriaPropTypes |= eCheckNamespaced;
}

Comment 44

[Aaron Leventhal]

Testcase using the hyphenated properties (note that the tri-state checkbox is not supported by any known screen reader yet)
http://www.mozilla.org/access/dhtml/new/checkbox

Comment 45

[Aaron Leventhal]

Attachment: Address Neil's comments

Comment 46

[alexander :surkov (:asurkov)]

nit: add new line in comment to HasUniversalAriaProperty method to separate description from arguments

Comment 47

[alexander :surkov (:asurkov)]

(In reply to comment #40)
> > you can avoid if you'll introduce IsAriaPropertyValue like
> > nsIContent::isAttrValue()
> I thought about it but it's not necessary for the few places we have it, which
> are rare. If it was used in a very common core loop I'd add it.
> 

Ok, it's up to you. But I really would like to see the method to set ARIA attribute to keep the logic in the same place (I'm about nsAccessible::SetSelected() method).

Comment 48

[alexander :surkov (:asurkov)]

nit:

> nsresult GetTextFromRelationID(EAriaProperty aIDProperty, nsString &aName);

please add a comment since you are here :)

Comment 49

[alexander :surkov (:asurkov)]

I would suggested not to get rid aRelationNameSpaceID argument of FindDescendantPointingToID end etc. Though we do not use it know but we may need it in future.

Comment 50

[alexander :surkov (:asurkov)]

(In reply to comment #47)
> (In reply to comment #40)
> > > you can avoid if you'll introduce IsAriaPropertyValue like
> > > nsIContent::isAttrValue()
> > I thought about it but it's not necessary for the few places we have it, which
> > are rare. If it was used in a very common core loop I'd add it.
> > 
> 
> Ok, it's up to you.

But you have this at least in 4 places, looks reasonable to have own method for this.

Comment 51

[alexander :surkov (:asurkov)]

I don't like two places:
1) new attribute aAriaProperty for FindNeighbourPointingToNode that is mutually exclusive with aRelationAttr argument

2) char *ariaPropertyString[] = { "live", "channel", "atomic", "relevant", "datatype", "level",
...

EAriaProperty ariaPropertyEnum[] = { eAria_live, eAria_channel, eAria_atomic, eAria_relevant,

Would be fine like you have for ARIA properties (nsARIAProps.h). But possibly it's not worth to do.

Comment 52

[alexander :surkov (:asurkov)]

Now GetAttrValue() can be easy used for successors of nsAccessible like xul:numberbox. Your version won't.

We get something insane with ariaEnum because our methods should take both arguments ariaEnum and attrAtomName that are mutually exclusive. Can you see a way out?

Comment 53

[alexander :surkov (:asurkov)]

btw, possibly it's worth to move FindDescendantPointingToID and etc to move into nsAccUtils?

Comment 54

[[e-mail]]

Note that your comments on the eCheck enums are out of date now.

Comment 55

[alexander :surkov (:asurkov)]

(In reply to comment #54)
> Note that your comments on the eCheck enums are out of date now.
> 

yes, I see and like the current way more.

Comment 56

[Aaron Leventhal]

(In reply to comment #49)
> I would suggested not to get rid aRelationNameSpaceID argument of
> FindDescendantPointingToID end etc. Though we do not use it know but we may
> need it in future.

I don't believe we're going to need it. Namespaces are out of favor in web standards development, currently.

> We get something insane with ariaEnum because our methods should take both
> arguments ariaEnum and attrAtomName that are mutually exclusive. Can you see a
> way out?
How about an assertion that only one is passed in?

> btw, possibly it's worth to move FindDescendantPointingToID and etc to move
> into nsAccUtils?
Sure.

> Now GetAttrValue() can be easy used for successors of nsAccessible like
> xul:numberbox. Your version won't.
We will need to reimplement it for XUL numberbox. It's a tiny method, that's not a problem. Making it handle both cases will just make it larger.

For the other suggestions, I do not see enough to gain to implement them.

Comment 57

[Aaron Leventhal]

Attachment: Fix comments, move some methods to nsAccUtils, add assertions for mutually exclusive arguments

Comment 58

[alexander :surkov (:asurkov)]

Comment on attachment 281821 [details] [diff] [review]
Fix comments, move some methods to nsAccUtils, add assertions for mutually exclusive arguments

ok. looks ok for me