Closed Bug 560939 Opened 10 years ago Closed 10 years ago

nsAccessibleWrap::GetNativeInterface() always returns NULL on OS X

Categories

(Core :: Disability Access APIs, defect, major)

All
macOS
defect
Not set
major

Tracking

()

RESOLVED FIXED

People

(Reporter: smichaud, Assigned: smichaud)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(2 files, 4 obsolete files)

nsAccessibleWrap::GetNativeInterface() always returns NULL on OS X,
because mNativeWrapper is always NULL when it's called.  This is
because nsAccessibleWrap::Init() is never called on the nsAccessible
corresponding to two top-level objects -- an nsXULDocument (whose
nsAccessible is an nsRootAccessibleWrap object), and an nsHTMLDocument
(whose nsAccessible is an nsDocAccessible object).

This is in turn because nsRootAccessible::Init() and
nsDocAccessible::Init() override nsAccessibleWrap::Init() without ever
calling the latter.

Because (on OS X) nsAccessibleWrap::GetNativeInterface() always fails
on a window's top-level objects, none of a window's lower-level
nsAccessible objects are visible to an accessibility client (like
Accessibility Inspector or Accessibility Verifier).  So most things
that should be "accessible" (and *are* "accessible" on other
platforms), aren't "accessible" on OS X.

As best I can tell this has been true from the beginning -- from the
time our accessibility code first supported OS X.
Attached patch Potential fix (obsolete) — Splinter Review
Here's a patch that fixes the problem.  I've tested it in both 32-bit
builds (on OS X 10.5.8) and 64-bit builds (on OS X 10.6.3).  I tested
with Accessibility Inspector and Accessibility Verifier (available
under /Developer/Applications/Utilties/Accessibility Tools/).

I'm not very familiar with the accessibility code, so I'm not sure
this patch is the best way to fix the bug, or even that it won't have
undesirable side effects.  But I didn't have any trouble in my (rather
brief) tests.  And I tried my best to conform to how things are done
in existing code (several other nsAccessible subclasses also have
Init() methods that call nsAccessibleWrap::Init()).

Because nsRootAccessible::Init() calls nsDocAccessibleWrap::Init()
(which is typedefed to nsDocAccessible::Init() on OS X), I didn't need
to also add a call to nsAccessibleWrap::Init() there.

There are still a few cases where
nsAccessibleWrap::GetNativeInterface() returns NULL -- notably for
nsLinkableAccessible objects and their subclasses.  But this is
because AncestorIsFlat() returns TRUE for those objects.

A tryserver build will follow in a few hours.  It will be a 32-bit
trunk build that (naturally) supports accessibility.

Note that Accessibility Verifier returns quite a few errors.  But I
suspect they have nothing to do with this patch -- that they are
caused by errors in other code that's now being exercised for the
first time.
Assignee: nobody → smichaud
Attachment #440600 - Flags: review?(bolterbugz)
Attachment #440600 - Flags: review?(surkov.alexander)
Blocks: osxa11y
A bit surprisingly, there are no failures in the a11y mochitests (make mochitest-a11y) with my patch.

I ran them locally with a 32-bit build on OS X 10.5.8.
Steven, great investigative work thanks! Note our a11y mochitests don't test our platform layers, only our common core API nsAccessible* and friends. So they are expected to pass on OSX.
> Steven, great investigative work thanks!

You're most welcome.  Glad I could help.

> Note our a11y mochitests don't test our platform layers, only our
> common core API nsAccessible* and friends. So they are expected to
> pass on OSX.

I suspected something like that :-)  But thanks for telling me.
Depends on: 543961
Keywords: regression
Comment on attachment 440600 [details] [diff] [review]
Potential fix

that's not right fix since document accessible will register itself in own child cache (on os x), so it backports the patch from bug 543961. I'll try to figure out more correct fix.

Steve, thank you much for the help, for the time you've spent on this (and of course for that you've found the problem)!
Attachment #440600 - Flags: review?(surkov.alexander)
Attachment #440600 - Flags: review?(bolterbugz)
Probably the best way would be to copy/paste nsAccessibleWrap::Init code to nsDocAccessibleWrap::Init.
How about this?

> that's not right fix since document accessible will register itself
> in own child cache (on os x), so it backports the patch from bug
> 543961.

I assume that by "backports" you mean something like "breaks".

