Closed Bug 357804 Opened 18 years ago Closed 18 years ago

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

Categories

(Core :: Disability Access APIs, enhancement)

PowerPC
macOS
enhancement
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: hwaara, Assigned: hwaara)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
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.
Attachment #243331 - Flags: review?(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);
Attachment #243331 - Flags: review?(ginn.chen) → review-
Attached patch Patch v2 (obsolete) — Splinter Review
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.
Attachment #243331 - Attachment is obsolete: true
Attachment #243477 - Flags: review?(ginn.chen)
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 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.
Attachment #243477 - Flags: review?(ginn.chen) → review-
(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.

Attached patch Patch v3 (-u8p) (obsolete) — Splinter Review
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.
Attachment #243591 - Flags: review?(ginn.chen)
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. :-(
Attachment #243591 - Flags: review?(ginn.chen)
Attached patch Patch v4Splinter Review
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
Attachment #243477 - Attachment is obsolete: true
Attachment #243591 - Attachment is obsolete: true
Attachment #243783 - Flags: review?(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.
(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.
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 on attachment 243783 [details] [diff] [review]
Patch v4

review + without changing src/mac/nsRootAccessibleWrap.*

hwaara, I was waiting you to post a new patch
Attachment #243783 - Flags: review?(ginn.chen) → review+
Finally checked in to trunk, thanks for the reviews Ginn!
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.