Last Comment Bug 396632 - Should not require namespaces for ARIA property usage in text/html
: Should not require namespaces for ARIA property usage in text/html
Status: RESOLVED FIXED
: access
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Aaron Leventhal
:
:
Mentors:
http://www.mozilla.org/access/dhtml/n...
Depends on:
Blocks: simplearia
  Show dependency treegraph
 
Reported: 2007-09-18 17:55 PDT by Aaron Leventhal
Modified: 2007-09-24 18:19 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP (95.30 KB, patch)
2007-09-18 17:56 PDT, Aaron Leventhal
no flags Details | Diff | Splinter Review
Work in progress #2 (100.77 KB, patch)
2007-09-19 09:03 PDT, Aaron Leventhal
no flags Details | Diff | Splinter Review
Ready for review, will follow up with detailed patch description (92.15 KB, patch)
2007-09-19 11:40 PDT, Aaron Leventhal
no flags Details | Diff | Splinter Review
Use Surkov's suggestion for commenting nsIAccessibleDocument constants (98.56 KB, patch)
2007-09-19 19:37 PDT, Aaron Leventhal
no flags Details | Diff | Splinter Review
Address Surkov's comments (98.54 KB, patch)
2007-09-19 23:20 PDT, Aaron Leventhal
no flags Details | Diff | Splinter Review
Address Surkov's comments (99.01 KB, patch)
2007-09-20 09:53 PDT, Aaron Leventhal
no flags Details | Diff | Splinter Review
Address Surkov's comments (99.04 KB, patch)
2007-09-20 11:00 PDT, Aaron Leventhal
neil: superreview+
Details | Diff | Splinter Review
Address Neil's comments (94.06 KB, patch)
2007-09-20 19:23 PDT, Aaron Leventhal
no flags Details | Diff | Splinter Review
Fix comments, move some methods to nsAccUtils, add assertions for mutually exclusive arguments (101.36 KB, patch)
2007-09-21 07:13 PDT, Aaron Leventhal
surkov.alexander: review+
dsicore: approval1.9+
Details | Diff | Splinter Review

Description Aaron Leventhal 2007-09-18 17:55:40 PDT
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 2007-09-18 17:56:42 PDT
Created attachment 281397 [details] [diff] [review]
WIP
Comment 2 Aaron Leventhal 2007-09-19 06:27:02 PDT
David, what do you think of Hixie's suggestion? Henri thinks it is wise to move ahead with this.
Comment 3 Aaron Leventhal 2007-09-19 06:29:48 PDT
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) (Not reading bugmail or doing reviews until 2016-10-03) 2007-09-19 07:14:29 PDT
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-7 (busy September 14-25) 2007-09-19 08:23:44 PDT
Hixie's propoal is fine with me.
Comment 6 Aaron Leventhal 2007-09-19 09:03:46 PDT
Created attachment 281489 [details] [diff] [review]
Work in progress #2
Comment 7 alexander :surkov 2007-09-19 11:28:16 PDT
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 2007-09-19 11:40:02 PDT
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 2007-09-19 11:40:44 PDT
Created attachment 281508 [details] [diff] [review]
Ready for review, will follow up with detailed patch description
Comment 10 Aaron Leventhal 2007-09-19 12:02:54 PDT
David, Henri -- just saving you from lots of bugmail by un-cc'ing you during the review process.
Comment 11 alexander :surkov 2007-09-19 12:21:32 PDT
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 2007-09-19 19:31:22 PDT
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 2007-09-19 19:37:07 PDT
Created attachment 281587 [details] [diff] [review]
Use Surkov's suggestion for commenting nsIAccessibleDocument constants
Comment 14 alexander :surkov 2007-09-19 21:38:46 PDT
Why do you need to copy atomsNS array into gAriaAtomsNS, I mean why you can't have one array?
Comment 15 alexander :surkov 2007-09-19 21:41:49 PDT
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 2007-09-19 21:43:53 PDT
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 2007-09-19 21:47:13 PDT
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 2007-09-19 21:52:41 PDT
(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 2007-09-19 22:00:23 PDT
what is universal ARIA property? Is the term defined by ARIA spec?
Comment 20 alexander :surkov 2007-09-19 22:18:32 PDT
does mime type always is available/correct before document is loaded?
Comment 21 alexander :surkov 2007-09-19 22:22:40 PDT
Do I understand correct: now I can't define ARIA states in text/html document without setAttriubteNS usage?
Comment 22 Aaron Leventhal 2007-09-19 23:03:48 PDT
> 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 2007-09-19 23:09:34 PDT
> 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 2007-09-19 23:10:44 PDT
> > 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 2007-09-19 23:20:26 PDT
Created attachment 281611 [details] [diff] [review]
Address Surkov's comments
Comment 26 alexander :surkov 2007-09-20 08:55:28 PDT
(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 2007-09-20 09:02:19 PDT
You use 'ARIA properties' for ARIA states and properties. Would it be more correct to use ARIA attributes?
Comment 28 Aaron Leventhal 2007-09-20 09:09:26 PDT
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 2007-09-20 09:17:13 PDT
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 2007-09-20 09:35:57 PDT
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 2007-09-20 09:53:10 PDT
Created attachment 281680 [details] [diff] [review]
Address Surkov's comments
Comment 32 alexander :surkov 2007-09-20 10:00:13 PDT
thanks for new patch, btw, can translate onme in 'A universal ARIA property is onme' :)
Comment 33 alexander :surkov 2007-09-20 10:05:29 PDT
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 2007-09-20 10:06:55 PDT
(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 2007-09-20 10:12:22 PDT
GetAriaProperty and HasAriaProperty are much similar. Can you think to reduce code  dublication?
Comment 36 Aaron Leventhal 2007-09-20 10:14:06 PDT
(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 2007-09-20 10:19:37 PDT
> 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 2007-09-20 10:22:35 PDT
(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 2007-09-20 10:24:31 PDT
(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 2007-09-20 10:32:25 PDT
> 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 2007-09-20 11:00:26 PDT
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.
Comment 42 Marco Zehe (:MarcoZ) 2007-09-20 12:45:46 PDT
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 neil@parkwaycc.co.uk 2007-09-20 13:25:42 PDT
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 2007-09-20 17:55:21 PDT
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 2007-09-20 19:23:26 PDT
Created attachment 281767 [details] [diff] [review]
Address Neil's comments
Comment 46 alexander :surkov 2007-09-20 20:52:00 PDT
nit: add new line in comment to HasUniversalAriaProperty method to separate description from arguments
Comment 47 alexander :surkov 2007-09-20 20:53:28 PDT
(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 2007-09-20 20:56:03 PDT
nit:

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

please add a comment since you are here :)
Comment 49 alexander :surkov 2007-09-20 21:01:38 PDT
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 2007-09-20 21:21:06 PDT
(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 2007-09-20 21:26:31 PDT
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 2007-09-20 21:41:55 PDT
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 2007-09-20 21:43:28 PDT
btw, possibly it's worth to move FindDescendantPointingToID and etc to move into nsAccUtils?
Comment 54 neil@parkwaycc.co.uk 2007-09-21 01:17:29 PDT
Note that your comments on the eCheck enums are out of date now.
Comment 55 alexander :surkov 2007-09-21 04:28:36 PDT
(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 2007-09-21 06:51:43 PDT
(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 2007-09-21 07:13:51 PDT
Created attachment 281821 [details] [diff] [review]
Fix comments, move some methods to nsAccUtils, add assertions for mutually exclusive arguments
Comment 58 alexander :surkov 2007-09-21 07:38:56 PDT
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

Note You need to log in before you can comment on or make changes to this bug.