Closed Bug 1172946 Opened 5 years ago Closed 5 years ago

Handle proxies in mozAccessible methods parent and children

Categories

(Core :: Disability Access APIs, defect)

x86_64
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: lsocks, Assigned: lsocks)

References

Details

Attachments

(3 files, 9 obsolete files)

1.67 KB, patch
tbsaunde
: review+
Details | Diff | Splinter Review
5.06 KB, patch
tbsaunde
: review+
Details | Diff | Splinter Review
4.71 KB, patch
lsocks
: review+
Details | Diff | Splinter Review
No description provided.
Blocks: 1171995
Attachment #8617453 - Flags: review?(tbsaunde+mozbugs)
Attachment #8617455 - Flags: review?(tbsaunde+mozbugs)
Attachment #8617453 - Attachment is obsolete: true
Attachment #8617453 - Flags: review?(tbsaunde+mozbugs)
Attachment #8621034 - Flags: review?(tbsaunde+mozbugs)
Attachment #8617455 - Attachment is obsolete: true
Attachment #8617455 - Flags: review?(tbsaunde+mozbugs)
Attachment #8621035 - Flags: review?(tbsaunde+mozbugs)
Attachment #8621035 - Attachment is obsolete: true
Attachment #8621035 - Flags: review?(tbsaunde+mozbugs)
Attachment #8621037 - Flags: review?(tbsaunde+mozbugs)
Comment on attachment 8621034 [details] [diff] [review]
Changing mozAccessible parent to respect proxies

># HG changeset patch
># User Lorien Hu <lorien@lorienhu.com>
># Date 1434037790 14400
>#      Thu Jun 11 11:49:50 2015 -0400
># Node ID 2e03d91df1ae811453c83b11a1194f84a1b6a8c7
># Parent  9ebd530c5843150c20631a86bc8624da943098c0
>Bug 1172946 - Add handling for proxies in mozAccessible parent
>
>diff --git a/accessible/mac/mozAccessible.h b/accessible/mac/mozAccessible.h
>--- a/accessible/mac/mozAccessible.h
>+++ b/accessible/mac/mozAccessible.h
>@@ -33,16 +33,22 @@ GetNativeFromGeckoAccessible(mozilla::a1
> }
> 
> inline mozAccessible*
> GetNativeFromProxy(mozilla::a11y::ProxyAccessible* aProxy)
> {
>   return reinterpret_cast<mozAccessible*>(aProxy->GetWrapper());
> }
> 
>+mozilla::a11y::ProxyAccessible*
>+GetProxyUnignoredParent(mozilla::a11y::ProxyAccessible* aProxy);
>+
>+BOOL
>+IsProxyIgnored(mozilla::a11y::ProxyAccessible* aProxy);

try to keep declarations on one line.

it might make sense to namespace these to keep down the mozilla::a11y::

