Closed Bug 689105 Opened 8 years ago Closed 8 years ago

Accessibility in main window broken for VoiceOver, VO doesn't see anything but the title bar and its buttons

Categories

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

x86_64
macOS
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla11

People

(Reporter: MarcoZ, Assigned: hub)

References

(Blocks 1 open bug)

Details

(Keywords: regression, Whiteboard: [bk1])

Attachments

(2 files, 3 obsolete files)

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.
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.
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.
Assignee: nobody → hfiguiere
Whiteboard: [bk1]
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.
Do a mozilla-central build from code just after my patch for bug 560939 landed.
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.
(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.
Attached patch proposed patch (obsolete) — Splinter Review
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 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?
Btw, it'll be great to get regression bug. I think you could bisect extension (http://mercurial.selenic.com/wiki/BisectExtension) for that.
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.
To be honest, I'm not sure how it was a regression per see. Comment 3.
(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?
(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.
(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.
(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.
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.
Depends on: 705404
regression from bug 646368 which is sort of expected (see bug 705404 comment #5)
Blocks: 646368
Attached patch proposed patch (obsolete) — Splinter Review
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.
Attachment #576392 - Attachment is obsolete: true
Attachment #578444 - Flags: review?(surkov.alexander)
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.
Attachment #578444 - Flags: review?(surkov.alexander) → review-
Attached patch proposed patch (obsolete) — Splinter Review
I had to expose "IsInitialized" so that I don't cache an empty children list.
Attachment #578444 - Attachment is obsolete: true
Attachment #578684 - Flags: review?(surkov.alexander)
Comment on attachment 578684 [details] [diff] [review]
proposed patch

cancelling review. There is a typo....
Attachment #578684 - Flags: review?(surkov.alexander)
Attachment #578684 - Attachment is obsolete: true
Attachment #578750 - Flags: review?
Attachment #578750 - Flags: review? → review?(surkov.alexander)
Attachment #578750 - Flags: review?(bolterbugz)
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 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;
Attachment #578750 - Flags: review?(surkov.alexander) → review+
Attachment #578750 - Flags: review?(bolterbugz) → review+
Can't commit with Level 1.

Has been run on a try server.
Attachment #579167 - Flags: checkin?
Attachment #579167 - Flags: checkin?
https://hg.mozilla.org/mozilla-central/rev/95e631d313a3
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
No longer depends on: 705404
You need to log in before you can comment on or make changes to this bug.