Closed
Bug 1172946
Opened 9 years ago
Closed 9 years ago
Handle proxies in mozAccessible methods parent and children
Categories
(Core :: Disability Access APIs, defect)
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.
Assignee | ||
Comment 1•9 years ago
|
||
Attachment #8617453 -
Flags: review?(tbsaunde+mozbugs)
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #8617455 -
Flags: review?(tbsaunde+mozbugs)
Assignee | ||
Comment 3•9 years ago
|
||
Attachment #8617453 -
Attachment is obsolete: true
Attachment #8617453 -
Flags: review?(tbsaunde+mozbugs)
Attachment #8621034 -
Flags: review?(tbsaunde+mozbugs)
Assignee | ||
Comment 4•9 years ago
|
||
Attachment #8617455 -
Attachment is obsolete: true
Attachment #8617455 -
Flags: review?(tbsaunde+mozbugs)
Attachment #8621035 -
Flags: review?(tbsaunde+mozbugs)
Assignee | ||
Comment 5•9 years ago
|
||
Attachment #8621035 -
Attachment is obsolete: true
Attachment #8621035 -
Flags: review?(tbsaunde+mozbugs)
Attachment #8621037 -
Flags: review?(tbsaunde+mozbugs)
Comment 6•9 years ago
|
||
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 7•9 years ago
|
||
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+
Assignee | ||
Comment 8•9 years ago
|
||
Attachment #8621600 -
Flags: review?(tbsaunde+mozbugs)
Assignee | ||
Comment 9•9 years ago
|
||
Attachment #8621034 -
Attachment is obsolete: true
Attachment #8621602 -
Flags: review?(tbsaunde+mozbugs)
Assignee | ||
Comment 10•9 years ago
|
||
Attachment #8621037 -
Attachment is obsolete: true
Attachment #8621604 -
Flags: review?(tbsaunde+mozbugs)
Assignee | ||
Comment 11•9 years ago
|
||
Attachment #8621600 -
Attachment is obsolete: true
Attachment #8621600 -
Flags: review?(tbsaunde+mozbugs)
Attachment #8626348 -
Flags: review?(tbsaunde+mozbugs)
Comment 12•9 years ago
|
||
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 13•9 years ago
|
||
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 14•9 years ago
|
||
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+
Assignee | ||
Comment 15•9 years ago
|
||
Updated to respect the addition of ConvertToNSArray
Attachment #8621604 -
Attachment is obsolete: true
Attachment #8633576 -
Flags: review?(tbsaunde+mozbugs)
Updated•9 years ago
|
Attachment #8633576 -
Flags: review?(tbsaunde+mozbugs) → review+
Assignee | ||
Comment 16•9 years ago
|
||
carry r=tbsaunde
Attachment #8621602 -
Attachment is obsolete: true
Attachment #8633598 -
Flags: review+
Assignee | ||
Comment 17•9 years ago
|
||
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+
Comment 18•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1807b43113be https://hg.mozilla.org/integration/mozilla-inbound/rev/5257286ca7be https://hg.mozilla.org/integration/mozilla-inbound/rev/5c9789844bd7
Comment 19•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/1807b43113be https://hg.mozilla.org/mozilla-central/rev/5257286ca7be https://hg.mozilla.org/mozilla-central/rev/5c9789844bd7
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox42:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in
before you can comment on or make changes to this bug.
Description
•