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

RESOLVED FIXED in mozilla12

Status

()

Core
Disability Access APIs
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: hub, Assigned: hub)

Tracking

(Blocks: 1 bug)

Trunk
mozilla12
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

6 years ago
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).
(Assignee)

Updated

6 years ago
Blocks: 342989
(Assignee)

Updated

6 years ago
Assignee: nobody → hub
(Assignee)

Comment 1

6 years ago
This also happen with switching tabs.

Comment 2

6 years ago
that could be because of bug 705404 (mac tree invalidation is broken)
(Assignee)

Comment 3

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

That could be.
(Assignee)

Comment 4

6 years ago
Created attachment 581823 [details] [diff] [review]
Make sure to update the Mac accessible tree. r=
(Assignee)

Updated

6 years ago
Assignee: hub → nobody
Summary: After loading a new page, the WebArea is not longer accessible → After loading a new page, the WebArea is no longer accessible
(Assignee)

Updated

6 years ago
Assignee: nobody → hub
(Assignee)

Comment 5

6 years ago
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
(Assignee)

Comment 6

6 years ago
To apply the patch one need the patch from bug 705404 first.
(Assignee)

Updated

6 years ago
Attachment #581823 - Flags: review?(trev.saunders)
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()
(Assignee)

Comment 8

6 years ago
(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*.
(Assignee)

Comment 9

6 years ago
Created attachment 582379 [details] [diff] [review]
Make sure to update the Mac accessible tree.
(Assignee)

Updated

6 years ago
Attachment #581823 - Attachment is obsolete: true
Attachment #581823 - Flags: review?(trev.saunders)
(Assignee)

Updated

6 years ago
Attachment #582379 - Flags: review?(trev.saunders)
> > >+  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 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
(Assignee)

Comment 12

6 years ago
Created attachment 583361 [details] [diff] [review]
Make sure to update the Mac accessible tree.
(Assignee)

Updated

6 years ago
Attachment #582379 - Attachment is obsolete: true
Attachment #582379 - Flags: review?(trev.saunders)
(Assignee)

Comment 13

6 years ago
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.
(Assignee)

Updated

6 years ago
Attachment #583361 - Flags: review?(trev.saunders)
(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 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
Attachment #583361 - Flags: review?(trev.saunders) → review+
(Assignee)

Comment 16

6 years ago
Created attachment 583937 [details] [diff] [review]
Make sure to update the Mac accessible tree.
(Assignee)

Updated

6 years ago
Attachment #583361 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #583937 - Flags: checkin?(trev.saunders)
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
Attachment #583937 - Flags: checkin?(trev.saunders)
https://hg.mozilla.org/mozilla-central/rev/3d0cf1dd0a5a
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12

Comment 19

6 years ago
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.
You need to log in before you can comment on or make changes to this bug.