342288 - New ATK: Use object attributes to expose tag name, heading level

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Aaron Leventhal]

In atk/nsAccessibleWrap.h we define USE_ATK_OBJECT_ATTRIBUTES when libatk supports object attributes. When it does, we need to expose the current tag. If the role is heading we need to expose the heading level via "Heading level = n".

Ultimately we may expose more information there, such as CSS or DOM attributes, but this is a good start.

Comment 1

[Mike Gao]

Attachment: patch v1, not finished yet

Comment 2

[Aaron Leventhal]

> NS_IMETHODIMP nsAccessible::GetAttributes(nsIArray **attrArray)

Call it aAttrArray (parameters always have an "a" prefix)

Comment 3

[Mike Gao]

Attachment: patch v2 draft, not finished yet

have to leave now. will finish it later.

Comment 4

[Aaron Leventhal]

+  void getAttributes (out unsigned long count , [retval, array, size_is(count)] out char attributes);
I think what we want is:
+  void getAttributes([retval] out unsigned long count,
                      [array, size_is(count)] out AString attrNames,
                      [array, size_is(count)] out AString attrValues);

Ginn, does that sound right?

Comment 5

[Mike Gao]

I we use AString, we will get this error:

./nsIAccessible.idl:251: Error: [domstring], [utf8string], [cstring], [astring] types cannot be used in array parameters

I intended to use one array and store the attributes as : { name1, value1, name2, value2, name3, value3...}.

Yours is to store as: 
( name1, name2, name3...}
( value1, value2, value3...}

Which do you think is better?

It would be very nice if any of you know where the full document/manual about xpcom is. Docs on http://www.mozilla.org/projects/xpcom/ are not in that detail. For example, keyword array is not listed on http://www.mozilla.org/scriptable/xpidl/notes/keywords.txt .

Comment 6

[Aaron Leventhal]

1 array will be a strange interface. A public interface needs to be obvious.

Why is nsIBinaryInputStream.idl able to use AString in that way?

Documentation in this project is whatever you can find with Google and LXR, or by asking on the newsgroups or IRC. I believe there are a number of newsgroups that would be applicable if you can't get answers on IRC because of the timezone difference. Look at http://www.mozilla.org/community/developer-forums.html
For example try mozilla.dev.tech.xpcom

Comment 7

[Mike Gao]

We can use 2 arrays as below:
  void getAttributes (out unsigned long countname, out unsigned long countvalue,
                      [array, size_is(countname)] out wstring attrNames,
                      [array, size_is(countvalue)] out wstring attrValues);

Someone recommends to use nsIPropertyElement:
  void getAttributestest (out unsigned long count, [array, size_is(count)] out nsIPropertyElement Attribute);

But nsIPropertyElement's key is nsACString, which is narrow char string, is that ok for name?

Comment 8

[Aaron Leventhal]

A narrow char string is okay for the names. The values need to be wide strings.

Comment 9

[Mike Gao]

Attachment: patch v3

I am not sure about the array length. And it was not tested yet.

Comment 10

[Mike Gao]

I will use nsIMutableArray for that.

Comment 11

[Mike Gao]

Attachment: patch v4, use nsIArray and nsIPropertyElement

Comment 12

[Aaron Leventhal]

Since you're only returning one thing, i believe you can go back to using |readonly attribute nsIArray attributes;|

nsHyperTextAccessible::GetAttributes() needs to call up to it's parent class's GetAttributes(), and then add to that. This is similar to how GetState() is implemented by various classes. First it calls [myParentClass]::GetState() and then it logically or's any more relevant states in.

So nsHyperTextAccessible::GetAttributes() first needs to call up so that the array is created and the first attributes are added. Then it can add its own heading level attribute, if necessary.

Comment 13

[Mike Gao]

Attachment: patch v5

Comment 14

[Aaron Leventhal]

Comment on attachment 234100 [details] [diff] [review]
patch v5

First, in nsAccessible::GetAttributes()
+  nsACString* keystring = nsnull;
+  nsAString* valuestring = nsnull;
No, you're going to be accessing the null pointer -- crash!
Look how strings are used in most of the rest of the code.
If you declare them like this you won't even need the * operator:
nsCAutoString keyString;
nsAutoString valueString;
These are allocated on the stack until they get to a certain size like 64 bytes or something, 
when they get transferred to the heap. Very convenient.

