762897 - implement UIA_AriaPropertiesPropertyId

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

UIA_AriaPropertiesPropertyId (http://msdn.microsoft.com/en-us/library/windows/desktop/ee684017%28v=vs.85%29.aspx):

"AriaProperties is a collection of Name/Value pairs with delimiters of = (equals) and ; (semicolon), for example, "checked=true;disabled=false""

I suggest to not follow the spec entirely for now (http://msdn.microsoft.com/en-us/library/windows/desktop/ee684013%28v=vs.85%29.aspx) and expose those ARIA attributes that we expose as object attributes. If this introduces a problems then let's deal with it as follow up.

Add a support of UIA_AriaPropertiesPropertyId into uiaRawElmProvider::GetPropertyValue():

see an example how to obtain needed ARIA attributes (http://mxr.mozilla.org/mozilla-central/source/accessible/src/generic/Accessible.cpp#1284)

how to generate BSTR string you can refer to existing code like AccessibleWrap::ConvertToIA2Attributes

Comment 1

[Andrew Quartey [:drexler]]

Attachment: patch

Comment 2

[Trevor Saunders (:tbsaunde)]

Comment on attachment 647595 [details] [diff] [review]
patch

>+      nsCOMPtr<nsIPersistentProperties> attributes;
>+      mAcc->GetAttributes(getter_AddRefs(attributes));
>+      attributes->GetStringProperty(NS_LITERAL_CSTRING("aria-"), ariaProperties);

I'm pretty sure that will get you an attribute whose name is "aria-" which I'm pretty sure will never exist, not the attirubtes like "checked" which came from aria-checked.

I think the right way to handle this would be to somehow factor the bit of GetAttributes() that gets aria properties out into its own function and then use that here somehow.

Comment 3

[Andrew Quartey [:drexler]]

True.Incorrect patch; belatedly noticed that an ARIA property's "aria-" prefix was being sub-stringed before being set.

Comment 4

[Andrew Quartey [:drexler]]

Attachment: patch

added Pair data structure to hold an ARIA attribute's name and associated value in addition to factoring out obtaining the aria attributes in Accessible.cpp.

Comment 5

[alexander :surkov (:asurkov)]

cool. wouldn't be the iterator approach nicer/faster?

Comment 6

[Andrew Quartey [:drexler]]

(In reply to alexander :surkov from comment #5)
> cool. wouldn't be the iterator approach nicer/faster?

No. https://developer.mozilla.org/en/XPCOM_array_guide#In-place_enumeration_4,

Comment 7

[alexander :surkov (:asurkov)]

I meant something like:

class ARIAAttrsIter
{
public:
  bool Next(nsAString& aName, nsAString& aValue);
private:
  nsIContent* mContent;
  PRUint32 mAttrIdx;
}

ARIAAttrsIter iter(mContent);
nsAutoString name, value;
while (iter.Next(name, value)) {
  // generate object attributes or UIA property
}

Comment 8

[Andrew Quartey [:drexler]]

Attachment: patch

Updated patch making use of the iterator approach; locally passed tests. 
Sent to Try: https://tbpl.mozilla.org/?tree=Try&rev=b2e9624a999c

Comment 9

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #7)
> I meant something like:
> 
> class ARIAAttrsIter
> {
> public:
>   bool Next(nsAString& aName, nsAString& aValue);
> private:
>   nsIContent* mContent;
>   PRUint32 mAttrIdx;
> }
> 
> ARIAAttrsIter iter(mContent);
> nsAutoString name, value;
> while (iter.Next(name, value)) {
>   // generate object attributes or UIA property
> }

Since lazyness won't ever help us avoid computations for current use cases I honestly don't know which is faster.

Since we can store the array of attributes on the stack, and very very rarely go beyond the preallocated size the non interator approach will have better I cache locality I would think,  the extra moves will be to the stack so probably neglagible, and the I believe we won't end up actually copying strings because of ref counted string buffers.  On the other hand the  iterator approach might end up being faster, but I'd hate to have to say without doing some bench marks, but I doubt its a huge difference either way.

Comment 10

[alexander :surkov (:asurkov)]

Comment on attachment 648473 [details] [diff] [review]
patch

Review of attachment 648473 [details] [diff] [review]:
-----------------------------------------------------------------

please fix the style, no spaces at the end of line and no tabs.

I would name the class as aria::AttrIterator and put it into nsARIAMap.h file. It's consistent to style we use (see AccIterator.h) and it belongs to aria stuffs rather than to accessible.

::: accessible/src/generic/Accessible.cpp
@@ +1227,5 @@
>  
>    // Expose object attributes from ARIA attributes.
> +  ARIAAttributeEnumerator attribIter(mContent);
> +  nsAutoString attribName;
> +  nsAutoString attribValue;

you can keep them on the same line, right? and you could call them as 'name' and value since they are too much locals.

@@ +3252,5 @@
> +////////////////////////////////////////////////////////////////////////////////
> +// ARIAAttributeEnumerator class
> +
> +ARIAAttributeEnumerator::ARIAAttributeEnumerator(nsIContent* aContent)
> +  :mContent(aContent), mAttrIdx(0)

no operators on new line

@@ +3260,5 @@
> +
> +bool
> +ARIAAttributeEnumerator::HasMore(nsAString& aAttrName, nsAString& aAttrValue)
> +{
> +  if(mAttrIdx < mAttrCount) {

space after if

@@ +3266,5 @@
> +    if (attr && attr->NamespaceEquals(kNameSpaceID_None)) {
> +      nsIAtom *attrAtom = attr->Atom();
> +      nsDependentAtomString attrStr(attrAtom);
> +      if (!StringBeginsWith(attrStr, NS_LITERAL_STRING("aria-"))) 
> +        return false; // Not ARIA

new line please

@@ +3269,5 @@
> +      if (!StringBeginsWith(attrStr, NS_LITERAL_STRING("aria-"))) 
> +        return false; // Not ARIA
> +      PRUint8 attrFlags = nsAccUtils::GetAttributeCharacteristics(attrAtom);
> +      if (attrFlags & ATTR_BYPASSOBJ)
> +        return false; // No need to handle exposing as obj attribute here

same

@@ +3271,5 @@
> +      PRUint8 attrFlags = nsAccUtils::GetAttributeCharacteristics(attrAtom);
> +      if (attrFlags & ATTR_BYPASSOBJ)
> +        return false; // No need to handle exposing as obj attribute here
> +      if ((attrFlags & ATTR_VALTOKEN) &&
> +         !nsAccUtils::HasDefinedARIAToken(mContent, attrAtom))

indent it properly please

@@ +3272,5 @@
> +      if (attrFlags & ATTR_BYPASSOBJ)
> +        return false; // No need to handle exposing as obj attribute here
> +      if ((attrFlags & ATTR_VALTOKEN) &&
> +         !nsAccUtils::HasDefinedARIAToken(mContent, attrAtom))
> +        return false; // only expose token based attributes if they are defined

same

Comment 11

[Andrew Quartey [:drexler]]

Attachment: patch

Comment 12

[Trevor Saunders (:tbsaunde)]

Comment on attachment 649476 [details] [diff] [review]
patch

> #include "Role.h"
> #include "States.h"
> 
> #include "nsIContent.h"
> #include "nsWhitespaceTokenizer.h"
>+#include "nsAttrName.h"
>+#include "nsAccUtils.h"

please keep nsAccUtils with a11y headers, and keep in alphabetical order.

>+
>+AttrIterator::AttrIterator(nsIContent* aContent) : 
>+  mContent(aContent), mAttrIdx(0)
>+{
>+  mAttrCount = mContent->GetAttrCount();
>+}

it might make more sense to put this in its own header so it can make the constructor inline.

>+AttrIterator::Next(nsAString& aAttrName, nsAString& aAttrValue)
>+{
>+  if (mAttrIdx < mAttrCount) {

nit, return early.

>+    const nsAttrName *attr = mContent->GetAttrNameAt(mAttrIdx++);

nit, type* name, and keep ++ on own line.

>+    if (attr && attr->NamespaceEquals(kNameSpaceID_None)) {

I believe you don't need the nul check since you know the index to be within the bounds.

>+      nsIAtom *attrAtom = attr->Atom();
>+      nsDependentAtomString attrStr(attrAtom);
>+      if (!StringBeginsWith(attrStr, NS_LITERAL_STRING("aria-"))) 
>+        return false; // Not ARIA

this is broken any time you have a nonaria- attribute before one with aria- in the array.  So for example <div role="scrollbar" aria-orientation="horizontal">

>+private:
>+  AttrIterator();
>+  AttrIterator(const AttrIterator&);
>+  AttrIterator& operator= (const AttrIterator&);

nit, add MOZ_DELETE

Comment 13

[Andrew Quartey [:drexler]]

Attachment: patch

Fixed nits and logic per the last review comments.

Comment 14

[Trevor Saunders (:tbsaunde)]

Comment on attachment 649717 [details] [diff] [review]
patch

>+  bool found = false;
>+  while (mAttrIdx < mAttrCount && !found) {
>+    const nsAttrName* attr = mContent->GetAttrNameAt(mAttrIdx);
>+    ++mAttrIdx;

nit, mAttrIdx++

>+      nsAutoString value;
>+      if (mContent->GetAttr(kNameSpaceID_None, attrAtom, value)) {
>+        aAttrName.Assign(Substring(attrStr, 5));
>+        aAttrValue.Assign(value);
>+        found = true;

just return true

>+class AttrIterator
>+{
>+public:
>+  AttrIterator(nsIContent* aContent): mContent(aContent), mAttrIdx(0)
>+  { 
>+    mAttrCount = mContent->GetAttrCount();
>+  }

do it as so
AttrIterator(nsIContent* aContent) :
  mContent(aContent), mIdx(0)
{ mAttrCount = mContent->GetAttributeCount(); }

>+  bool Next(nsAString& aAttrName, nsAString& aAttrValue);

you should be able to make it const

>+  nsIContent* mContent;

this can be const

>+  PRUint32 mAttrIdx;
>+  PRUint32 mAttrCount;

this one too

>+
>+    //ARIA Properties /shortcut

what is /shortcut about?

>+    case UIA_AriaPropertiesPropertyId: {
>+      nsAutoString ariaProperties;
>+
>+      aria::AttrIterator attribIter(mAcc->GetContent());
>+      nsAutoString attribName, attribValue;
>+      while (attribIter.Next(attribName, attribValue)) {
>+        ariaProperties.Append(attribName);
>+        ariaProperties.Append('=');
>+        ariaProperties.Append(attribValue);
>+        ariaProperties.Append(';');
>+      }
>+      //remove last delimiter:
>+      ariaProperties.Truncate(ariaProperties.Length()-1);

what happens if there was no aria attributes?

Comment 15

[Andrew Quartey [:drexler]]

(In reply to Trevor Saunders (:tbsaunde) from comment #14)
> Comment on attachment 649717 [details] [diff] [review]
> patch
> 
> >+  bool found = false;
> >+  while (mAttrIdx < mAttrCount && !found) {
> >+    const nsAttrName* attr = mContent->GetAttrNameAt(mAttrIdx);
> >+    ++mAttrIdx;
> 
> nit, mAttrIdx++

mAttrIdx++ will just create another temporary-that wont be used-before the increment. Thus ++mAttrIdx. Or is this part of the code-style? 


> >+  bool Next(nsAString& aAttrName, nsAString& aAttrValue);
> 
> you should be able to make it const

Cant make it const since we might modify the state via mAttrIdx's increments. 

> >+  nsIContent* mContent;
> 
> this can be const

Making this const requires that i use a const_cast<> for nsAccUtils::HasDefinedARIAToken
http://mxr.mozilla.org/mozilla-central/source/accessible/src/base/nsAccUtils.cpp#184. Is that ok? 
> 
> >+  PRUint32 mAttrIdx;
> >+  PRUint32 mAttrCount;
> 
> this one too

A const PRUint32 mAttrCount will require me to initialize it in the initialization list which will look like: 

 AttrIterator(nsIContent* aContent) : 
    mContent(aContent), mAttrIdx(0), mAttrCount(aContent->GetAttrCount())
  { }

A bit iffy on calling a function there. Doesn't 'feel' right.

Comment 16

[Trevor Saunders (:tbsaunde)]

> > 
> > >+  bool found = false;
> > >+  while (mAttrIdx < mAttrCount && !found) {
> > >+    const nsAttrName* attr = mContent->GetAttrNameAt(mAttrIdx);
> > >+    ++mAttrIdx;
> > 
> > nit, mAttrIdx++
> 
> mAttrIdx++ will just create another temporary-that wont be used-before the
> increment. Thus ++mAttrIdx. Or is this part of the code-style? 
> 
> 
> > >+  bool Next(nsAString& aAttrName, nsAString& aAttrValue);
> > 
> > you should be able to make it const
> 
> Cant make it const since we might modify the state via mAttrIdx's
> increments. 

its the style, also who cares about compilers that are to stupid to optimize away unused temporaries?

> > >+  nsIContent* mContent;
> > 
> > this can be const
> 
> Making this const requires that i use a const_cast<> for
> nsAccUtils::HasDefinedARIAToken
> http://mxr.mozilla.org/mozilla-central/source/accessible/src/base/nsAccUtils.
> cpp#184. Is that ok?

no, then just leave it as is, but please file a bug to make nsAccUtils::HasDefinedARIAToken() take a const nsIContent*  It looks like it should just work if you change the type of the argument.

> > >+  PRUint32 mAttrIdx;
> > >+  PRUint32 mAttrCount;
> > 
> > this one too
> 
> A const PRUint32 mAttrCount will require me to initialize it in the
> initialization list which will look like: 
> 
>  AttrIterator(nsIContent* aContent) : 
>     mContent(aContent), mAttrIdx(0), mAttrCount(aContent->GetAttrCount())
>   { }
> 
> A bit iffy on calling a function there. Doesn't 'feel' right.

yeah, ok, it probably doesn't actually help the compiler anyway.

Comment 17

[Andrew Quartey [:drexler]]

fixed style and other nits; sent to inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/683e8441bb24

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/683e8441bb24