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

RESOLVED FIXED

Status

()

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

People

(Reporter: Aaron Leventhal, Assigned: Mike Gao)

Tracking

(Blocks: 1 bug, {access})

Trunk
x86
Linux
access
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 10 obsolete attachments)

(Reporter)

Description

12 years ago
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.
(Reporter)

Updated

12 years ago
Blocks: 340809
(Reporter)

Updated

12 years ago
Summary: Use ATK object attributes to expose tag name, heading level → New ATK: Use object attributes to expose tag name, heading level
(Reporter)

Updated

12 years ago
No longer blocks: 333492
(Assignee)

Updated

12 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 1

12 years ago
Created attachment 231796 [details] [diff] [review]
patch v1, not finished yet
(Reporter)

Comment 2

12 years ago
> NS_IMETHODIMP nsAccessible::GetAttributes(nsIArray **attrArray)

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

(Assignee)

Comment 3

12 years ago
Created attachment 232693 [details] [diff] [review]
patch v2 draft, not finished yet

have to leave now. will finish it later.
Attachment #231796 - Attachment is obsolete: true
(Reporter)

Comment 4

12 years ago
+  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?
(Assignee)

Comment 5

12 years ago
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 .
(Reporter)

Comment 6

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

Comment 7

12 years ago
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?
(Reporter)

Comment 8

12 years ago
A narrow char string is okay for the names. The values need to be wide strings.
(Assignee)

Comment 9

12 years ago
Created attachment 233808 [details] [diff] [review]
patch v3

I am not sure about the array length. And it was not tested yet.
Attachment #232693 - Attachment is obsolete: true
(Assignee)

Comment 10

12 years ago
I will use nsIMutableArray for that.
(Assignee)

Comment 11

12 years ago
Created attachment 234019 [details] [diff] [review]
patch v4, use nsIArray and nsIPropertyElement
Attachment #233808 - Attachment is obsolete: true
(Reporter)

Comment 12

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

Comment 13

12 years ago
Created attachment 234100 [details] [diff] [review]
patch v5
Attachment #234019 - Attachment is obsolete: true
Attachment #234100 - Flags: review?(aaronleventhal)
(Reporter)

Comment 14

12 years ago
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);
Attachment #234100 - Flags: review?(aaronleventhal) → review-
(Assignee)

Comment 15

12 years ago
Created attachment 234221 [details] [diff] [review]
patch v6
Attachment #234100 - Attachment is obsolete: true
(Reporter)

Comment 16

12 years ago
Still no switch statement in nsHyperTextAccessible?
(Reporter)

Comment 17

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

Comment 18

12 years ago
Created attachment 234246 [details] [diff] [review]
patch v7
Attachment #234221 - Attachment is obsolete: true
(Reporter)

Comment 19

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

Comment 20

12 years ago
Created attachment 235378 [details] [diff] [review]
patch v8
Attachment #234246 - Attachment is obsolete: true
(Assignee)

Comment 21

12 years ago
Created attachment 236062 [details] [diff] [review]
use nsIPersistentProperties instead of nsIArray

I used pypoke to test this new feature. I can see the tag name and heading level are exposed.
Attachment #235378 - Attachment is obsolete: true
Attachment #236062 - Flags: review+
(Assignee)

Updated

12 years ago
Attachment #236062 - Flags: review+ → review?(aaronleventhal)
(Assignee)

Comment 22

12 years ago
Created attachment 236065 [details]
The screen capture of the object attributes exposed on pypoke
(Reporter)

Comment 23

12 years ago
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
Attachment #236062 - Flags: review?(aaronleventhal) → review-
(Assignee)

Comment 24

12 years ago
Created attachment 236173 [details] [diff] [review]
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.
Attachment #236062 - Attachment is obsolete: true
Attachment #236173 - Flags: review?(aaronleventhal)
(Reporter)

Updated

12 years ago
Attachment #236173 - Flags: superreview?(neil)
Attachment #236173 - Flags: review?(aaronleventhal)
Attachment #236173 - Flags: review+

Comment 25

12 years ago
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.
Attachment #236173 - Flags: superreview?(neil) → superreview+
(Assignee)

Comment 26

12 years ago
Created attachment 236284 [details] [diff] [review]
patch addressing Neil's comments
Attachment #236173 - Attachment is obsolete: true
(Assignee)

Comment 27

12 years ago
Need I request review again?

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

(Reporter)

Comment 28

12 years ago
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.
(Reporter)

Updated

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