The default bug view has changed. See this FAQ.

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

RESOLVED FIXED

Status

()

Core
Disability Access APIs
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: Aaron Leventhal, Assigned: Aaron Leventhal)

Tracking

({access})

Trunk
access
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 8 obsolete attachments)

(Assignee)

Description

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

Comment 1

10 years ago
Created attachment 281397 [details] [diff] [review]
WIP
(Assignee)

Comment 2

10 years ago
David, what do you think of Hixie's suggestion? Henri thinks it is wise to move ahead with this.
(Assignee)

Comment 3

10 years ago
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.
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.
Hixie's propoal is fine with me.
(Assignee)

Comment 6

10 years ago
Created attachment 281489 [details] [diff] [review]
Work in progress #2
Attachment #281397 - Attachment is obsolete: true

Comment 7

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

Comment 8

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

Comment 9

10 years ago
Created attachment 281508 [details] [diff] [review]
Ready for review, will follow up with detailed patch description
Attachment #281489 - Attachment is obsolete: true
(Assignee)

Comment 10

10 years ago
David, Henri -- just saving you from lots of bugmail by un-cc'ing you during the review process.
(Assignee)

Updated

10 years ago
Attachment #281508 - Attachment description: Work in progress #3 -- tested, works, cleanup phase now → Ready for review, will follow up with detailed patch description
Attachment #281508 - Flags: review?(surkov.alexander)

Comment 11

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

Comment 12

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

Comment 13

10 years ago
Created attachment 281587 [details] [diff] [review]
Use Surkov's suggestion for commenting nsIAccessibleDocument constants
Attachment #281508 - Attachment is obsolete: true
Attachment #281587 - Flags: review?(surkov.alexander)
Attachment #281508 - Flags: review?(surkov.alexander)

Comment 14

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

Comment 15

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

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

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

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

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

Comment 20

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

Comment 21

10 years ago
Do I understand correct: now I can't define ARIA states in text/html document without setAttriubteNS usage?
(Assignee)

Comment 22

10 years ago
> 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]
(Assignee)

Comment 23

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

(Assignee)

Comment 24

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

Comment 25

10 years ago
Created attachment 281611 [details] [diff] [review]
Address Surkov's comments
Attachment #281587 - Attachment is obsolete: true
Attachment #281611 - Flags: review?(surkov.alexander)
Attachment #281587 - Flags: review?(surkov.alexander)

Comment 26

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

10 years ago
You use 'ARIA properties' for ARIA states and properties. Would it be more correct to use ARIA attributes?
(Assignee)

Comment 28

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

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

Comment 30

10 years ago
Use case of ariaPropTypes in public method is for Fire Vox, so it can save time not looking for extra attributes.
(Assignee)

Comment 31

10 years ago
Created attachment 281680 [details] [diff] [review]
Address Surkov's comments
Attachment #281611 - Attachment is obsolete: true
Attachment #281680 - Flags: review?(surkov.alexander)
Attachment #281611 - Flags: review?(surkov.alexander)

Comment 32

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

Comment 33

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

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

10 years ago
GetAriaProperty and HasAriaProperty are much similar. Can you think to reduce code  dublication?
(Assignee)

Comment 36

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

Comment 37

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

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

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

Comment 40

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

Comment 41

10 years ago
Created attachment 281690 [details] [diff] [review]
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.
Attachment #281680 - Attachment is obsolete: true
Attachment #281690 - Flags: superreview?(neil)
Attachment #281690 - Flags: review?(surkov.alexander)
Attachment #281680 - Flags: review?(surkov.alexander)

Comment 42

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

10 years ago
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;
}
Attachment #281690 - Flags: superreview?(neil) → superreview+
(Assignee)

Comment 44

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

Updated

10 years ago
(Assignee)

Comment 45

10 years ago
Created attachment 281767 [details] [diff] [review]
Address Neil's comments
Attachment #281767 - Flags: review?(surkov.alexander)

Comment 46

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

Comment 47

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

10 years ago
nit:

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

please add a comment since you are here :)

Comment 49

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

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

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

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

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

Updated

10 years ago
Attachment #281690 - Attachment is obsolete: true
Attachment #281690 - Flags: review?(surkov.alexander)

Comment 54

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

Comment 55

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

Comment 56

10 years ago
(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.



(Assignee)

Comment 57

10 years ago
Created attachment 281821 [details] [diff] [review]
Fix comments, move some methods to nsAccUtils, add assertions for mutually exclusive arguments
Attachment #281821 - Flags: review?(surkov.alexander)
(Assignee)

Updated

10 years ago
Attachment #281767 - Attachment is obsolete: true
Attachment #281767 - Flags: review?(surkov.alexander)

Updated

10 years ago
Attachment #281821 - Attachment is patch: true
Attachment #281821 - Attachment mime type: application/octet-stream → text/plain

Comment 58

10 years ago
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
Attachment #281821 - Flags: review?(surkov.alexander)
Attachment #281821 - Flags: review+
Attachment #281821 - Flags: approval1.9?
(Assignee)

Updated

10 years ago
Blocks: 391713
No longer blocks: 343213

Updated

10 years ago
Attachment #281821 - Flags: approval1.9? → approval1.9+
(Assignee)

Updated

10 years ago
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.