Lorien Hu (:lsocks) Assignee
	Description • 9 years ago

mozAccessible methods accessibilityIsIgnored, accessibilityAttributeNames, accessibilityAttributeValue, accessibilityHitTest should respect proxies

	Lorien Hu (:lsocks) Assignee
	Comment 2 • 9 years ago

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).

	Trevor Saunders (:tbsaunde)
	Comment 3 • 9 years ago

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

	Lorien Hu (:lsocks) Assignee
	Comment 4 • 9 years ago

Carry r=tbsaunde

	Lorien Hu (:lsocks) Assignee
	Comment 5 • 9 years ago

carry r=tbsaunde

Breaking up patch into multiple patches and changing to work with new table code.

	Lorien Hu (:lsocks) Assignee
	Comment 7 • 9 years ago

carry r=tbsaunde

	Trevor Saunders (:tbsaunde)
	Comment 9 • 9 years ago

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.

	Trevor Saunders (:tbsaunde)
	Comment 10 • 9 years ago

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?

	Lorien Hu (:lsocks) Assignee
	Comment 12 • 9 years ago

carry r=tbsaunde

	Trevor Saunders (:tbsaunde)
	Comment 13 • 9 years ago

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.

	Lorien Hu (:lsocks) Assignee
	Comment 14 • 9 years ago

carry r=tbsaunde

	Lorien Hu (:lsocks) Assignee
	Comment 15 • 9 years ago

carry r=tbsaunde

	Lorien Hu (:lsocks) Assignee
	Comment 16 • 9 years ago

carry r=tbsaunde

	Pulsebot
	Comment 18 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/6bacbe6ab496
https://hg.mozilla.org/integration/mozilla-inbound/rev/728d627656ca
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c981fda1d41
https://hg.mozilla.org/integration/mozilla-inbound/rev/79131088413a
https://hg.mozilla.org/integration/mozilla-inbound/rev/73f2e7591ff0
https://hg.mozilla.org/integration/mozilla-inbound/rev/e27f10f740a2
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a04fa687d38

	Ryan VanderMeulen [:RyanVM]
	Comment 19 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/6bacbe6ab496
https://hg.mozilla.org/mozilla-central/rev/728d627656ca
https://hg.mozilla.org/mozilla-central/rev/5c981fda1d41
https://hg.mozilla.org/mozilla-central/rev/79131088413a
https://hg.mozilla.org/mozilla-central/rev/73f2e7591ff0
https://hg.mozilla.org/mozilla-central/rev/e27f10f740a2
https://hg.mozilla.org/mozilla-central/rev/5a04fa687d38