357804 - [Mac a11y] Make checkboxes, buttons and textfields "flat"

Status: RESOLVED FIXED

(Core :: Disability Access APIs, enhancement)

Description

[Håkan Waara]

One major problem for mac has been that the accessibility hierarchy does not look like it expects. For example, a button can't have a static text child. That makes VoiceOver choke.

So in order to make textboxes, checkboxes, etc, look like it expects, we need to "flatten" them. 

That is, the mac toolkit won't have to see these children, as long as the parent (e.g., a AXCheckbox) contains all the relevant info.

Comment 1

[Håkan Waara]

Attachment: Implement the concept of flat accessibles

This makes some accessibles flat for the mac toolkit. For people scripting nsIAccessible, the accessibility hierarchy will look the same as on other platforms, as requested by Aaron at the hackfest.

Here's how it works:

-- nsAccessibleWrap.h --

* IsFlat() defines by the role constant, whether a native mac accessible object will be flat.

* IsIgnored() is shorthand for checking if an nsAccessibleWrap doesn't have a native accessible (i.e., is "ignored"). If it does not, it means it's a child to a flat accessible.

* Because we sometimes create accessibles without their parents being created yet, I needed AncestorIsFlat() to check at runtime whether an accessible has to create a native (or if it is gonna be ignored anyway, because it is a child of a flat accessible).

-- nsAccessibleWrap.mm --

* GetNativeType() will create the right kind of native accessible. This is part of other bugs (bug 357805, bug 354447)

* GetUnignoredChildCount(), GetUnignoredParent(), GetUnignoredChildren(): These are the methods the mac accessibles use to get their children/parent/childCount etc.   I can't use nsAccessible versions, because I need to respect flatness.

It was a nice exercise for learning to use nsTArray. :-)

Let me know if you want to go through this together on IRC, or via mail or something.

Comment 2

[Ginn Chen]

Comment on attachment 243331 [details] [diff] [review]
Implement the concept of flat accessibles

Looks good to me.

Please simplify GetUnignoredChildCount as the discussion in irc.

Other comments:
-  if (mNativeWrapper)
+  if (mNativeWrapper) {
     *aOutInterface = (void**)mNativeWrapper->getNativeObject();
   return NS_OK;
 }
+  return NS_ERROR_FAILURE;
+}
Looks like it doesn't indent correctly.

+nsresult
+nsAccessibleWrap::GetUnignoredChildCount(PRInt32 *aOutCount, PRBool aDeepCount)
Since it always return NS_OK,
why not return count directly?
same to GetUnignoredChildren and GetUnignoredParent

+  PRInt32 childCount=0;
please add space around "="

+      PRInt32 unignoredChildCount=0;
please add space around "="

+    if (aDeepCount && !childWrap->IsFlat()) {
+      PRInt32 childrensCount=0;
+      childWrap->GetUnignoredChildCount(&childrensCount, aDeepCount);
+      childCount += childrensCount;
+    }
please add space around "="
Personally, I prefer
childWrap->GetUnignoredChildCount(&childrensCount, PR_TRUE);

Comment 3

[Håkan Waara]

Attachment: Patch v2

