Closed Bug 1171995 Opened 5 years ago Closed 4 years ago

Handle proxies in mozAccessible methods accessibility{IsIgnored,AttributeNames,AttributeValue,HitTest,FocusedUIElement}

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

(7 files, 5 obsolete files)

5.08 KB, patch
lsocks
: review+
Details | Diff | Splinter Review
1.29 KB, patch
lsocks
: review+
Details | Diff | Splinter Review
3.50 KB, patch
lsocks
: review+
Details | Diff | Splinter Review
1006 bytes, patch
lsocks
: review+
Details | Diff | Splinter Review
1.93 KB, patch
lsocks
: review+
Details | Diff | Splinter Review
13.68 KB, patch
lsocks
: review+
Details | Diff | Splinter Review
1.53 KB, patch
tbsaunde
: review+
Details | Diff | Splinter Review
mozAccessible methods accessibilityIsIgnored, accessibilityAttributeNames, accessibilityAttributeValue, accessibilityHitTest should respect proxies
Depends on: 1172946
I'm definitely doing something wrong in this patch. In ProxyAccessible.cpp, I'm getting a rootID, but I can't get anything from mDoc->GetAccessible(rootID).
Comment on attachment 8617479 [details] [diff] [review]
proxy handling for accessibility{IsIgnored,AttributeNames,AttributeValue,HitTest}, adding FocusedChild to proxyaccessible

># HG changeset patch
># User Lorien Hu <lorien@lorienhu.com>
># Date 1433872681 14400
>#      Tue Jun 09 13:58:01 2015 -0400
># Node ID 1cbb7465fe0410fbc19ce415d954cf5393490a0c
># Parent  7c348a2e3eba51e79ec481a51d8d12e256610ca9
>Bug 1171995 - Handle proxies in mozAccessible methods accessibility{IsIgnored,AttributeNames,AttributeValue,HitTest}

in the future please break stuff up.