> Steve, thank you much for the help, for the time you've spent on
> this (and of course for that you've found the problem)!

You're most welcome.

The underlying problem has been bugging me, too, since I first
encountered it a year ago.  But before now I never knew enough to do
anything about it.
Attachment #440600 - Attachment is obsolete: true
Attachment #440762 - Flags: review?(surkov.alexander)
Attached patch Fix rev2 (tighten up) (obsolete) — Splinter Review
Or better still.
Attachment #440762 - Attachment is obsolete: true
Attachment #440764 - Flags: review?(surkov.alexander)
Attachment #440762 - Flags: review?(surkov.alexander)
Steven, I realize it's really good point to share the code. However possibly not in this case. It might be easier and readable to create nsDocAccessibleWrap and override Init() method. Especially it makes sense because I believe AncestorIsFlat() check (used in nsAccessibleWrap::Init()) doesn't make sense on documents.
(In reply to comment #8)

> > that's not right fix since document accessible will register itself
> > in own child cache (on os x), so it backports the patch from bug
> > 543961.
> 
> I assume that by "backports" you mean something like "breaks".

yep, I meant it's partial back out that patch.

> The underlying problem has been bugging me, too, since I first
> encountered it a year ago.  But before now I never knew enough to do
> anything about it.

Do you mean the problem exists during one year?
> Do you mean the problem exists during one year?

By "underlying problem" I mean the following (from comment #0):

> Because (on OS X) nsAccessibleWrap::GetNativeInterface() always fails
> on a window's top-level objects, none of a window's lower-level
> nsAccessible objects are visible to an accessibility client (like
> Accessibility Inspector or Accessibility Verifier).  So most things
> that should be "accessible" (and *are* "accessible" on other
> platforms), aren't "accessible" on OS X.

This problem (and this bug) have existed for *at least* a year.
That's interesting because I thought it must be regression from bug 543961 which was landed couple months ago.
> That's interesting because I thought it must be regression from bug
> 543961 which was landed couple months ago.

I simply don't know ... and I really don't have time to find out.

All I do know is that the underlying problem (objects inside a window
not being "accessible") has existed at least since I first saw it a
year ago.  And (judging by the number of errors in Accessibility
Verifier) I suspect it's existed from the time our accessibility code
first supported OS X.
OK then, how about this?
Attachment #440764 - Attachment is obsolete: true
Attachment #440803 - Flags: review?(surkov.alexander)
Attachment #440764 - Flags: review?(surkov.alexander)
Comment on attachment 440803 [details] [diff] [review]
Fix rev3 (follow Alex's suggestion)


>+class nsDocAccessibleWrap: public nsDocAccessible
>+{
>+public:
>+    nsDocAccessibleWrap(nsIDOMNode *aNode, nsIWeakReference *aShell);
>+    virtual ~nsDocAccessibleWrap();
>+
>+    // creates the native accessible connected to this one.

nit: use /** */ style for comments
nit: please add "// nsIAccessNode" before the comment

>+    NS_IMETHOD Init ();

nit: virtual nsresult, since NS_IMETHOD used for interface methods (idl) prototype, though I know there is no difference, just style issue

>+ * Contributor(s):
>+ *   Original Author: Steven Michaud (smichaud@pobox.com)

nit: usually the style: Steven Michaud <smichaud@pobox.com> (original author) is used in a11y

>+#include "nsDocAccessibleWrap.h"
>+
>+#import "mozAccessibleWrapper.h"
>+
>+nsDocAccessibleWrap::nsDocAccessibleWrap(nsIDOMNode *aDOMNode, nsIWeakReference *aShell) :

nit: if it's more than 80 chars then do

nsDocAccessibleWrap::
  nsDocAccessibleWrap(nsIDOMNode *aDOMNode, nsIWeakReference *aShell) :

>+nsresult
>+nsDocAccessibleWrap::Init () 
>+{
>+  nsresult rv = nsDocAccessible::Init();
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  if (!mNativeWrapper) {

actually it shoudn't be possible that Init() is called twice. I'm fine to have a protection from this but please add NS_ASSERTION.

>+    // Create our native object using the class type specified in GetNativeType().
>+    mNativeWrapper = new AccessibleWrapper (this, GetNativeType());

this line makes me worry, we create base class for document accessible, however nsChildView is created for chrome and for content documents if I get right. I wonder if we need to use mozRootAccessible here or similar. Any way this way is how it worked before it was broken so it's not a subject for this bug.

r=me with nits fixed.

Again, thank you for doing this!
Attachment #440803 - Flags: review?(surkov.alexander) → review+
The tree is currently closed, and may stay that way for a while.  So
here's another revision, with (hopefully) all the nits fixed.

> I wonder if we need to use mozRootAccessible here or similar.

As best I can tell the answer is "no".

In mozDocAccessible.h there's the following comment:

/*
  The root accessible. There is one per window.
  Created by the nsRootAccessibleWrap.
*/

This implies there should only be one mozRootAccessible per window --
the one created by nsRootAccessibleWrap.
Attachment #440803 - Attachment is obsolete: true
Attachment #440848 - Flags: review?(surkov.alexander)
Comment on attachment 440848 [details] [diff] [review]
Fix rev4 (fix nits)

r=me, thank you
Attachment #440848 - Flags: review?(surkov.alexander) → review+
(In reply to comment #18)

> This implies there should only be one mozRootAccessible per window --
> the one created by nsRootAccessibleWrap.

ok
Comment on attachment 440848 [details] [diff] [review]
Fix rev4 (fix nits)

I just wanted to confirm that, after building with this patch locally, I can confirm that we're back to a state where we can work on bugs such as bug 499927 and bug 499931, as well as performance. VoiceOver again sees the dialogs and page content with this patch. Thanks Steven!
> VoiceOver again sees the dialogs and page content with this patch.

Glad to hear it.

> Thanks Steven!

You're very welcome!

> I just wanted to confirm that, after building with this patch
> locally, I can confirm that we're back to a state where we can work
> on bugs such as bug 499927 and bug 499931, as well as performance.

Hopefully you'll no longer need my help on those two bugs :-)

By the way, it's while working on them that I first noticed what I've
been calling "the underlying problem" -- objects inside a window not
being "accessible".  It wasn't quite a year ago ... but I guess it now
seems like it :-)
For the record, here's the debugging patch I used to figure out this
bug.  I've updated it to remove false leads and to include the latest
revision of my patch.  Most of what I say in comment #0 and comment #1
is based upon logs that it produced.

To compile it, you need to add the following you your mozconfig (RTTI
is Run-Time Type Information):

ac_add_options --enable-cpp-rtti

This is just to show how much you can accomplish without stepping
through code in a debugger.
The tree is open, but currently in bad shape.  I'll take another look this afternoon.
Landed on trunk:
http://hg.mozilla.org/mozilla-central/rev/db79d2ce808c
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.