1110510 - Create proxies for document accessibles

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Trevor Saunders (:tbsaunde)]

I'm not sure if this the cause of bug 1099153, but its certainly a bug.  Because document accessibles are created differently from other proxies we need make sure to create proxies there too.

Comment 1

[Trevor Saunders (:tbsaunde)]

Attachment: make sure to create proxies for documents

Comment 2

[Trevor Saunders (:tbsaunde)]

Attachment: make sure to create and destroy proxies for documents

Comment 3

[alexander :surkov (:asurkov)]

Comment on attachment 8535288 [details] [diff] [review]
make sure to create proxies for documents

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

the patch looks reasonable but I do have some lack of knowledge of e10s a11y architecture so it might make sense to ask for another review, it's up to you though

::: accessible/ipc/ProxyAccessible.h
@@ +87,5 @@
>  
>  protected:
> +  ProxyAccessible(DocAccessibleParent* aThisAsDoc) :
> +    mParent(nullptr), mDoc(aThisAsDoc), mWrapper(0), mID(0),
> +    mRole(roles::DOCUMENT)

generic proxy accessible initialized by document role looks weird

Comment 4

[alexander :surkov (:asurkov)]

Comment on attachment 8535727 [details] [diff] [review]
make sure to create and destroy proxies for documents

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

::: accessible/ipc/DocAccessibleParent.cpp
@@ +148,5 @@
> +
> +void
> +DocAccessibleParent::ActorDestroy(ActorDestroyReason aWhy)
> +{
> +  ProxyDestroyed(this);

don't you need similar for DocManager?

Comment 5

[Trevor Saunders (:tbsaunde)]

> ::: accessible/ipc/ProxyAccessible.h
> @@ +87,5 @@
> >  
> >  protected:
> > +  ProxyAccessible(DocAccessibleParent* aThisAsDoc) :
> > +    mParent(nullptr), mDoc(aThisAsDoc), mWrapper(0), mID(0),
> > +    mRole(roles::DOCUMENT)
> 
> generic proxy accessible initialized by document role looks weird

Its a constructor only used for a subclass which is kind of weird I guess, and its not a fully correct solution, but making documents always have correct role doesn't seem like super urgent problem.

Comment 6

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #4)
> Comment on attachment 8535727 [details] [diff] [review]
> make sure to create and destroy proxies for documents
> 
> Review of attachment 8535727 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/ipc/DocAccessibleParent.cpp
> @@ +148,5 @@
> > +
> > +void
> > +DocAccessibleParent::ActorDestroy(ActorDestroyReason aWhy)
> > +{
> > +  ProxyDestroyed(this);
> 
> don't you need similar for DocManager?

no, ActorDestroy is called on both root and not root DocAccessible Parents.

Comment 7

[alexander :surkov (:asurkov)]

yeah, it's just weird, can you pass role as constructor argument?

Comment 8

[Trevor Saunders (:tbsaunde)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a8b8d912bd69

Comment 9

[Ryan VanderMeulen (PTO, back 6-April)]

https://hg.mozilla.org/mozilla-central/rev/a8b8d912bd69

Comment 10

[Daniel Holbert [:dholbert]]

Comment on attachment 8535727 [details] [diff] [review]
make sure to create and destroy proxies for documents

>+DocAccessibleParent::ActorDestroy(ActorDestroyReason aWhy)
>+{
>+  ProxyDestroyed(this);
>+  MOZ_ASSERT(mChildDocs.IsEmpty(),
>+      "why wheren't the child docs destroyed already?");

FWIW, I just pushed a trivial typo fix for this assertion-message (s/wheren't/weren't/):
  https://hg.mozilla.org/integration/mozilla-inbound/rev/a170277d1c67

(This assertion flew by in my terminalspew, and I tried to find it in the backscroll with my terminal's "find" functionality, but I got no hits because I was spelling it correctly when searching. :)  Fixing it so that this doesn't bite anyone else.)

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a170277d1c67