Closed Bug 1109891 Opened 5 years ago Closed 4 years ago

teach mozAccessible to store references to proxies

Categories

(Core :: Disability Access APIs, defect)

x86_64
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: tbsaunde, Assigned: lsocks)

References

Details

Attachments

(3 files, 4 obsolete files)

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.
Attachment #8608273 - Flags: review?(tbsaunde+mozbugs)
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.
Attachment #8608273 - Flags: review?(tbsaunde+mozbugs) → review+
Attached patch mgecko2 (obsolete) — Splinter Review
Attachment #8608273 - Attachment is obsolete: true
Attachment #8608707 - Flags: review?(tbsaunde+mozbugs)
Attached patch mgecko2Splinter Review
left extra whitespace by accident
Attachment #8608707 - Attachment is obsolete: true
Attachment #8608707 - Flags: review?(tbsaunde+mozbugs)
Attachment #8608712 - Flags: review?(tbsaunde+mozbugs)
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.
Attachment #8608712 - Flags: review?(tbsaunde+mozbugs) → review+
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);
Attachment #8608771 - Flags: review+
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.
Attachment #8608901 - Flags: feedback?(tbsaunde+mozbugs) → review+
Attachment #8608771 - Attachment is obsolete: true
Attachment #8608995 - Flags: review?(tbsaunde+mozbugs)
Attachment #8608901 - Attachment is obsolete: true
Attachment #8609007 - Flags: review?(tbsaunde+mozbugs)
Attachment #8609007 - Flags: review?(tbsaunde+mozbugs) → review+
Attachment #8608995 - Flags: review?(tbsaunde+mozbugs) → review+
https://hg.mozilla.org/mozilla-central/rev/52644686f062
https://hg.mozilla.org/mozilla-central/rev/cda6c79898f6
https://hg.mozilla.org/mozilla-central/rev/0552f33f61e8
Assignee: nobody → lorien
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.