1109891 - teach mozAccessible to store references to proxies

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Trevor Saunders (:tbsaunde)]

It should work to do this in the same way as linux.  That is
- change mozAccessible::mGeckoAccessible to be uint64_t and use the least significant bit to signal proxy vs Accessiblewrap.
- All the places that use mGeckoAccessible need to check if they have a proxy or a AccessibleWrap To start it probably makes sense for most to just bail on proxies and use functions to get the proxy / Accessible out of a mozAccessible
- implement a11y::Proxy{Created,Destroyed}() to update the mozAccessibles appropriately.

Comment 1

[Lorien Hu (:lsocks)]

Attachment: mGeckoAccessible use moved to getGeckoAccessible function

Comment 2

[Trevor Saunders (:tbsaunde)]

Comment on attachment 8608273 [details] [diff] [review]
mGeckoAccessible use moved to getGeckoAccessible function

>+- (mozilla::a11y::AccessibleWrap*) getGeckoAccessible;

a comment would be nice just

/*
 * Return the Accessible for this mozAccessible.
 */

would be fine.

>-  return !mGeckoAccessible || ([[self role] isEqualToString:NSAccessibilityUnknownRole] &&
>-                               !(mGeckoAccessible->InteractiveState() & states::FOCUSABLE));
>+  return ![self getGeckoAccessible] || ([[self role] isEqualToString:NSAccessibilityUnknownRole] &&
>+                               !([self getGeckoAccessible]->InteractiveState() & states::FOCUSABLE));

it might make sense to use variables to avoid multiple calls, but don't bother if you don't want to figure out where you can.

>   if ([attribute isEqualToString:NSAccessibilityTitleUIElementAttribute]) {
>-    Relation rel = mGeckoAccessible->RelationByType(RelationType::LABELLED_BY);
>+    Relation rel = [self getGeckoAccessible]->RelationByType(RelationType::LABELLED_BY);

please keep lines under 80 chars.

>-  Accessible* child = mGeckoAccessible->ChildAtPoint(geckoPoint.x, geckoPoint.y,
>+  Accessible* child = [self getGeckoAccessible]->ChildAtPoint(geckoPoint.x, geckoPoint.y,
>                                                      Accessible::eDeepestChild);

you need to adjust this line so it still lines up with the args on the line above.

btw patches are easier to read if you make diffs with the -p flag.

Comment 3

[Lorien Hu (:lsocks)]

Attachment: mgecko2

Comment 4

[Lorien Hu (:lsocks)]

Attachment: mgecko2

left extra whitespace by accident

Comment 5

[Lorien Hu (:lsocks)]

Attachment: changing mGeckoAccessible to be uintptr

Comment 6

[Trevor Saunders (:tbsaunde)]

Comment on attachment 8608712 [details] [diff] [review]
mgecko2

>+  AccessibleWrap* accWrap = [self getGeckoAccessible];
>+
>+  if (!accWrap)
>     return nil;

In the future don't add blank lines in places like that.

SO
Accessible* foo = bar;
if (foo)
  baz;

is good.

> 
> - (NSWindow*)window
> {
>   NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NIL;
> 
>-  AccessibleWrap* accWrap = static_cast<AccessibleWrap*>(mGeckoAccessible);
>+  AccessibleWrap* accWrap = static_cast<AccessibleWrap*>([self getGeckoAccessible]);

this cast seems unnecessary.

We can deal with those issues later I'll commit this for you when I get a chance.

Comment 7

[Trevor Saunders (:tbsaunde)]

Comment on attachment 8608771 [details] [diff] [review]
changing mGeckoAccessible to be uintptr

>diff -r 44a5c984d4c4 accessible/mac/mozAccessible.h
>--- a/accessible/mac/mozAccessible.h	Thu May 21 09:49:47 2015 -0400
>+++ b/accessible/mac/mozAccessible.h	Thu May 21 10:42:28 2015 -0400
>@@ -26,22 +26,24 @@ GetObjectOrRepresentedView(id <mozAccess
> inline mozAccessible*
> GetNativeFromGeckoAccessible(mozilla::a11y::Accessible* aAccessible)
> {
>   mozAccessible* native = nil;
>   aAccessible->GetNativeInterface((void**)&native);
>   return native;
> }
> 
>+static const uintptr_t IS_PROXY = 1;

seems like a comment here would be useful.

> - (mozilla::a11y::AccessibleWrap*)getGeckoAccessible
> {
>-  return mGeckoAccessible;
>+

why the blank line?

>+  // mGeckoAccessible is a proxy

seems kind of obvious.  If you want to comment here something like

// Check if mGeckoAccessible points at a proxy.

seems better.


>+  if (mGeckoAccessible & IS_PROXY)
>+    return nil;
>+
>+  AccessibleWrap* accWrap = reinterpret_cast<AccessibleWrap*>(mGeckoAccessible);
>+
>+  if (!accWrap)
>+    return nil;
>+
>+  return accWrap;

I don't see a point in a check here the only way you can enter the return NIL case is if accWrap is NULL.  So you might as well just write

return reinterpret_cast<AccessibleWrap*>(mGeckoAccessible);

Comment 8

[Lorien Hu (:lsocks)]

Attachment: Replacing mGeckoTextAccessible field with calls to getGeckoAccessible->AsHypertext()

Comment 9

[Trevor Saunders (:tbsaunde)]

Comment on attachment 8608901 [details] [diff] [review]
Replacing mGeckoTextAccessible field with calls to getGeckoAccessible->AsHypertext()

> - (NSString*)stringFromRange:(NSRange*)range
> {
>-  NS_PRECONDITION(mGeckoTextAccessible && range, "no Gecko text accessible or range");
>+  // is this supposed to go before the precondition?
>+  AccessibleWrap* accWrap = [self getGeckoAccessible];
>+  HyperTextAccessible* textAcc = accWrap? accWrap->AsHyperText() : nullptr;
>+  NS_PRECONDITION(textAcc && range, "no Gecko text accessible or range");

afaict the assertion textAcc is non-null is just wrong because if the gecko accessible has gone away and expire has been called on the mozTextAccesible then it is null, and that can happen.  the part about range being non null can go either place though it seems kind of silly since its the whole point of the method I would think.  So the conclussion is do what you like with the range part it doesn't really matter, and for the textAcc what you have is fine, and a unrelated fix would be to take it out of the pre condition and add a return if its null.

Comment 10

[Lorien Hu (:lsocks)]

Attachment: changing mGeckoAccessible to be uintptr. Also deleting some unnecessary whitespace

Comment 11

[Lorien Hu (:lsocks)]

Attachment: Replacing mGeckoTextAccessible field with calls to getGeckoAccessible->AsHypertext()

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/52644686f062
https://hg.mozilla.org/integration/mozilla-inbound/rev/cda6c79898f6
https://hg.mozilla.org/integration/mozilla-inbound/rev/0552f33f61e8

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/52644686f062
https://hg.mozilla.org/mozilla-central/rev/cda6c79898f6
https://hg.mozilla.org/mozilla-central/rev/0552f33f61e8