>+GetProxyUnignoredParent(ProxyAccessible* aProxy) {

{ on next line

>+  ProxyAccessible* parent = static_cast<ProxyAccessible*>(aProxy->Parent());

no static cast needed

>+  while (parent && IsProxyIgnored(aProxy))
>+    parent = static_cast<ProxyAccessible*>(parent->Parent());

same

>+BOOL
>+IsProxyIgnored(ProxyAccessible* aProxy) {

{ on next line

>+  NS_OBJC_BEGIN_TRY_ABORT_BLOCK_RETURN;

probably not needed since this is never called by code we don't control

>+
>+  mozAccessible* nativeObject = GetNativeFromProxy(aProxy);
>+  return (!nativeObject) || [nativeObject accessibilityIsIgnored];

seems like

if (!nativeObject)
  return true;

return [nativeObject accessibilityIsIgnored];
is more readable maybe

or at least drop the parens around !nativeObject

>+  ProxyAccessible* proxy = [self getProxyAccessible];
>+  if (!accWrap && !proxy)
>+    return nil;

this double checking proxy and accWrap seems kinda weird.

>+    if (accWrap)
>+      nativeParent = GetNativeFromGeckoAccessible(accWrap->RootAccessible());
>+    else {

if you are going to brace one side do both please

>+      Accessible* outerdoc = proxy->OuterDocOfRemoteBrowser();

outerDoc would be more typical naming

>+      nativeParent = GetNativeFromGeckoAccessible(outerdoc->RootAccessible());

I'm kinda worried outerdoc might be null here given bug 1171728 :/
Attachment #8621034 - Flags: review?(tbsaunde+mozbugs) → review+
Comment on attachment 8621037 [details] [diff] [review]
Changing mozAccessible children to respect proxies

># HG changeset patch
># User Lorien Hu <lorien@lorienhu.com>
># Date 1434038636 14400
>#      Thu Jun 11 12:03:56 2015 -0400
># Node ID 24b898089040f00dec497d7c0763340028a354a1
># Parent  2e03d91df1ae811453c83b11a1194f84a1b6a8c7
>Bug 1172946 - Add handling for proxies in mozAccessible children
>
>diff --git a/accessible/ipc/ProxyAccessible.cpp b/accessible/ipc/ProxyAccessible.cpp
>--- a/accessible/ipc/ProxyAccessible.cpp
>+++ b/accessible/ipc/ProxyAccessible.cpp
>@@ -50,16 +50,32 @@ ProxyAccessible::SetChildDoc(DocAccessib
>     mOuterDoc = true;
>   } else {
>     MOZ_ASSERT(mChildren.Length() == 1);
>     mChildren.Clear();
>     mOuterDoc = false;
>   }
> }
> 
>+ProxyAccessible*
>+ProxyAccessible::GetChildAt(uint32_t aIndex) const

We already have a ChildAt, having both seems confusing

>+{
>+  ProxyAccessible* child = mChildren.SafeElementAt(aIndex, nullptr);

mChildren[i] should be fine

>+  if (!child)
>+    return nullptr;
>+
>+#ifdef DEBUG
>+  ProxyAccessible* realParent = child->mParent;
>+  NS_ASSERTION(!realParent || realParent == this,

you could do Parent() || Parent() == this and not need ifdef

>+               "Two accessibles have the same first child accessible!");

am I crazy or is that message unrelated?

>+  ProxyAccessible* GetChildAt(uint32_t aIndex) const;
>+
>+  uint32_t ChildCount() const { return mChildren.Length(); }

we already have ChildrenCount()

>+GetProxyUnignoredChildren(ProxyAccessible* aProxy,
>+                          nsTArray<ProxyAccessible*>* aChildrenArray)

I believe we might be able to get away with const ProxyAccessible* for one or both here.

> - (NSArray*)children
> {
>   NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NIL;
> 
>   AccessibleWrap* accWrap = [self getGeckoAccessible];
>-  if (mChildren || !accWrap->AreChildrenCached())
>+  ProxyAccessible* proxy = [self getProxyAccessible];

you might consider limiting the scope of this var to the else if where its used

>+    // now iterate through the children array, and get each native accessible.

I guess that's preexisting, but it seems obvious

>+    uint32_t totalCount = childrenArray.Length();

imho childCount or childrenCount is a better name

>+    for (uint32_t idx = 0; idx < totalCount; idx++) {
>+      ProxyAccessible* curAccessible = childrenArray.ElementAt(idx);

use []

>+      if (curAccessible) {

that can never be false right?
Attachment #8621037 - Flags: review?(tbsaunde+mozbugs) → review+
Attachment #8621600 - Flags: review?(tbsaunde+mozbugs)
Attachment #8621034 - Attachment is obsolete: true
Attachment #8621602 - Flags: review?(tbsaunde+mozbugs)
Attachment #8621037 - Attachment is obsolete: true
Attachment #8621604 - Flags: review?(tbsaunde+mozbugs)
Attachment #8621600 - Attachment is obsolete: true
Attachment #8621600 - Flags: review?(tbsaunde+mozbugs)
Attachment #8626348 - Flags: review?(tbsaunde+mozbugs)
Comment on attachment 8626348 [details] [diff] [review]
part 1 - use mozilla a11y namespace in mozAccessible.h

># HG changeset patch
># User Lorien Hu <lorien@lorienhu.com>
># Date 1435271572 25200
>#      Thu Jun 25 15:32:52 2015 -0700
># Node ID 527db8373f3cb1ab2f65089a959772b4851829cb
># Parent  0bcd7c0d292652a05a5ea1fcb3f9040da06ddaf3
>Bug 1172946 - (part 1) Adding mozilla a11y namespace to mozAccessible.h

s/adding/add/
Attachment #8626348 - Flags: review?(tbsaunde+mozbugs) → review+
Comment on attachment 8621602 [details] [diff] [review]
part 2 - handle proxies in mozAccessible parent

waiting  for response to last question for review.

>+GetProxyUnignoredParent(ProxyAccessible* aProxy)

const ProxyAccessible* ?

>+BOOL
>+IsProxyIgnored(ProxyAccessible* aProxy)

const?

>+{
>+  mozAccessible* nativeObject = GetNativeFromProxy(aProxy);
>+  if (!nativeObject)
>+   return true;

can it be null?

>-  Accessible* accessibleParent = accWrap->GetUnignoredParent();
>-  if (accessibleParent) {
>-    id nativeParent = GetNativeFromGeckoAccessible(accessibleParent);
>-    if (nativeParent)
>+  ProxyAccessible* proxy = [self getProxyAccessible];
>+
>+  id nativeParent = nil;
>+
>+  if (accWrap) {

nit no blank line?

I think this might be more readable if you delt with proxies in one chunk and then did AccessibleWrap stuff instead of this mixture, but maybe that's just the diff.

>+    Accessible* accessibleParent = accWrap->GetUnignoredParent();
>+    if (accessibleParent)
>+      nativeParent = GetNativeFromGeckoAccessible(accessibleParent);
>+  } else if (proxy) {
>+    // Go up the chain to find a parent that is not ignored.
>+    ProxyAccessible* accessibleParent = GetProxyUnignoredParent(proxy);
>+    if (accessibleParent)
>+      nativeParent = GetNativeFromProxy(accessibleParent);
>+  } else {
>+    return nil;
>+  }
>+
>+  if (nativeParent)
>       return GetClosestInterestingAccessible(nativeParent);

This is a preexisting issue so you don't need to deal with it, but this code is pretty silly, GetUnignoredParent makes sure to return an AccessibleWrap* where the mozAccessible returns false for isIgnored calls.  So GetClosestInterestingAccessible will always return its argument.

>-  }
> 
>   // GetUnignoredParent() returns null when there is no unignored accessible all the way up to
>   // the root accessible. so we'll have to return whatever native accessible is above our root accessible
>   // (which might be the owning NSWindow in the application, for example).
>   //
>   // get the native root accessible, and tell it to return its first parent unignored accessible.
>-  id nativeParent =
>-    GetNativeFromGeckoAccessible(accWrap->RootAccessible());
>+    if (accWrap) {
>+      nativeParent = GetNativeFromGeckoAccessible(accWrap->RootAccessible());
>+    } else {
>+      Accessible* outerDoc = proxy->OuterDocOfRemoteBrowser();
>+      nativeParent = GetNativeFromGeckoAccessible(outerDoc->RootAccessible());
>+    }

It kind of feels like this should be unneccessary because of what mozRootAccessible does, but maybe not I can't quiet convince myself.

That said there is onecase you need to handle here.  Consider the proxy for the top level document in a child process.  In that case you should call OuterDocForRemoteBrowser() and then just get the closest interesting accessible for it I think.
Attachment #8621602 - Flags: review?(tbsaunde+mozbugs) → review+
Comment on attachment 8621604 [details] [diff] [review]
part 3 - handle proxies in mozAccessible children

>+GetProxyUnignoredChildren(const ProxyAccessible* aProxy,
>+                          nsTArray<ProxyAccessible*>* aChildrenArray)
>+{
>+  if (aProxy->MustPruneChildren())
>+    return;
>+
>+  uint32_t childCount = aProxy->ChildrenCount();
>+  for (uint32_t childIdx = 0; childIdx < childCount; childIdx++) {

kinda feel like those should be size_t *shrug*
Attachment #8621604 - Flags: review?(tbsaunde+mozbugs) → review+
Updated to respect the addition of ConvertToNSArray
Attachment #8621604 - Attachment is obsolete: true
Attachment #8633576 - Flags: review?(tbsaunde+mozbugs)
Attachment #8633576 - Flags: review?(tbsaunde+mozbugs) → review+
carry r=tbsaunde
Attachment #8621602 - Attachment is obsolete: true
Attachment #8633598 - Flags: review+
carry r=tbsaunde

Moved proxies and accessibles to be handled in separate blocks. I left the pre-existing issues in and added a check for outerDoc being nil.
Attachment #8633598 - Attachment is obsolete: true
Attachment #8633601 - Flags: review+
You need to log in before you can comment on or make changes to this bug.