+  nsCOMPtr<nsIPropertyElement> attrElement;
+
+  nsCOMPtr<nsIContent> content(do_QueryInterface(mDOMNode));
+  nsIAtom *tag = content->Tag();
You can get the current tag as a string. We care about the tag of any element, not just headings.
So use nsIDOMElement instead of nsIContent, and use the GetLocalName() method.

Now, in nsHyperTextAccessible::GetAttributes()
+  nsCOMPtr<nsIContent> content(do_QueryInterface(mDOMNode));
+  nsIAtom *tag = content->Tag();
+  if (tag == nsAccessibilityAtoms::h1) {
This is good to use the atom in this case, but use a switch statement as I showed you on IRC. 
You're comparing the same thing many times. It also looks better, doesn't it?
Also, use interCaps for variable names.
switch (tag) {
  case nsAccessibilityAtoms::h1 : headLevel = 1; break;
  case nsAccessibilityAtoms::h2 : headLevel = 2; break;
  case nsAccessibilityAtoms::h3 : headLevel = 3; break;
  ...
}

+  if (headlevel) {
+    char levelstr[2];
+    sprintf(levelstr, "%d", headlevel);
We really avoid direct use of the C char* functions in Mozilla, for security and platform compatibility reasons.
You won't need this as you'll see when we get to assigning the value.

+    (*keystring).AssignLiteral("Heading Level");
Let's just call it "level". I want to keep the key strings all lowercase, to avoid any confusion. Also, we already know it's a heading.

+    (*valuestring).AssignLiteral(levelstr);
valueString.AppendInt(headLevel);
Sorry that we don't have better string docs! You can only find out about that by scanning the code or asking on IRC/newsgroups.

In atk/nsAccessibleWrap.h you have an extra whitespace change, please undo that.

Not all methods need to be NS_IMETHOD. In fact you don't use the return value of AddAttribute at all, so just make it void. Even if you did, NS_IMETHOD is only for interface methods, not for helper methods. You'd use nsresult there if you wanted to use the return value for something. Does AppendElement() return an nsresult? It probably can return an out of memory error. So you could keep AddAttribute() as nsresult and do:
return aArray->AppendElement(attrElement, PR_FALSE);

Comment 15

[Mike Gao]

Attachment: patch v6

Comment 16

[Aaron Leventhal]

Still no switch statement in nsHyperTextAccessible?

Comment 17

[Aaron Leventhal]

Don't need this at the end of nsHyperTextAccessible::GetAttributes() since it's already true:
+ *aAttributes = mutableArray;

In general in Mozilla they declare variables right before they use them, unless it's necessary to move it outside of the block. So, the strings can be declared right before you use them. In fact that helps make the assignment easier. I *think* you can do this, but I'm not sure:
nsCAutoString keyString("level");

Important. You still have not fixed nsAccessible::GetAttributes() so that *any* element gets a tag attribute, not just for headings! You don't need the if h1, h2, h3 stuff at all. Just assign the tag name from any element directly into the valueString.

Comment 18

[Mike Gao]

Attachment: patch v7

Comment 19

[Aaron Leventhal]

How is the nsIAccessible testing going? That's the first step to making this work.

In terms of testing with AT-SPI, Ginn Chen has cleared the way for that He has fixed some AT-SPI issues with object attributes, and also fixed bug 347983 so that the #ifdef USE_FOO preprocessor stuff is no longer necessary (recent atk headers are included in the mozilla source code tree now).

So, update your build and remove the use of USE_ATK_OBJECT_ATTRIBUTES (always support object attributes now). Next, update your AT-SPI to the tip. There may be a lot of dependencies. But, you should be able to test object attributes via at-spi.

Comment 20

[Mike Gao]

Attachment: patch v8

Comment 21

[Mike Gao]

Attachment: use nsIPersistentProperties instead of nsIArray

I used pypoke to test this new feature. I can see the tag name and heading level are exposed.

Comment 22

[Mike Gao]

Attachment: The screen capture of the object attributes exposed on pypoke

Comment 23

[Aaron Leventhal]

Comment on attachment 236062 [details] [diff] [review]
use nsIPersistentProperties instead of nsIArray

Very good. Getting close.

1. For nsAccessible::GetAttributes()
In the first line null out the out param, so that if you return early it's empty:
*aAttributes = nsnull;

+ nsAutoString aValue; 
We only use the a prefix for parameters. In this context it's not a parameter. 
By looking at the variable name you should be able to tell if it is in the method declaration.
I would do:
+ nsAutoString oldValueUnused;
BTW, I had to look at the method to see what is being passed back. The IDL should be changed to
state that the old value is being passed back. If you want you can make that
part of your next patch or file a bug on updating the IDL comments.

+    nsCAutoString keyString("tag");
+    attributes->SetStringProperty(keyString, tagName, aValue);
I would do:

+    attributes->SetStringProperty(NS_LITERAL_CSTRING("tag"), tagName, oldValueUnused);


+  nsCOMPtr<nsIPersistentProperties> attributes = 
+      do_CreateInstance(NS_PERSISTENTPROPERTIES_CONTRACTID);  
+  NS_ENSURE_STATE(attributes);
  
If you want you can do:
+  *aAttributes = do_CreateInstance(NS_PERSISTENTPROPERTIES_CONTRACTID);
That way you can avoid the extra temporary variable and since it AddRefs for you you won't need these 2 lines:
+  *aAttributes = attributes;
+  NS_ADDREF(*aAttributes);


2. For AtkAttributeSet *getAttributesCB(AtkObject *aAtkObj)
+        nsresult rv;
+        nsCOMPtr<nsISimpleEnumerator> propEnum;
+        rv = attributes->Enumerate(getter_AddRefs(propEnum));
This can be:
+        nsCOMPtr<nsISimpleEnumerator> propEnum;
+        nsresult rv = attributes->Enumerate(getter_AddRefs(propEnum));


+        if (NS_FAILED(rv)) {
+            return nsnull; 
+        }
This and other similar code in this method could be shortened to:
+        NS_ENSURE_SUCCESS(rv, nsnull);

  

+            nsCOMPtr<nsIPropertyElement> propElem = do_QueryInterface(sup, &rv);
+            if (NS_FAILED(rv)) {
+                return nsnull;
+            }
I would use NS_ENSURE_TRUE or NS_ENSURE_STATE (doesn't matter which but mozilla/accessible usually uses NS_ENSURE_TRUE for whatever reason):
+            nsCOMPtr<nsIPropertyElement> propElem = do_QueryInterface(sup);
             NS_ENSURE_TRUE(propElem, nsnull)

+            nsCAutoString aName;
...
+            nsCAutoString aValue;
Use name and value, these aren't parameters within this method.


3. For nsHyperTextAccessible::GetAttributes()

+  nsAccessibleWrap::GetAttributes(aAttributes);
Make sure  to check if *aAttributes is null here

+    nsCAutoString keyString("level");
Same comment as for "tag" about -- use NS_LITERAL_CSTRING inline within SetStringProperty

Comment 24

[Mike Gao]

Attachment: patch addressing Aaron's comments

*aAttributes = do_CreateInstance(NS_PERSISTENTPROPERTIES_CONTRACTID);

does not seem to work:(?)

nsAccessible.cpp:1891: error: cannot convert 'const nsCreateInstanceByContractID' to 'nsIPersistentProperties*' in assignment

So I keep this in the old way. Will update if I can find a better method.

Comment 25

[[e-mail]]

Comment on attachment 236173 [details] [diff] [review]
patch addressing Aaron's comments

>   /**
>+   * Attributes of accessible
>+   */
>+  readonly attribute nsIPersistentProperties attributes;
>+
When you make a change like this you need to change the uuid too.

>+  *aAttributes = nsnull;

>+  *aAttributes = attributes;
>+  NS_ADDREF(*aAttributes);
Since you so nicely cleared aAttributes above you can use
attributes.swap(*aAttributes);

>+  NS_ENSURE_TRUE(*aAttributes, nsnull);
This is wrong, because this function returns an nsresult.

>+  }
>+
>+   return  NS_OK;
Nit: incorrect indentation on this line.

sr=me with these fixed.

Comment 26

[Mike Gao]

Attachment: patch addressing Neil's comments

Comment 27

[Mike Gao]

Need I request review again?

(In reply to comment #26)
> Created an attachment (id=236284) [edit]
> patch addressing Neil's comments
> 

Comment 28

[Aaron Leventhal]

I had to change the declaration of GetAttributes in nsHyperTextAccessible and move it out of protected into public.

  NS_IMETHOD GetAttributes(nsIPersistentProperties **aAttributes);

It was nsresult.