Patch with comments addressed. Both methods simplified; I actually found a few bugs in both GetUnignoredChildCount() and GetUnignoredChildren() while reviewing the algorithms.  (I recommend inter-diff, if you're interested.)

If indenting looks messed up, it's only because the diff is -w.

Comment 4

[Håkan Waara]

Comment on attachment 243477 [details] [diff] [review]
Patch v2

+  nsCOMPtr<nsIAccessible> parent(GetParent())

This change shouldn't be there, because the loop only sets the parent if it's not ignored. I will change it back to how it was in the last patch.

Comment 5

[Ginn Chen]

Comment on attachment 243477 [details] [diff] [review]
Patch v2

Please change
+nsresult
+nsAccessibleWrap::GetUnignoredChildren(nsTArray<nsRefPtr<nsAccessibleWrap> > &aChildrenArray)
to
+nsTArray<nsRefPtr<nsAccessibleWrap> > &
+nsAccessibleWrap::GetUnignoredChildren()
or
+void
+nsAccessibleWrap::GetUnignoredChildren(nsTArray<nsRefPtr<nsAccessibleWrap> > &aChildrenArray)

Change
+nsresult
+nsAccessibleWrap::GetUnignoredParent(nsIAccessible **aParent)
to
+already_AddRefed<nsIAccessible>
+nsAccessibleWrap::GetUnignoredParent()

+  nsCOMPtr<nsIAccessible> parent(GetParent())
+  do {
+    parent = GetParent();
+  } while (NS_STATIC_CAST(nsAccessibleWrap*, (nsIAccessible*)parent.get())->IsIgnored());
should be
+  nsCOMPtr<nsIAccessible> parent = GetParent();
+  while (parent && NS_STATIC_CAST(nsAccessibleWrap*, (nsIAccessible*)parent.get())->IsIgnored())
+    parent = parent->GetParent();
+  }
right?

P.S.
If you attach your patch in diff -uw format for review, please also attach the diff -u version.

Comment 6

[Håkan Waara]

(In reply to comment #5)
> +  nsCOMPtr<nsIAccessible> parent(GetParent())
> +  do {
> +    parent = GetParent();
> +  } while (NS_STATIC_CAST(nsAccessibleWrap*,
> (nsIAccessible*)parent.get())->IsIgnored());
> should be
> +  nsCOMPtr<nsIAccessible> parent = GetParent();
> +  while (parent && NS_STATIC_CAST(nsAccessibleWrap*,
> (nsIAccessible*)parent.get())->IsIgnored())
> +    parent = parent->GetParent();
> +  }
> right?

No, the loop won't work of |parent| is set there, see comment 4.

Comment 7

[Håkan Waara]

Attachment: Patch v3 (-u8p)

I can't use a return value of type nsTArray<nsRefPtr<nsAccessibleWrap> >&, because I can't return a reference to a stack-based variable (out of scope), therefore I need the array as an in-reference, so I have something to operate on.

Did the other changes.

Comment 8

[Håkan Waara]

Comment on attachment 243591 [details] [diff] [review]
Patch v3 (-u8p)

I need to work more on this, it is much more complicated than I thought. :-(

Comment 9

[Håkan Waara]

Attachment: Patch v4

Ok, I think I'm back on track! Solved a few bugs in my mozAccessible* code so it worked correctly.

And made a few essential fixes to this code as well:

* Rewrote GetUnignoredParent(), it's now recursive, and simpler to follow.
* GetUnignoredParent() will go all the way up, but at the most to the root accessible.
* Reordered things in GetUnignoredChildren() a bit to make more sense

Comment 10

[Ginn Chen]

Comment on attachment 243783 [details] [diff] [review]
Patch v4

I'm OK with this patch.
I concern nsRootAccessibleWrap::GetUnignoredParent is inconsistent with nsRootAccessibleWrap::GetParent.

GetUnignoredParent returns itselft, but GetParent returns nsnull.

Comment 11

[Håkan Waara]

(In reply to comment #10)
> (From update of attachment 243783 [details] [diff] [review] [edit])
> I'm OK with this patch.
> I concern nsRootAccessibleWrap::GetUnignoredParent is inconsistent with
> nsRootAccessibleWrap::GetParent.
> 
> GetUnignoredParent returns itselft, but GetParent returns nsnull.

Ok, I've removed all changes to nsRootAccessibleWrap and verified that I don't actually need them. So GetUnignoredParent() will return null for the root accessible, just like GetParent() does.

Comment 12

[Håkan Waara]

Ginn, FYI, the patch still has "review?", I don't know if it's intended or if you just forgot to stamp it since you gave OK on IRC. :-)

Comment 13

[Ginn Chen]

Comment on attachment 243783 [details] [diff] [review]
Patch v4

review + without changing src/mac/nsRootAccessibleWrap.*

hwaara, I was waiting you to post a new patch

Comment 14

[Håkan Waara]

Finally checked in to trunk, thanks for the reviews Ginn!