Closed Bug 342288 Opened 15 years ago Closed 15 years ago

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

Categories

(Core :: Disability Access APIs, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: aaronlev, Assigned: gaomingcn)

References

(Blocks 1 open bug)

Details

(Keywords: access)

Attachments

(2 files, 10 obsolete files)

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.
Blocks: textattra11y
Summary: Use ATK object attributes to expose tag name, heading level → New ATK: Use object attributes to expose tag name, heading level
No longer blocks: newatk
Status: NEW → ASSIGNED
Attached patch patch v1, not finished yet (obsolete) — Splinter Review
> NS_IMETHODIMP nsAccessible::GetAttributes(nsIArray **attrArray)

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

Attached patch patch v2 draft, not finished yet (obsolete) — Splinter Review
have to leave now. will finish it later.
Attachment #231796 - Attachment is obsolete: true
+  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?
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 .
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
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?
A narrow char string is okay for the names. The values need to be wide strings.
Attached patch patch v3 (obsolete) — Splinter Review
I am not sure about the array length. And it was not tested yet.
Attachment #232693 - Attachment is obsolete: true
I will use nsIMutableArray for that.
Attachment #233808 - Attachment is obsolete: true
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.
Attached patch patch v5 (obsolete) — Splinter Review
Attachment #234019 - Attachment is obsolete: true
Attachment #234100 - Flags: review?(aaronleventhal)
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-
Attached patch patch v6 (obsolete) — Splinter Review
Attachment #234100 - Attachment is obsolete: true
Still no switch statement in nsHyperTextAccessible?
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.
Attached patch patch v7 (obsolete) — Splinter Review
Attachment #234221 - Attachment is obsolete: true
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.
Attached patch patch v8 (obsolete) — Splinter Review
Attachment #234246 - Attachment is obsolete: true
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+
Attachment #236062 - Flags: review+ → review?(aaronleventhal)
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-
*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)
Attachment #236173 - Flags: superreview?(neil)
Attachment #236173 - Flags: review?(aaronleventhal)
Attachment #236173 - Flags: review+
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+
Attachment #236173 - Attachment is obsolete: true
Need I request review again?

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

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.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.