Last Comment Bug 689105 - Accessibility in main window broken for VoiceOver, VO doesn't see anything but the title bar and its buttons
: Accessibility in main window broken for VoiceOver, VO doesn't see anything bu...
Status: RESOLVED FIXED
[bk1]
: regression
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: x86_64 Mac OS X
: -- major (vote)
: mozilla11
Assigned To: Hubert Figuiere [:hub]
:
Mentors:
Depends on:
Blocks: osxa11y 646368
  Show dependency treegraph
 
Reported: 2011-09-26 00:51 PDT by Marco Zehe (:MarcoZ) on PTO until August 15
Modified: 2012-02-01 14:00 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
proposed patch (871 bytes, patch)
2011-11-22 19:45 PST, Hubert Figuiere [:hub]
no flags Details | Diff | Splinter Review
proposed patch (780 bytes, patch)
2011-12-01 17:14 PST, Hubert Figuiere [:hub]
surkov.alexander: review-
Details | Diff | Splinter Review
proposed patch (1.83 KB, patch)
2011-12-02 12:44 PST, Hubert Figuiere [:hub]
no flags Details | Diff | Splinter Review
proposed patch v4.1 (1.83 KB, patch)
2011-12-02 15:25 PST, Hubert Figuiere [:hub]
surkov.alexander: review+
dbolter: review+
Details | Diff | Splinter Review
Final patch from patch queue (1.08 KB, patch)
2011-12-05 17:43 PST, Hubert Figuiere [:hub]
no flags Details | Diff | Splinter Review

Description Marco Zehe (:MarcoZ) on PTO until August 15 2011-09-26 00:51:13 PDT
STR:
1. Build current trunk with accessibility enabled.
2. Run the build.
3. Run VoiceOver.
4. If you get a "Nightly is not your current browser" dialog, notice that VoiceOver does see all the text in the dialog, plus the checkbox and the two buttons. You can press the No button with CTRL+Option+Space, for example.
5. Once you get to the main window, and Nightly displays its start page, VoiceOver does not see anything but the title bar and the close, minimize, and zoom buttons. Normally, it should also see toolbars, the main web area, and the status bar. But none of that is visible. You can find this out by navigating through the top level hierarchy with CTRL+Option+Arrow right and left.

This is a regression from some time in the Firefox 4 timeframe I believe, where I saw this working better. But finding a regression range is not really an option since no regular nightly build was ever built with accessibility enabled.
Comment 1 alexander :surkov 2011-09-26 01:05:49 PDT
Steven, if you get a chance please look into. You might be a best person to detect where we regressed. Thank you for the help.
Comment 2 Steven Michaud [:smichaud] (Retired) 2011-09-26 08:24:28 PDT
I'm currently swamped, so it'll be a while before I can get to this.

But until we get someone specifically working on Mac accessibility, it's probably true that I'm the best one to figure this out.
Comment 3 Hubert Figuiere [:hub] 2011-11-14 16:19:08 PST
To be honest, I just checked with a custom build of 3.6.24 and it is work than what is in Mozilla central. With Nightly build from mozilla central I can at least partially navigate through the webpage under some circumstances.

As for the dialog it isn't read either way, but we can use the VoiceOver navigation keys to dismiss it (buttons are read).

Definitely broken, not sure if it actually regressed.
Comment 4 Steven Michaud [:smichaud] (Retired) 2011-11-14 16:29:16 PST
Do a mozilla-central build from code just after my patch for bug 560939 landed.
Comment 5 Hubert Figuiere [:hub] 2011-11-16 14:53:40 PST
Using the Accessibility Validator, we actually have a tree that "randomly" miss complete subtrees, consistently with the accessibility issue we observe.
Needs more digging as to why this happens.

