708144 - After loading a new page, the WebArea is no longer accessible

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Hubert Figuiere [:hub]]

After loading a new page, the WebArea is not longer accessible

To reproduce:

1. install Nightly on Mac with accessibility enabled.
2. Load a new page

The new page content is no longer accessible.

This is IMHO, not the same regression as bug 689105 (I could reproduce before it got broken).

Comment 1

[Hubert Figuiere [:hub]]

This also happen with switching tabs.

Comment 2

[alexander :surkov (:asurkov)]

that could be because of bug 705404 (mac tree invalidation is broken)

Comment 3

[Hubert Figuiere [:hub]]

(In reply to alexander :surkov from comment #2)
> that could be because of bug 705404 (mac tree invalidation is broken)

That could be.

Comment 4

[Hubert Figuiere [:hub]]

Attachment: Make sure to update the Mac accessible tree. r=

Comment 5

[Hubert Figuiere [:hub]]

Patch attached is what I think is a solution to the problem. I wonder if it was deliberate for nsOuterDocAccessible to only call nsAccessible instead of its direct parent nsAccessibleWrap.

I have submitted the patch to a try server.
http://dev.philringnalda.com/tbpl/?tree=Try&rev=4b3aff5b5232

Comment 6

[Hubert Figuiere [:hub]]

To apply the patch one need the patch from bug 705404 first.

Comment 7

[Trevor Saunders (:tbsaunde)]

Comment on attachment 581823 [details] [diff] [review]
Make sure to update the Mac accessible tree. r=

>+  // get the native obj-c object (mozAccessible)
>+  NS_IMETHOD GetNativeInterface (void **aOutAccessible);
>+  
>+  // the objective-c |Class| type that this accessible's native object
>+  // should be instantied with.   used on runtime to determine the
>+  // right type for this accessible's associated native object.
>+  virtual Class GetNativeType ();

while your changing these make the comments

/**
 * blah blah blah
 */

>+  // ignored means that the accessible might still have children, but is not displayed
>+  // to the user. it also has no native accessible object represented for it.

same

>+  bool HasPopup () {
>+    return (NativeState() & mozilla::a11y::states::HASPOPUP);
>+  }

while your here

inline bool HasPopup()
  { return NativeState() & mozilla::a11y::states::HASPOPUP; }

>+  bool IsNativeInited() const {
>+    return mNativeInited;
>+  }

hiding the  member var seems pointless, and without looking at mNativeObject pretty useless outside of GetNativeObject / Shutdown.

btw please use the style suggested for HasPopup()

>+nsAccessibleWrap::AppendChild(nsAccessible *aAccessible)
>+{
>+  NS_OBJC_BEGIN_TRY_ABORT_BLOCK;

this is C++ method  why do we need this?  Shouldn't be enough to have one place catch the exception before it propogates to the system stuff calling us?

>+  if(IsNativeInited())
>+    [GetNativeObject() invalidateChildren];

you have no gaurentee so far as I can see that GetNativeObject() returns an object, the case of an accessible who is initialized, but doesn't get a native.

Invalidating all of the children seems like a  heavy weight solution here, why not get the native and append it to the array, if the array is currently cached.  Do you have reason to believe this is faster?

>+nsAccessibleWrap::RemoveChild(nsAccessible *aAccessible)
>+{
>+  NS_OBJC_BEGIN_TRY_ABORT_BLOCK;

C++ again so why is this necessary?

>+  if(IsNativeInited())
>+    [GetNativeObject() invalidateChildren];

same questions as for the same part of AppendChild()

Comment 8

[Hubert Figuiere [:hub]]

(In reply to Trevor Saunders (:tbsaunde) from comment #7)

> >+  bool IsNativeInited() const {
> >+    return mNativeInited;
> >+  }
> 
> hiding the  member var seems pointless, and without looking at mNativeObject
> pretty useless outside of GetNativeObject / Shutdown.

That's what it is intended for. Only determining if we have tentatively create the native object.


> >+nsAccessibleWrap::AppendChild(nsAccessible *aAccessible)
> >+{
> >+  NS_OBJC_BEGIN_TRY_ABORT_BLOCK;
> 
> this is C++ method  why do we need this?  Shouldn't be enough to have one
> place catch the exception before it propogates to the system stuff calling
> us?

I talked to smichaud and he told me "it is deprecated, don't add the new one. No need to remove the old ones". So be it, they are gone.

> >+  if(IsNativeInited())
> >+    [GetNativeObject() invalidateChildren];
> 
> you have no gaurentee so far as I can see that GetNativeObject() returns an
> object, the case of an accessible who is initialized, but doesn't get a
> native.

It is perfectly fine if it is nil. There is a guarantee that calling an Objective-C method with nil is safe. All I want here is to not needlessly instantiate the native object as if it is not, there is nothing to invalidate.

Ok fine, I'll remove it and just call on mNativeObject.

> Invalidating all of the children seems like a  heavy weight solution here,
> why not get the native and append it to the array, if the array is currently
> cached.  Do you have reason to believe this is faster?

I assume that we are gonna a bunch of add / remove. Remove can't be made fast unless I change the lookup of individual objects to something that's not linear.
Invalidate just remove them all and then *later*, we'll recreate them *on demand*.

Comment 9

[Hubert Figuiere [:hub]]

Attachment: Make sure to update the Mac accessible tree.

Comment 10

[Trevor Saunders (:tbsaunde)]

> > >+  NS_OBJC_BEGIN_TRY_ABORT_BLOCK;
> > 
> > this is C++ method  why do we need this?  Shouldn't be enough to have one
> > place catch the exception before it propogates to the system stuff calling
> > us?
> 
> I talked to smichaud and he told me "it is deprecated, don't add the new
> one. No need to remove the old ones". So be it, they are gone.

ok, thanks

> > >+  if(IsNativeInited())
> > >+    [GetNativeObject() invalidateChildren];
> > 
> > you have no gaurentee so far as I can see that GetNativeObject() returns an
> > object, the case of an accessible who is initialized, but doesn't get a
> > native.
> 
> It is perfectly fine if it is nil. There is a guarantee that calling an
> Objective-C method with nil is safe. All I want here is to not needlessly

welp

> instantiate the native object as if it is not, there is nothing to
> invalidate.
> 
> Ok fine, I'll remove it and just call on mNativeObject.

I would have suggested if mIsInited, but this seems even clearer what's going on, so  good :)

> > Invalidating all of the children seems like a  heavy weight solution here,
> > why not get the native and append it to the array, if the array is currently
> > cached.  Do you have reason to believe this is faster?
> 
> I assume that we are gonna a bunch of add / remove. Remove can't be made
> fast unless I change the lookup of individual objects to something that's
> not linear.

invalidating all of the children is also linear, and I'd expect has a worse constant.

> Invalidate just remove them all and then *later*, we'll recreate them *on
> demand*.

yes, I know, but as well as invalidateing the children being linear  getting the unignored children is super linear

Comment 11

[Trevor Saunders (:tbsaunde)]

Comment on attachment 582379 [details] [diff] [review]
Make sure to update the Mac accessible tree.

>+  /**
>+   * Ignored means that the accessible might still have children, but is not displayed
>+   * to the user. it also has no native accessible object represented for it.
>+   */

over 80 chars.

>+   * Returns this accessible's all children, adhering to "flat" accessibles by not 
>+   * returning their children.

same

Comment 12

[Hubert Figuiere [:hub]]

Attachment: Make sure to update the Mac accessible tree.

Comment 13

[Hubert Figuiere [:hub]]

removing a children still invalidate the whole tree. This is because I haven't found an easier solution yet that would appear to be much faster. It works, and I'll see if this has a real impact on performance as we have a lot to tackle in that area.

Comment 14

[Trevor Saunders (:tbsaunde)]

(In reply to Hub Figuiere [:hub] from comment #13)
> removing a children still invalidate the whole tree. This is because I
> haven't found an easier solution yet that would appear to be much faster. It
> works, and I'll see if this has a real impact on performance as we have a
> lot to tackle in that area.

I'd advise just doing the obvious linear time thing, given the current archetecture removing an accessible is already linear, and I'd think in most"reasonable" cases N has less than some smallish constant.  However this patch doesn't actively harm anything here so...

Comment 15

[Trevor Saunders (:tbsaunde)]

Comment on attachment 583361 [details] [diff] [review]
Make sure to update the Mac accessible tree.

r=me with comments fixed

>+  // if mChildren is nil, then we don't even need to bother
>+  if (mChildren) {

early return here

>+  if (removed && mNativeObject) {
>+    [mNativeObject invalidateChildren];
>+  }

get rid of curleys

Comment 16

[Hubert Figuiere [:hub]]

Attachment: Make sure to update the Mac accessible tree.

Comment 17

[Trevor Saunders (:tbsaunde)]

Comment on attachment 583937 [details] [diff] [review]
Make sure to update the Mac accessible tree.

landed https://hg.mozilla.org/integration/mozilla-inbound/rev/3d0cf1dd0a5a

Comment 18

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/3d0cf1dd0a5a

Comment 19

[alexander :surkov (:asurkov)]

I realize small steps on case by case basis gives certain results but I'd say it makes sense to get a big picture (like to fix bug 705404 as it was stated originally), otherwise the logic gets complicated/not performant and that could make the work a time wasting.