1241453 - start exposing proxied accessibles to js

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Comment 1

[Trevor Saunders (:tbsaunde)]

Attachment: add an AccessibleOrProxy class

We have several places that store either a ProxyAccessible* or an Accessible*
 in the same member using a uintptr_t and stealing the lowest bit of the
 pointer.  The goal of the AccessibleOrProxy class is to make this simpler and
 consolidate the logic involved in doing it.

Comment 2

[Trevor Saunders (:tbsaunde)]

Attachment: switch MaiAtkObject to use AccessibleOrProxy

Comment 3

[Trevor Saunders (:tbsaunde)]

Attachment: switch MaiHyperLink to use AccessibleOrProxy

Comment 4

[Trevor Saunders (:tbsaunde)]

Attachment: clean up GetAccessibleWrap()

We can simplify it some to make better use of AccessibleOrProxy.

Comment 5

[Trevor Saunders (:tbsaunde)]

Attachment: allow storing proxies in xpcAccessibleGeneric::mIntl

Comment 6

[Trevor Saunders (:tbsaunde)]

Attachment: allow constructing xpcAccessibles with proxies

Comment 7

[Trevor Saunders (:tbsaunde)]

Attachment: allow xpcAccessibleDocument::mCache to use proxies as keys

Comment 8

[Trevor Saunders (:tbsaunde)]

Attachment: fixup xpcAccessible Intl() methods to not assume mIntl is always an Accessible

Comment 9

[Trevor Saunders (:tbsaunde)]

Attachment: allow caching xpc documents for remote documents

Comment 10

[Trevor Saunders (:tbsaunde)]

Attachment: assert accessibles are only added to non remote xpcAccessibleDocuments

Comment 11

[Trevor Saunders (:tbsaunde)]

Attachment: allow caching proxies in xpcAccessibleDocuments

Comment 12

[Trevor Saunders (:tbsaunde)]

Attachment: add DocAccessibleParent::GetXPCAccessible()

Comment 13

[Trevor Saunders (:tbsaunde)]

Attachment: factor dispatching nsIAccessibleEvents out of HandleAccEvent()

Comment 14

[Trevor Saunders (:tbsaunde)]

Attachment: fire nsIAccessibleStateChangeEvents for proxied accessibles

Comment 15

[Trevor Saunders (:tbsaunde)]

Attachment: fire nsIAccessibleTextChangeEvents for proxies

Comment 16

[Trevor Saunders (:tbsaunde)]

Attachment: fire nsIAccessibleEvents for proxied accessibles

Comment 17

[Trevor Saunders (:tbsaunde)]

Attachment: fire nsIAccessibleCaretMoveEvents for proxies

Comment 18

[alexander :surkov (:asurkov)]

Comment on attachment 8710369 [details] [diff] [review]
allow constructing xpcAccessibles with proxies

Review of attachment 8710369 [details] [diff] [review]:
-----------------------------------------------------------------

why do you need this?

Comment 19

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #18)
> Comment on attachment 8710369 [details] [diff] [review]
> allow constructing xpcAccessibles with proxies
> 
> Review of attachment 8710369 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> why do you need this?

for the later patches in the series?