> - (BOOL)accessibilityIsIgnored
> {
>   NS_OBJC_BEGIN_TRY_ABORT_BLOCK_RETURN;
> 
>   // unknown (either unimplemented, or irrelevant) elements are marked as ignored
>   // as well as expired elements.
> 
>-  AccessibleWrap* accWrap = [self getGeckoAccessible];
>-  return !accWrap || ([[self role] isEqualToString:NSAccessibilityUnknownRole] &&
>-                               !(accWrap->InteractiveState() & states::FOCUSABLE));
>+  bool noRole = [[self role] isEqualToString:NSAccessibilityUnknownRole];
>+  if (AccessibleWrap* accWrap = [self getGeckoAccessible])
>+    return (noRole && !(accWrap->InteractiveState() & states::FOCUSABLE));
>+
>+  else if (ProxyAccessible* proxy = [self getProxyAccessible])

the blank line is kinda weird

also no else after return

>+    return (noRole && !(proxy->State() & states::FOCUSABLE));
>+    // TODO: implement InteractiveStates for proxies in the future

so is the todo after what its commenting on, though I'm not sure its totally useful.

>   if ([attribute isEqualToString:NSAccessibilityTitleUIElementAttribute]) {
>-    Relation rel =
>-      [self getGeckoAccessible]->RelationByType(RelationType::LABELLED_BY);
>-    Accessible* tempAcc = rel.Next();
>-    return tempAcc ? GetNativeFromGeckoAccessible(tempAcc) : nil;
>+    if (accWrap) {
>+      Relation rel = accWrap->RelationByType(RelationType::LABELLED_BY);
>+      Accessible* tempAcc = rel.Next();
>+      return tempAcc ? GetNativeFromGeckoAccessible(tempAcc) : nil;
>+    }
>+    else {

please cuddle elses

>+      nsTArray<ProxyAccessible*> rel = proxy->RelationByType(RelationType::LABELLED_BY);
>+      ProxyAccessible* tempProxy = reinterpret_cast<ProxyAccessible*>(rel.SafeElementAt(0, nil));

the reinterpret_cast shouldn't be needed.

I don't think you need to pass nullptr to SafeElementAt

>+    Accessible* child = accWrap->ChildAtPoint(geckoPoint.x, geckoPoint.y,
>+                                  Accessible::eDeepestChild);
>+    if (child)
>+      nativeChild = GetNativeFromGeckoAccessible(child);
>+  }
>+  else if (proxy) {

cuddle else if too

> - (id)accessibilityFocusedUIElement
> {
>+
>   AccessibleWrap* accWrap = [self getGeckoAccessible];

why the blank line?

>+    Accessible* focusedGeckoChild = accWrap->FocusedChild();
>+    if (focusedGeckoChild)
>+      focusedChild = GetNativeFromGeckoAccessible(focusedGeckoChild);
>   }
>+  else if (proxy) {

cuddle else
Attachment #8617479 - Flags: review?(tbsaunde+mozbugs) → review+
Summary: Handle proxies in mozAccessible methods accessibility{IsIgnored,AttributeNames,AttributeValue,HitTest} → Handle proxies in mozAccessible methods accessibility{IsIgnored,AttributeNames,AttributeValue,HitTest,FocusedUIElement}
carry r=tbsaunde

Breaking up patch into multiple patches and changing to work with new table code.
Attachment #8621038 - Attachment is obsolete: true
Attachment #8633897 - Flags: review+
Attachment #8633898 - Flags: review?(tbsaunde+mozbugs)
carry r=tbsaunde
Attachment #8633899 - Flags: review+
Attachment #8633900 - Flags: review?(tbsaunde+mozbugs)
Comment on attachment 8633898 [details] [diff] [review]
Add IsTable{,Row,Cell} methods to ProxyAccessible

># HG changeset patch
># User Lorien Hu <lorien@lorienhu.com>
># Parent  fd621a207665b71d601ef2158fbfddf91224fdc9
>Bug 1171995 - Part 2: Add IsTable, IsTableRow, IsTableCell to proxied accessibles
>
>diff --git a/accessible/ipc/ProxyAccessible.cpp b/accessible/ipc/ProxyAccessible.cpp
>--- a/accessible/ipc/ProxyAccessible.cpp
>+++ b/accessible/ipc/ProxyAccessible.cpp
>@@ -407,16 +407,34 @@ uint32_t
> ProxyAccessible::EndOffset(bool* aOk)
> {
>   uint32_t retVal = 0;
>   unused << mDoc->SendEndOffset(mID, &retVal, aOk);
>   return retVal;
> }
> 
> bool
>+ProxyAccessible::IsTable()

make it const and inline it

Also add comment these aren't quiet correct because of wierd aria roles on html tables and such madness.
Attachment #8633898 - Flags: review?(tbsaunde+mozbugs) → review+
Comment on attachment 8633900 [details] [diff] [review]
(p4) - accessibilityAttributeNames changes

>-  if (accWrap->IsTable())
>-    objectAttributes = tableAttrs;
>-  else if (accWrap->IsTableRow())
>+  if ((accWrap && accWrap->IsTable()) || (proxy && proxy->IsTable()))
>+      objectAttributes = tableAttrs;

isn't that over indented?
Attachment #8633900 - Flags: review?(tbsaunde+mozbugs) → review+
Attachment #8634147 - Flags: review?(tbsaunde+mozbugs)
carry r=tbsaunde
Attachment #8634150 - Flags: review+
Comment on attachment 8634147 [details] [diff] [review]
p5-accessibilityAttributeValue changes

general nit no else after return please.

Also please file a bug to use AccIter with filter::GetRow to iterate over rows the current code is broken for tables using row groups, and I guess we should do something similar for proxies.
Attachment #8634147 - Flags: review?(tbsaunde+mozbugs) → review+
carry r=tbsaunde
Attachment #8633898 - Attachment is obsolete: true
Attachment #8634320 - Flags: review+
carry r=tbsaunde
Attachment #8633900 - Attachment is obsolete: true
Attachment #8634322 - Flags: review+
carry r=tbsaunde
Attachment #8634147 - Attachment is obsolete: true
Attachment #8634323 - Flags: review+
Attachment #8634351 - Flags: review?(tbsaunde+mozbugs) → review+
You need to log in before you can comment on or make changes to this bug.