There are also some glitches in the implementation that cause some errors with the Validator, the AXMain and AXMinimized property missing from the dialog, and the some extra unexpected properties. I don't think these are fatal but I plan on fixing them too.
Comment 6 alexander :surkov 2011-11-16 20:44:50 PST
(In reply to Hub Figuiere [:hub] from comment #5)

> There are also some glitches in the implementation that cause some errors
> with the Validator, the AXMain and AXMinimized property missing from the
> dialog, and the some extra unexpected properties. I don't think these are
> fatal but I plan on fixing them too.

please file separate bugs for that.
Comment 7 Hubert Figuiere [:hub] 2011-11-22 19:45:27 PST
Created attachment 576392 [details] [diff] [review]
proposed patch

The proposed patch. It needs more testing, but I do believe this is the actual problem. It seems to solve all the issues I had with the hierarchy.

I need to test on a non debug build.
Comment 8 alexander :surkov 2011-11-22 21:02:14 PST
Comment on attachment 576392 [details] [diff] [review]
proposed patch

>diff --git a/accessible/src/mac/nsAccessibleWrap.mm b/accessible/src/mac/nsAccessibleWrap.mm
>--- a/accessible/src/mac/nsAccessibleWrap.mm
>+++ b/accessible/src/mac/nsAccessibleWrap.mm
>@@ -256,16 +256,19 @@ nsAccessibleWrap::IsIgnored()
> 
> void
> nsAccessibleWrap::GetUnignoredChildren(nsTArray<nsRefPtr<nsAccessibleWrap> > &aChildrenArray)
> {
>   // we're flat; there are no children.
>   if (nsAccUtils::MustPrune(this))
>     return;
> 
>+  // make sure the children are here.
>+  EnsureChildren();

You never should call it explicitly. Children are cached automatically when accessibility starts but that happens async. That could be a problem. Maybe we should deliver then events that tree was changed instead? Can you describe in details what happens here?
Comment 9 alexander :surkov 2011-11-22 21:05:46 PST
Btw, it'll be great to get regression bug. I think you could bisect extension (http://mercurial.selenic.com/wiki/BisectExtension) for that.
Comment 10 Hubert Figuiere [:hub] 2011-11-22 21:15:54 PST
What happens is that in some situation we don't have the children created. As you said it is done asynchronously and that's exactly what happens. Adding the "EnsureChildren()" make sure they are here when we query the accessible tree.

I guess we will have to deliver the events that tree was changed instead.
Comment 11 Hubert Figuiere [:hub] 2011-11-22 21:17:50 PST
To be honest, I'm not sure how it was a regression per see. Comment 3.
Comment 12 alexander :surkov 2011-11-25 02:54:32 PST
(In reply to Hub Figuiere [:hub] from comment #10)

> I guess we will have to deliver the events that tree was changed instead.

If this is a cause then nsaccessibility prootocol provides events AXUIElementCreated and AXUIElementDestroyed. But that shouldn't be issue since we never delivered them. Did you checked if mac accessible children cache doesn't get updated by any chance?
Comment 13 alexander :surkov 2011-11-25 05:09:47 PST
(In reply to alexander surkov from comment #12)
> Did you checked if mac accessible children
> cache doesn't get updated by any chance?

For example, nsAccessibleWrap::InvalidateChildren doesn't trigger on nsOuterDocAccessible, what could lead for example tab document accessible is not presented in mac cached children.
Comment 14 alexander :surkov 2011-11-25 06:06:28 PST
(In reply to alexander surkov from comment #13)
> (In reply to alexander surkov from comment #12)
> > Did you checked if mac accessible children
> > cache doesn't get updated by any chance?
> 
> For example, nsAccessibleWrap::InvalidateChildren doesn't trigger on
> nsOuterDocAccessible, what could lead for example tab document accessible is
> not presented in mac cached children.

yes, the problem mac cached children (mChildren) on root accessible weren't invalidated.
Comment 15 alexander :surkov 2011-11-25 06:09:13 PST
(In reply to alexander surkov from comment #14)

> yes, the problem mac cached children (mChildren) on root accessible weren't
> invalidated.

and those debug observations are right because initial tree creation doesn't trigger invalidateChildren. That'll be a fix.
Comment 16 alexander :surkov 2011-11-25 06:21:05 PST
btw, invalidateChildren approach doesn't work in either case because children invalidation on ignored accessible that has unignored children doesn't update mac children cache of the parent.
Comment 17 alexander :surkov 2011-12-01 12:15:55 PST
regression from bug 646368 which is sort of expected (see bug 705404 comment #5)
Comment 18 Hubert Figuiere [:hub] 2011-12-01 17:14:18 PST
Created attachment 578444 [details] [diff] [review]
proposed patch

This the new patch. Almost the same as the old one except that this time EnsureChildren() is called in -[mozAccessible children]

nsAccessibleWrap::GetUnignoredChildren() is only called from that location anyway.

-[mozAccessible children] is only called from the MacOS Universal Access API and caches the result until invalidated by a tree update.
Comment 19 alexander :surkov 2011-12-01 20:20:35 PST
Comment on attachment 578444 [details] [diff] [review]
proposed patch

Layout is responsible for time of accessible tree creation. The patch introduces extra code path for that and put the creation time out of our control. It's harder to debug any broken tree issues and it can lead to broken accessible tree because we operate on unfinished layout. So I wouldn't try this approach as long as we have alternative.
Comment 20 Hubert Figuiere [:hub] 2011-12-02 12:44:59 PST
Created attachment 578684 [details] [diff] [review]
proposed patch

I had to expose "IsInitialized" so that I don't cache an empty children list.
Comment 21 Hubert Figuiere [:hub] 2011-12-02 14:57:55 PST
Comment on attachment 578684 [details] [diff] [review]
proposed patch

cancelling review. There is a typo....
Comment 22 Hubert Figuiere [:hub] 2011-12-02 15:25:27 PST
Created attachment 578750 [details] [diff] [review]
proposed patch v4.1
Comment 23 Hubert Figuiere [:hub] 2011-12-02 15:27:35 PST
This fix the VoiceOver at startup. But reloading the page, or going to a new one make it empty again. This is also the case before the regression.
Comment 24 alexander :surkov 2011-12-04 20:55:51 PST
Comment on attachment 578750 [details] [diff] [review]
proposed patch v4.1


> - (NSArray*)children
> {
>   NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NIL;
> 
>   if (mChildren)
>     return mChildren;
>-    
>+  
>+  if(!mGeckoAccessible->IsInitialized())
>+    return nil;

use AreChildrenCached

if (mChildren || !mGeckoAccessible-AreChildrenCached())
  return mChildren;
Comment 25 Hubert Figuiere [:hub] 2011-12-05 17:43:05 PST
Created attachment 579167 [details] [diff] [review]
Final patch from patch queue

Can't commit with Level 1.

Has been run on a try server.
Comment 26 alexander :surkov 2011-12-05 22:11:44 PST
inbound land https://hg.mozilla.org/integration/mozilla-inbound/rev/95e631d313a3
Comment 27 Ed Morley [:emorley] 2011-12-06 10:05:17 PST
https://hg.mozilla.org/mozilla-central/rev/95e631d313a3

Note You need to log in before you can comment on or make changes to this bug.