Comment 20

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #19)
> (In reply to alexander :surkov from comment #18)
> > Comment on attachment 8710369 [details] [diff] [review]
> > allow constructing xpcAccessibles with proxies
> > 
> > Review of attachment 8710369 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > why do you need this?
> 
> for the later patches in the series?

I was rather asking about the bug in whole. I see you may be using that for e10s layer testing, are there other consumers/reasons?

Comment 21

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #20)
> (In reply to Trevor Saunders (:tbsaunde) from comment #19)
> > (In reply to alexander :surkov from comment #18)
> > > Comment on attachment 8710369 [details] [diff] [review]
> > > allow constructing xpcAccessibles with proxies
> > > 
> > > Review of attachment 8710369 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > why do you need this?
> > 
> > for the later patches in the series?
> 
> I was rather asking about the bug in whole. I see you may be using that for
> e10s layer testing, are there other consumers/reasons?

that wasn't really clear since you commented on a random patch.

I think testing is the only thing currently needing this.

Comment 22

[David Bolter [:davidb] (NeedInfo me for attention)]

Is it okay if I get to reviews on Monday or do we need this sooner? (I might get to them this afternoon)

Comment 23

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 8710364 [details] [diff] [review]
add an AccessibleOrProxy class

Review of attachment 8710364 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/base/AccessibleOrProxy.h
@@ +18,5 @@
> +  /**
> +   * This class stores an Accessible* or a ProxyAccessible* in a safe manner
> +   * with size sizeof(void*).
> +   */
> +class AccessibleOrProxy

Should probably remove the comment indent.

Comment 24

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 8710370 [details] [diff] [review]
allow xpcAccessibleDocument::mCache to use proxies as keys

Review of attachment 8710370 [details] [diff] [review]:
-----------------------------------------------------------------

(Aside: I'm just reviewing for what you've done here, not reviewing for anything you might have missed)

Comment 25

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 8710371 [details] [diff] [review]
fixup xpcAccessible Intl() methods to not assume mIntl is always an Accessible

Review of attachment 8710371 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/xpcom/xpcAccessibleHyperText.h
@@ +45,5 @@
>  private:
> +  HyperTextAccessible* Intl()
> +  {
> +    if (Accessible* acc = mIntl.AsAccessible()) {
> +    return acc->AsHyperText();

nit: need more indent on return.

Comment 26

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 8710375 [details] [diff] [review]
assert accessibles are only added to non remote xpcAccessibleDocuments

Review of attachment 8710375 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/xpcom/xpcAccessibleDocument.h
@@ +29,4 @@
>  
>    xpcAccessibleDocument(ProxyAccessible* aProxy, uint32_t aInterfaces) :
> +    xpcAccessibleHyperText(aProxy, aInterfaces), mCache(kDefaultCacheLength),
> + mRemote(true) {}

nit: more (double) indent for mRemote initializer

Comment 27

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 8710378 [details] [diff] [review]
add DocAccessibleParent::GetXPCAccessible()

Review of attachment 8710378 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/ipc/DocAccessibleParent.cpp
@@ +294,5 @@
>  }
>  
> +xpcAccessibleGeneric*
> +DocAccessibleParent::GetXPCAccessible(ProxyAccessible* aProxy)
> +{

Should we assert mRemote here?

Comment 28

[Trevor Saunders (:tbsaunde)]

Attachment: allow caching xpc documents for remote documents

We need to handle the case that accessibility is enabled in a child process, but not the parent.  The easiest way to do that is to change from using a member to using a static pointer to a hash table.

Comment 29

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f35ea53092d7
https://hg.mozilla.org/integration/mozilla-inbound/rev/028866ac3b66
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c4c77b8e2e3
https://hg.mozilla.org/integration/mozilla-inbound/rev/8dbc0a23db6b
https://hg.mozilla.org/integration/mozilla-inbound/rev/1dc79d4dac30
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb2fb56c44aa
https://hg.mozilla.org/integration/mozilla-inbound/rev/f97fa91a4a9e
https://hg.mozilla.org/integration/mozilla-inbound/rev/1acf6d48e34c
https://hg.mozilla.org/integration/mozilla-inbound/rev/70f830b7665f
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd3e8b694f8b
https://hg.mozilla.org/integration/mozilla-inbound/rev/4044d673c2b2
https://hg.mozilla.org/integration/mozilla-inbound/rev/909c18f9fd6d
https://hg.mozilla.org/integration/mozilla-inbound/rev/6fa48455c395
https://hg.mozilla.org/integration/mozilla-inbound/rev/33fd55182ab7
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb4ffc4e5f67
https://hg.mozilla.org/integration/mozilla-inbound/rev/35675c2952cd
https://hg.mozilla.org/integration/mozilla-inbound/rev/100f1565de78

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f35ea53092d7
https://hg.mozilla.org/mozilla-central/rev/028866ac3b66
https://hg.mozilla.org/mozilla-central/rev/8c4c77b8e2e3
https://hg.mozilla.org/mozilla-central/rev/8dbc0a23db6b
https://hg.mozilla.org/mozilla-central/rev/1dc79d4dac30
https://hg.mozilla.org/mozilla-central/rev/cb2fb56c44aa
https://hg.mozilla.org/mozilla-central/rev/f97fa91a4a9e
https://hg.mozilla.org/mozilla-central/rev/1acf6d48e34c
https://hg.mozilla.org/mozilla-central/rev/70f830b7665f
https://hg.mozilla.org/mozilla-central/rev/bd3e8b694f8b
https://hg.mozilla.org/mozilla-central/rev/4044d673c2b2
https://hg.mozilla.org/mozilla-central/rev/909c18f9fd6d
https://hg.mozilla.org/mozilla-central/rev/6fa48455c395
https://hg.mozilla.org/mozilla-central/rev/33fd55182ab7
https://hg.mozilla.org/mozilla-central/rev/bb4ffc4e5f67
https://hg.mozilla.org/mozilla-central/rev/35675c2952cd
https://hg.mozilla.org/mozilla-central/rev/100f1565de78