Last Comment Bug 762897 - implement UIA_AriaPropertiesPropertyId
: implement UIA_AriaPropertiesPropertyId
Status: RESOLVED FIXED
[good first bug][mentor=surkov.alexan...
: access
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: x86 Windows 7
: -- normal (vote)
: mozilla17
Assigned To: Andrew Quartey [:drexler]
:
: alexander :surkov
Mentors:
Depends on:
Blocks: uia
  Show dependency treegraph
 
Reported: 2012-06-08 07:05 PDT by alexander :surkov
Modified: 2012-08-11 19:55 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (1.21 KB, patch)
2012-07-31 10:50 PDT, Andrew Quartey [:drexler]
no flags Details | Diff | Splinter Review
patch (6.20 KB, patch)
2012-07-31 21:37 PDT, Andrew Quartey [:drexler]
no flags Details | Diff | Splinter Review
patch (5.97 KB, patch)
2012-08-02 13:50 PDT, Andrew Quartey [:drexler]
no flags Details | Diff | Splinter Review
patch (7.43 KB, patch)
2012-08-06 16:52 PDT, Andrew Quartey [:drexler]
no flags Details | Diff | Splinter Review
patch (7.97 KB, patch)
2012-08-07 11:38 PDT, Andrew Quartey [:drexler]
tbsaunde+mozbugs: review+
Details | Diff | Splinter Review

Description alexander :surkov 2012-06-08 07:05:44 PDT
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] 2012-07-31 10:50:32 PDT
Created attachment 647595 [details] [diff] [review]
patch
Comment 2 Trevor Saunders (:tbsaunde) 2012-07-31 11:15:46 PDT
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] 2012-07-31 11:25:54 PDT
True.Incorrect patch; belatedly noticed that an ARIA property's "aria-" prefix was being sub-stringed before being set.
Comment 4 Andrew Quartey [:drexler] 2012-07-31 21:37:48 PDT
Created attachment 647840 [details] [diff] [review]
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 2012-08-01 04:21:33 PDT
cool. wouldn't be the iterator approach nicer/faster?
Comment 6 Andrew Quartey [:drexler] 2012-08-01 11:12:27 PDT
(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 2012-08-01 23:38:52 PDT
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] 2012-08-02 13:50:51 PDT
Created attachment 648473 [details] [diff] [review]
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) 2012-08-02 21:30:55 PDT
(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 2012-08-02 23:16:20 PDT
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] 2012-08-06 16:52:41 PDT
Created attachment 649476 [details] [diff] [review]
patch
Comment 12 Trevor Saunders (:tbsaunde) 2012-08-06 19:25:57 PDT
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] 2012-08-07 11:38:32 PDT
Created attachment 649717 [details] [diff] [review]
patch

Fixed nits and logic per the last review comments.
Comment 14 Trevor Saunders (:tbsaunde) 2012-08-08 22:57:05 PDT
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] 2012-08-09 12:52:36 PDT
(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) 2012-08-09 19:34:26 PDT
> > 
> > >+  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] 2012-08-10 13:16:30 PDT
fixed style and other nits; sent to inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/683e8441bb24
Comment 18 Ryan VanderMeulen [:RyanVM] 2012-08-11 19:55:27 PDT
https://hg.mozilla.org/mozilla-central/rev/683e8441bb24

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